Re: Review: extension template

Hi,

Thanks a lot for your detailed review!

Markus Wanner <markus(at)bluegap(dot)ch> writes:
> Initially, I was confused about what the patch is supposed to achieve.
> The 'template' naming certainly contributed to that confusion. My mental

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.

> The major distinguishing factor is not the 'template' character of
> extensions "installed" that way, but the storage place of the its
> control data: filesystem vs system catalog. I'd either recommend
> appropriate renaming to reflect that fact and to avoid confusing users;
> or follow the "template" model better and decouple the extension from
> its template - with implications on extensions requiring additional
> binary code. Thinking of it, I kind of like that approach...

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

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

> As originally mentioned by Heikki, if the tplscript doesn't parse, the
> error message is just "syntax error at or near". That matches the
> behavior of extensions installed on the file system. However, given this
> adds a second way, a hint about where this error actually comes from is
> mandatory, IMO.

Will have a look at what it takes to implement support for better error
messages. May I suggest to implement that later, though? I think this is
an improvement over the current system that will be complicated to get
right and I don't want that to swamp the current patch. After all, this
patch is already in its 3rd development cycle…

> Trying to re-create a pre-existing template properly throws 'template
> for extension "$NAME" version "$VERSION" already exists'. However, if
> the extension is already enabled for that database, it instead says:
> "extension "$NAME" already exists". I can see why that's fine if you
> assume a strong binding between the "instantiation" and the "template".

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?

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

> 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, not producing a new patch version as of yet,
I think we need to set the new error messages first and I'm running
short of inspiration tonight.

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

> In any case, the error message could again be more specific:
>
> (having extension 'pex' version '0.9' on the file-system)
>
> # CREATE TEMPLATE FOR EXTENSION pex VERSION '1.0' ...
> ERROR: extension "pex" already available
>
> [ This one could mention it exists on the file-system. ]
>
> # CREATE TEMPLATE FOR EXTENISON pex FROM '1.9' TO '1.10' AS ...
> ERROR: no template for extension "pex"
>
> This last error is technically correct only if you consider file system
> extensions to not be templates. In any case, there is *something*
> relevant called "pex" on the file-system, that prevents creation of the
> template in the system catalogs. The error message should definitely be
> more specific.

Will fix early next week.

> With delight I note that renaming to an extension name that pre-exists
> on the file-system is properly prohibited. Again, the error message
> could be more specific and point to the appropriate place. However,
> that's reasonably far down the wish-list.
>
> [ Also note the nifty difference between "extension already available"
> vs "extension already exists". The former seems to mean the "template"
> exists, while the latter refers to the "instantiation". ]

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 the crux of that patch difficulties.

> However, the other way around cannot be prevented by Postgres: Files
> representing an extension (template, if you want) can always be created
> alongside a pre-existing system catalog installation. Postgres has no
> way to prevent that (other than ignoring the files, maybe, but...). It
> seems the file system variant takes precedence over anything
> pre-existing in the system catalogs. This should be documented.

Right. That will be documented in version 9 of the patch.

> The documentation for ALTER TEMPLATE FOR EXTENSION says: "Currently, the
> only supported functionality is to change the template's default
> version." I don't understand what that's supposed to mean. You can
> perfectly well rename and alter the template's code.

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

> In catalog/objectaddress.c, get_object_address_unqualified(): the case
> OBJECT_TABLESPACE is being moved down. That looks like an like an
> unintended change. Please revert.

Fixed in my github branch already, will appear in next patch version.

> 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

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

Of course, we might still need another round at it, and that's life.

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