REVIEW: alter extension upgrade (patch v3)

This review is mostly about trying to break stuff by using brute force.
I am not skilled enough in C to do a code level review.

The patch applies cleanly after applying the extensions support for
pg_dump patch. The patch is in context format.

There is one make check error:
test rules ... FAILED

The regression.diffs file is attached.

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

The regression.diffs for this is attached as contrib_regression.diffs

I don't know too well how the regression tests are supposed to be run,
so error on my part is very much possible.

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.

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.

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.

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.

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?

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

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.

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.

- Anssi