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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers