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

Search Discussions

  • Dimitri Fontaine at Feb 2, 2011 at 3:46 pm
    Hi,

    Anssi Kääriäinen <anssi.kaariainen@thl.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@.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@.

    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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedFeb 2, '11 at 3:32p
activeFeb 2, '11 at 3:46p
posts2
users2
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2022 Grokbase