Review: extension template

Hi,

I reviewed Dimitri's work on extension templates [2]. There's some
discussion still ongoing and the patch has gone through several
revisions since its addition to the current CF. The patch has already
been marked as 'returned with feedback', and I can support that
resolution (for this CF). I still see value in the following review.

Initially, I was confused about what the patch is supposed to achieve.
The 'template' naming certainly contributed to that confusion. My mental
model of a template is something that I can throw away after its use. (I
note the text search "templates" don't follow that model, either). Where
as up to now, extensions were something that I install (system wide) and
then enable per database (by CREATE, which can be thought of as a
misnomer as well).

Of course you can think of such a system wide installation as a template
for the creation (an instantiation per database). However, we didn't
ever call the system wide installations templates. Nor does the patch
start to adapt that naming scheme.

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

Dimitri responded promptly to a request to rebase the patch. Version 8
still applies cleanly to git master (as of Jul 5, 9ce9d). The patch
matches git revision c0c507022ec912854e6658c5a10a3dedb1c36d67 of dim's
github branch 'tmpl4' [2]. That's what I tested with.

The base of the patched tree builds just fine (i.e. plain 'make'),
although the compiler rightfully warns about an 'evi' variable being set
but not used. extension.c, line 1170. Jaime mentioned that already.

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’

The build passes 'make check'. Additional tests are provided. Good.

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.

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

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

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.

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?

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.

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

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.

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.

I didn't look too deep into the code, but it seems Jaime and Hitoshi
raised some valid points.

Assorted very minor nit-picks:

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.

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.

Regards

Markus Wanner

[1]: CF: Extension Templates
https://commitfest.postgresql.org/action/patch_view?id=1032

[2]: Dimitri's github branch 'tmpl4':
https://github.com/dimitri/postgres/tree/tmpl4