Re: REVIEW: alter extension upgrade (patch v3)

Hi,

Anssi Kääriäinen <anssi(dot)kaariainen(at)thl(dot)fi> writes:
> There is one make check error:
> test rules ... FAILED

I forgot to update the pg_available_extensions system view in the tests,
but failed to do so in the extension's patch too, where Itagaki fixed
it. Will fix if not beaten to it, in which case the fix for this very
patch will fall down of rebasing against master…

> When building the contrib modules, make installcheck has one failure:
> test dblink ... FAILED

Apparently your setup is not allowing dblink to connect…

> There is no upgrade rule for adminpack. If I am not mistaken, this could be
> defined as
> upgrade_from_all = '.* => adminpack.sql'
> as the adminpack.sql only contains create or replace statements.

Right. Sorry about missing it. In fact the regexp .* will not match
NULL, so you have to add a specific NULL case.

> dblink has upgrade rule
> upgrade_from_null = 'null => dblink.upgrade.sql'
> however, there is not file dblink.upgrade.sql. Same for btree_gin and
> possibly others.

Those must have been missed in my git adding, I'm quite sure I have them
on another computer than the one I'm using here, will fix later. Sorry
about that.

> For example the int_aggregate.upgrade.sql uses this form:
> alter function @extschema(at)(dot)int_array_enum(integer[]) set extension
> int_aggregate;
> while btree_gist has this (no @extschema@):
> alter function gbt_text_compress(internal) set extension btree_gist;
> I am not sure which one of them is correct. I think having extschema there
> is what is wanted, and if so, this should also be mentioned in the
> documentation.

The @extschema@ is correct, sorry about that too. I've been pushing to
finish all those upgrade scripts early enough :(

Here's what says the comments in the code (which I've been working on a
moon ago, with much less pressure and tiredness):

* At upgrade time, it's highly possible that the script will need
* to ALTER already existing objects, so we only offer support for
* the placeholder @extschema(at)(dot)

So I think the code is ok but the upgrade scripts will need another
round of care.

> In general, I think the upgrade rules and associated .upgrade.sql files
> should be revisited. I did not have time to go through them all.

Will do. Thanks for the heads up.

> Might it be possible that there is a risk of attaching incompatible versions
> of objects to an extension? Say, you have contrib module foo which is loaded
> in 8.4 with \i. The module contains function bar, which is redefined in
> 9.1. When you upgrade from null, you will have the 8.4's version of function
> bar registered in the extension foo. That is, upgrade from null does not
> upgrade the objects in an extension, it just attaches them to the
> extension. As a result, when restoring from dump, you get back 9.1's
> version.
>
> What will happen if you have tables, indexes etc depending on the objects in
> the extension? Is it possible to create rules to upgrade these extensions
> easily?

Let's not mix the mechanism and how we use it. What's in the patch is a
way for the authors to provide for upgrade scripts. If 9.0 and 9.1
versions of the extension are different there's no reason for them to
provide the same upgrade from null script.

See my lo example on a previous mail to Itagaki.

> As the operator used for matching the version number is ~, the rule ' =>
> foo.upgrade.sql' will match any version. This is just as specified, but if
> you have two upgrade rules: '9.1 => foo.upgrade.sql.1' and '9.1.1 =>
> foo.upgrade.sql.2' the regexp for 9.1 will match also 9.1.1 . This could be
> quite surprising. Should the operator be such that exact match is required,
> that is ^ is inserted before the search string and $ after it? Also, should
> the ALTER EXTENSION UPGRADE give a notice: 'using rule upgrade_from_xxx'?

Yeah, anchored matches might be a better choice here. Will wait for
some more comments and do just that baring objections.

As far as the notice is concerned, though, you have the information at
the DEBUG1 level, I'm not sure about getting a notice message. That's
useful for authors debugging their control files, not for users…

> While trying to fool the upgrade engine, I noticed that if you use absolute
> paths, you get the error:
> ERROR: script path not allowed
> DETAIL: Extension's script are expected in "/usr/local/pgsql/share".
> Is this correct? Isn't the path "/usr/local/pgsql/share/contrib"
>
> Is it intentional that paths are allowed in the upgrade script name? You can
> use ../../foobar.upgrade.sql as the filename.

Itagaki raised some concerns about the path management too, so I will
follow what comes out of this in the upgrade patch too.

> All in all, so far the feature has been relatively easy and intuitive to
> use. My biggest concern is the upgrade_from_null, as specified, does not
> actually upgrade the objects, just attach them to the extension.
>
> I have no more time to test the patch. I hope this is helpful as is.

Very much so, thanks you!

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