Re: Review: extension template

Hi,

Please find attached to this mail version 9 of the Extension Templates
patch with fixes for the review up to now.

Markus Wanner <markus(at)bluegap(dot)ch> writes:
>> 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.

Then what happens at pg_restore time? the CREATE EXTENSION in the backup
script will suddenly install the other extension's that happen to have
the same name? I think erroring out is the only safe way here.

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

It's all about pg_restore really, but it's easy to forget about that and
get started into other views of the world. I'll try to be better at not
following those tracks and just hammer my answers with "pg_restore" now.

> 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.

Yes, you're right on spot here. So I've amended the patch to implement
the following behavior (and have added a new regression test):

# CREATE TEMPLATE FOR EXTENSION foo VERSION 'v' AS '';
# CREATE EXTENSION foo;
# ALTER TEMPLATE FOR EXTENSION foo RENAME TO bar;
ERROR: 55006: template for extension "foo" is in use
DETAIL: extension "foo" already exists
LOCATION: AlterExtensionTemplateRename, template.c:1040
STATEMENT: ALTER TEMPLATE FOR EXTENSION foo RENAME TO bar;

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

FWIW, I do agree. But my understanding is that the community consensus
is not going that way.

> 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.

I think we need to follow the link model more closely because that's the
consensus, and I will fix all the remaning discrepancies in between the
two models that we can find. Please continue showing them to me!

>>> 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.

Fixed in the attached version 9 of the patch.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support