Re: Review: extension template

Salut Dimitri,

On 07/06/2013 10:30 PM, Dimitri Fontaine wrote:
> Yes, I did share this viewpoint over the naming of the feature, but Tom
> insisted that we already have those kind of templates for text search.

I think it's a question of what mental model we want extensions to
follow. See my other mail. IMO "inline" could well serve as a
differentiator against out-of-line, i.e. file-system based extensions
(or templates).

> Could you go into more details into your ideas here? I don't understand
> what you're suggesting.

Sorry for not making that clearer. See my follow-up mail "template-ify
(binary) extensions":
http://archives.postgresql.org/message-id/51D72C1D.7010609@bluegap.ch

>> Compiling pg_upgrade_support in contrib fails:
>>
>>> $SRC/contrib/pg_upgrade_support/pg_upgrade_support.c:193:8:
>>> error: too few arguments to function ‘InsertExtensionTuple’
>
> I don't have that problem here. Will try to reproduce early next week.

'make' followed by 'make -C contrib' reproduces that for me.

> Will have a look at what it takes to implement support for better error
> messages. May I suggest to implement that later, though?

Great, thanks. I agree this is not a major issue and can be deferred.

> The idea here is to protect against mixing the file based extension
> installation mechanism with the catalogs one. I can see now that the
> already installed extension could have been installed using a template
> in the first place, so that message now seems strange.
>
> I still think that we shouldn't allow creating a template for an
> extension that is already installed, though. Do you have any suggestions
> for a better error message?

If we go for the template model, I beg to differ. In that mind-set, you
should be free to create (or delete) any kind of template without
affecting pre-existing extensions.

However, in case we follow the ancestor-derivate or link model with the
pg_depend connection, the error message seems fine.

>> However, it's possible to enable an extension and then rename its
>> template. The binding (as in pg_depend) is still there, but the above
>> error (in that case "extension $OLD_NAME already existing") certainly
>> doesn't make sense. One can argue whether or not an extension with a
>> different name is still the same extension at all...
>
> When renaming a template, the check against existing extensions of the
> same name is made against the new name of the template, so I don't see
> what you say here in the code. Do you have a test case?

Consider this (not actually tested again, only off the top of my head):

# CREATE TEMPLATE FOR EXTENSION foo ...
ERROR: Extension "bar" already existing

"Uh? .. so what?" is my reaction to such an error message. It fails to
make the distinction between template and extension. Or rather parent
and derivate. The link between the two is the actual reason for the failure.

In any case, I'm arguing this "template" renaming behavior (and the
subsequent error) are the wrong thing to do, anyways. Because an
extension being linked to a parent of a different name is weird and IMO
not an acceptable state.

In the link model, the name should better be thought of as a property of
the parent. A rename of it should thus rename the derived extensions in
all databases. That would prevent the nasty case of having a parent with
different name than the derivative extension. (I note that existing
file-system extensions do not work that way. They are a lot closer to
the template model. However, that's just an argument for following that
as well for inline extensions and dropping the pg_depend link between
extension and template.)

In the template model, renaming the template should not have an effect
on any extension. Neither should creation or deletion of any template.
Thus, creating a template with the same name as a pre-existing extension
(in any database) is a completely fine and valid operation. No error
needs to be thrown. This nicely shows why I currently favor the template
approach: it seems easier to understand *and* easier to implement.

>> Trying to alter an inexistent or file-system stored extension doesn't
>> throw an error, but silently succeeds. Especially in the latter case,
>> that's utterly misleading. Please fix.
>
> Fixed in my github branch

Nice.

>> That started to get me worried about the effects of a mixed
>> installation, but I quickly figured it's not possible to have a full
>> version on disk and then add an incremental upgrade via the system
>> catalogs. I think that's a fair limitation, as mixed installations would
>> pose their own set of issues. On the other hand, isn't ease of upgrades
>> a selling point for this feature?
>
> The main issue to fix when you want to have that feature, which I want
> to have, is how to define a sane pg_dump policy around the thing. As we
> couldn't define that in the last rounds of reviews, we decided not to
> allow the case yet.

I bet that's because people have different mental models in mind. But I
probably stressed that point enough by now...

> I think that's a fair remark that we want to get there eventually, and
> that like the superuser only limitation, that's something for a future
> patch and not this one.

I agree to defer a specific implementation. I disagree in that we should
*not* commit something that follows a mixture of mental models
underneath and sets in stone a strange API we later need to be
compatible with.

Specifically, I request to either follow the template model more closely
(accompanied by a separate patch to adjust binary, out-of-line
templates) or follow the link model more closely. The current naming
doesn't match any of the two, so renaming seems inevitable.

> Yeah, we might want to find some better naming for the on-file extension
> "templates". We can't just call them extension templates though because
> that is the new beast implemented in that patch and it needs to be part
> of your pg_dump scripts, while the main feature of the file based kind
> of templates is that they are *NOT* part of your dump.

That's one issue where the mixture of concepts shines through, yeah. I
fear simply (re)naming things is not quite enough, at this point.
Especially because the thing on the file-system is closer to being a
template than the system catalog based variant.

[ Maybe we could even go with "template extensions" being the
file-system ones vs. "inline extensions" being the system catalog
ones... In that case, they would follow different mental models. Which
might even be a reasonable solution. However, we need to be aware of
that difference and document it properly, so that users have a chance to
grasp the nifty difference. I'd rather prefer to eliminate such nifty
differences, though. ]

> It's just something I forgot to cleanup when completing the feature set.
> Cleaned up in my git branch.

Great!

>> src/backend/commands/event_trigger.c, definition of
>> event_trigger_support: several unnecessary whitespaces added. These make
>> it hard(er) than necessary to review the patch.
>
> Here with the following command I see no problem, please advise:
>
> git diff --check --color-words postgres/master.. -- src/backend/commands/event_trigger.c

<rant>That's why I personally don't trust git.</rant> Please look at the
patch you submitted.

> Markus Wanner <markus(at)bluegap(dot)ch> writes:
>> Dimitri, do you agree to resolve with "returned with feedback" for this
>> CF? Or how would you like to proceed?
>
> I'd like to proceed, it's the 3rd development cycle that I'm working on
> this idea

Okay, I'm certainly not opposed to that and welcome further development.
I didn't change the status, so it should still be "waiting on author",
which seems reasonable to me, ATM.

> (used to be called "Inline Extensions", I also had a selective
> pg_dump patch to allow dumping some extension scripts, etc). I really
> wish we would be able to sort it out completely in this very CF and
> that's why I'm not proposing any other patch this time.

I understand it's tedious work, sometimes. Please look at this as
incremental progress. I would not have been able to reason about mental
models without your patch. Thanks for that, it was a necessary and good
step from my point of view. Please keep up the good work!

Regards

Markus Wanner