I used the patch from CommitFest application and applied the following
commit to fix a known issue:

http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=commitdiff;h=d4991d35283ae0ceeb7f9e4203cf6a9dfb5d128d

Is the patch in context diff format?
Yes.

Does the patch apply cleanly?
No:
patching file src/include/commands/defrem.h
Hunk #2 succeeded at 107 with fuzz 2 (offset 27 lines).
Hunk #3 FAILED at 105.
1 out of 5 hunks FAILED -- saving rejects to file
src/include/commands/defrem.h.rej

I have used the git head of

http://git.postgresql.org/gitweb?p=postgresql-extension.git branch extensions

to do the rest of reviewing. There is one compiler warning:

gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith

-Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing
-fwrapv -g -I../../../src/interfaces/libpq -I../../../src/include -D_GNU_
SOURCE -c -o pg_dump.o pg_dump.c
pg_dump.c: In function ‘getTables’:
pg_dump.c:3748: warning: too many arguments for format


And, make check gives me the following errors:
test sanity_check ... FAILED
test rules ... FAILED

Does it include reasonable tests, necessary doc patches, etc?

There is enough documentation, but I think the documentation needs some
polishing. I am not a native English speaker, so it might be it is my
English that is failing. The data is there, but the representation might
be a little off.

I didn't spot any tests. This could be that I don't know what to look for...

Usability review:

The patch implements a way to create extensions. While the patch is
labeled extensions support for pg_dump, it actually implements more. It
implements a new way to package and install extension, and changes
contrib extensions to use the new way.

I do think we want these features, and that we do not have those
already. As far as I understand, there is nothing in the standard
regarding this feature.

I wonder if the structure of having all the extensions in share/contrib/
is a good idea. It might be better if one could separate the contrib
extensions in one place, and user created extensions in another place.
This could make it easy to see what user created extensions is installed
in (or installable to) the cluster. I think this might be helpful to
DBAs upgrading PostgreSQL.

Overall, the system seems intuitive to use. It is relatively easy to
create extension using this feature, and loading contrib extensions is
really easy. I haven't tested how easy it is to create C-language
extensions using this. If I am not mistaken it just adds the compile &
install the C-functions before installing the extension.

Feature test:

The packaging feature works as advertised, expect for the following bugs
and inconsistencies.

When using the plain CREATE EXTENSION foobar command without schema
qualifier, the extension is created in schema public (by name) without
regard to the current search path. This is inconsistent with create
table, and if the public schema is renamed, the command gives error:

ERROR: schema "public" does not exist

This makes the name "public" have a special meaning, and I suspect that
is not wanted.

When issuing CREATE EXTENSION foo SCHEMA bar, and the extension foo is
not relocatable and it's control file uses the SET search_path TO
@extschema@, the search_path is set to bar for the session. That is, it
is not changed to the original search_path after the command has completed.

When trying to load extensions which contain identical signatured
functions, if the loading will succeed is dependent on the search path.
If there is a conflicting function in search path (first extension
loaded to schema public), then the second extension load will fail. But
if the order is reversed (first extension loaded to schema foo which is
not in search path), then the second extension can be loaded to the
public schema.

While it is not possible to drop functions in extensions, it is possible
to rename a function, and also to CREATE OR REPLACE a function in an
extension. After renaming or CORing a function, it is possible to drop
the function. I also wonder if alter function ... set parameter should
be allowed?

There is no validation for the extension names in share/contrib/. It is
possible to have extensions control files named ".control",
".name.control", "name''.control" etc. While it is stupid to name them
so, it might be better to give an explicit warning / error in these
cases. It is of course possible to use these extensions.

If there is no comment for a given extension in the .control file, then
\dx will not show the extension installed even if it is installed.

I was able to make it crash:

postgres=# alter extension foo.bar set schema baz;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

Log contains this:
TRAP: FailedAssertion("!(list_length(name) == 1)", File: "extension.c",
Line: 1163)

It doesn't matter if the schema foo exists or not, and if the extension
is there or not.

I haven't done anything that could be called review on the code level,
but I have quickly glanced over the patch. Some things that caught my eye:

In file /src/backend/commands/extension.c:
+ * The extension control file format is the most simple name = value, we
+ * don't need anything more there. The SQL file to execute commands from is
+ * hardcoded to `pg_config --sharedir`/<extension>.install.sql.

This seems to be outdated.
In file src/bin/pg_dump/pg_dump.c:

! /*
! * So we want the namespaces, but we want to filter out any
! * namespace created by an extension's script. That's unless the
! * user went over his head and created objects into the extension's
! * schema: we now want the schema not to be filtered out to avoid:
! *
! * pg_dump: schema with OID 77869 does not exist
! */

I wonder what that last line is doing there...

I haven't had time to review the pg_dump part of the patch yet, will do
that next (tomorrow). I hope it is OK to post a partial review...

- Anssi

Search Discussions

  • Alvaro Herrera at Jan 17, 2011 at 3:57 pm

    Excerpts from Anssi Kääriäinen's message of lun ene 17 12:41:25 -0300 2011:

    While it is not possible to drop functions in extensions, it is possible
    to rename a function, and also to CREATE OR REPLACE a function in an
    extension. After renaming or CORing a function, it is possible to drop
    the function.
    Hmm, this seems a serious problem. I imagine that what's going on is
    that the function cannot be dropped because the extension depends on it;
    but after the rename, the dependencies of the function are dropped and
    recreated, but the dependency that relates it to the extension is
    forgotten.

    --
    Álvaro Herrera <alvherre@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
  • Dimitri Fontaine at Jan 17, 2011 at 5:15 pm

    Alvaro Herrera writes:

    Excerpts from Anssi Kääriäinen's message of lun ene 17 12:41:25 -0300 2011:
    While it is not possible to drop functions in extensions, it is possible
    to rename a function, and also to CREATE OR REPLACE a function in an
    extension. After renaming or CORing a function, it is possible to drop
    the function.
    Hmm, this seems a serious problem. I imagine that what's going on is
    that the function cannot be dropped because the extension depends on it;
    but after the rename, the dependencies of the function are dropped and
    recreated, but the dependency that relates it to the extension is
    forgotten.
    Well I'm not seeing that here:

    ~:5490=# drop function utils.lo_manage_d();
    ERROR: cannot drop function utils.lo_manage_d() because extension lo requires it
    HINT: You can drop extension lo instead.

    src/backend/commands/functioncmds.c

    /* rename */
    namestrcpy(&(procForm->proname), newname);
    simple_heap_update(rel, &tup->t_self, tup);
    CatalogUpdateIndexes(rel, tup);

    But here:

    src/backend/catalog/pg_proc.c

    /*
    * Create dependencies for the new function. If we are updating an
    * existing function, first delete any existing pg_depend entries.
    * (However, since we are not changing ownership or permissions, the
    * shared dependencies do *not* need to change, and we leave them alone.)
    */
    if (is_update)
    deleteDependencyRecordsFor(ProcedureRelationId, retval);

    [ ... adding all dependencies back ... ]

    /* dependency on extension */
    if (create_extension)
    {
    recordDependencyOn(&myself, &CreateExtensionAddress, DEPENDENCY_INTERNAL);
    }

    Will investigate some more later.
    --
    Dimitri Fontaine
    http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
  • Kääriäinen Anssi at Jan 17, 2011 at 5:59 pm
    Well I'm not seeing that here
    I am not at work at the moment and I don't have the possibility to compile PostgreSQL on this computer, so the example here is from memory.

    The issue I saw was this: assume you have an extension foo, containing one function, test().

    CREATE EXTENSION foo;
    DROP FUNCTION test();
    -- restricted due to dependency

    ALTER FUNCTION test() RENAME TO test2;
    DROP FUNCTION test2();
    -- not restricted!

    The same can be done using CREATE OR REPLACE.

    I hope this is not an error on my part. It is possible because I had a lot of schemas and my search_path might have been wrong...

    - Anssi
    PS: Using web email client, I hope this comes out in somewhat sane format.
  • Anssi Kääriäinen at Jan 18, 2011 at 8:23 am

    On 01/17/2011 07:58 PM, Kääriäinen Anssi wrote:
    The issue I saw was this: assume you have an extension foo, containing one function, test().

    CREATE EXTENSION foo;
    DROP FUNCTION test();
    -- restricted due to dependency

    ALTER FUNCTION test() RENAME TO test2;
    DROP FUNCTION test2();
    -- not restricted!

    The same can be done using CREATE OR REPLACE.

    I hope this is not an error on my part. It is possible because I had a lot of schemas and my search_path might have been wrong...
    The rename is an error on my part, sorry for that. Renaming can be done,
    but dropping is not possible even after rename. But a function in a
    package can be CREATE OR REPLACEd, and after that the function can be
    dropped. COR should be restricted in the same way as DROP is. I will
    check if this is still a problem with the latest patch.

    Another problem is that you can ALTER FUNCTION test() SET SCHEMA =
    something_else, and you can alter the functions search_path, which could
    be a problem for non-relocatable extensions. Even if the schema is
    changed, dropping extension / altering extension will work as expected.
    The function is just in different schema than the extension. But, both
    of these IMO fall in the category "don't do that".

    - Anssi
  • Dimitri Fontaine at Jan 18, 2011 at 9:42 am

    Anssi Kääriäinen writes:
    The rename is an error on my part, sorry for that. Renaming can be done, but
    dropping is not possible even after rename.
    Ok, that matches my tests and reading fo the code.
    But a function in a package can
    be CREATE OR REPLACEd, and after that the function can be dropped. COR
    should be restricted in the same way as DROP is. I will check if this is
    still a problem with the latest patch.
    There, I've been missing exactly what Alvaro has been telling us about
    here, that CREATE OR REPLACE on pg_proc will drop all known dependencies
    then recreate them, using deleteDependencyRecordsFor(). As Tom
    explained this week, DEPENDENCY_INTERNAL are working in the reverse way
    than usual, which means that the function/extension dependency were lost
    here.

    I've fixed the case by having the code remember the function's extension
    if any, and restore it along with the other dependencies. Here's my
    test case that now passes:

    ~:5490=# \dx+ lo
    Objects in extension "lo"
    Object Class | Object OID | Object Description
    --------------+------------+---------------------------------
    pg_proc | 33082 | function utils.lo_manage()
    pg_proc | 33081 | function utils.lo_oid(utils.lo)
    pg_type | 33080 | type utils.lo
    (3 rows)

    ~:5490=# CREATE OR REPLACE FUNCTION utils.lo_manage()
    RETURNS pg_catalog.trigger
    AS '$libdir/lo'
    LANGUAGE C;
    CREATE FUNCTION
    ~:5490=# \dx+ lo
    Objects in extension "lo"
    Object Class | Object OID | Object Description
    --------------+------------+---------------------------------
    pg_proc | 33082 | function utils.lo_manage()
    pg_proc | 33081 | function utils.lo_oid(utils.lo)
    pg_type | 33080 | type utils.lo
    (3 rows)

    (before the fix it would have missed the utils.lo_manage() function in
    this second listing).


    The fix is in my git repository and in the attached v26 patch,
    containing also the fixes from yesterday. Thanks again for your
    complete testing!
    Another problem is that you can ALTER FUNCTION test() SET SCHEMA =
    something_else, and you can alter the functions search_path, which could be
    a problem for non-relocatable extensions. Even if the schema is changed,
    dropping extension / altering extension will work as expected. The function
    is just in different schema than the extension. But, both of these IMO fall
    in the category "don't do that".
    Agreed. And on the other hand, I can image cases where as a work around
    you'll be glad that you still have "full power" on the extension's objects.

    Regards,
    --
    Dimitri Fontaine
    http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
  • Anssi Kääriäinen at Jan 18, 2011 at 9:56 am

    On 01/18/2011 11:42 AM, Dimitri Fontaine wrote:
    I've fixed the case by having the code remember the function's extension
    if any, and restore it along with the other dependencies.
    The only question here is should CREATE OR REPLACE be allowed. I just
    realized this could present a new problem. If I am not mistaken, when
    loading from dump, you suddenly get the extension's version back, not
    the one you defined in CREATE OR REPLACE. If this is the case, this
    should NOT be allowed. And by the same reasoning, ALTER FUNCTION
    [anything] should not be allowed either. Or at least then the
    function/(or any object for that matter) should be restored somehow from
    the backup, not from the extension files.

    I still haven't had the time to start pg_dump reviewing, so I haven't
    verified if this is really a problem. But I suspect so...

    - Anssi
  • Anssi Kääriäinen at Jan 18, 2011 at 10:34 am

    On 01/18/2011 12:11 PM, Anssi Kääriäinen wrote:
    The only question here is should CREATE OR REPLACE be allowed. I just
    realized this could present a new problem. If I am not mistaken, when
    loading from dump, you suddenly get the extension's version back, not
    the one you defined in CREATE OR REPLACE. If this is the case, this
    should NOT be allowed. And by the same reasoning, ALTER FUNCTION
    [anything] should not be allowed either. Or at least then the
    function/(or any object for that matter) should be restored somehow from
    the backup, not from the extension files.

    I still haven't had the time to start pg_dump reviewing, so I haven't
    verified if this is really a problem. But I suspect so...
    Ok, verified at least for CREATE OR REPLACE, ALTER FUNCTION RENAME and
    ALTER FUNCTION SET search_path. You will get the extensions version back
    when restoring from plain sql dump, not the CORed function, rename is
    lost and same for search_path. I suspect this is a problem for any
    object type supported in extensions. But unfortunately I do not have
    time to verify that.

    One more problem with pg_dump. If you have CREATE EXTENSION in you
    extensions .sql file, you will have problems restoring. I know it is
    stupid to do so, but still CREATE EXTENSION inside CREATE EXTENSION
    should be disallowed, as it is possible you find out too late that this
    is stupid thing to do. Also, the functions created in the "recursive"
    CREATE EXTENSION will be dumped, and the dependencies are not created
    correctly.

    Unfortunately I have used up all the time I have for reviewing this
    patch. I can arrange some more time, maybe late this week, maybe a bit
    later. So, I do not have time to do the pg_dump part review in full
    detail right now. Still, I hope the work I have done is helpful.

    Should I write up a post that contains all the current outstanding
    issues in one post, or is it enough to just link this thread in the
    CommitFest application?

    - Anssi
  • Dimitri Fontaine at Jan 18, 2011 at 11:03 am

    Anssi Kääriäinen writes:
    Ok, verified at least for CREATE OR REPLACE, ALTER FUNCTION RENAME and ALTER
    FUNCTION SET search_path. You will get the extensions version back when
    restoring from plain sql dump, not the CORed function, rename is lost and
    same for search_path. I suspect this is a problem for any object type
    supported in extensions. But unfortunately I do not have time to verify
    that.
    Yes it's the same, if you edit an extension's object directly the edited
    version is not in the dump. There's provision to have those objects in
    the dump but the function to make it so is currently restricted to only
    be called from the extension's script.

    pg_extension_flag_dump() changes the dependency type between an
    extension's and one of its object, given by oid. The default behavior
    of an extension is to skip objects in pg_dump, but in some cases
    that's not what you want to achieve, as an extension author. If your
    extension provides user data (in a table, for example), then flag this
    table so that it's part of the backups, like so:

    SELECT pg_extension_flag_dump('schema.tablename'::regclass);

    Maybe we should open this function so that it's usable even outside of
    the extension's script, but I'm not keen on doing so.

    Again, editing the extension's objects out of the scripts is still
    limited to superusers and not the only way to shoot yourself in the
    foot…
    One more problem with pg_dump. If you have CREATE EXTENSION in you
    extensions .sql file, you will have problems restoring. I know it is stupid
    to do so, but still CREATE EXTENSION inside CREATE EXTENSION should be
    disallowed, as it is possible you find out too late that this is stupid
    thing to do. Also, the functions created in the "recursive" CREATE EXTENSION
    will be dumped, and the dependencies are not created correctly.
    That will be handled later, it's called inter-extension dependencies.
    We said we won't address that yet…
    Unfortunately I have used up all the time I have for reviewing this patch. I
    can arrange some more time, maybe late this week, maybe a bit later. So, I
    do not have time to do the pg_dump part review in full detail right
    now. Still, I hope the work I have done is helpful.
    Very much so, thanks a lot for the time you already spent on it!
    Should I write up a post that contains all the current outstanding issues in
    one post, or is it enough to just link this thread in the CommitFest
    application?
    I'd appreciate a list of yet-to-fix items. What I have is the
    search_path issue where CREATE EXTENSION foo; can leave it changed for
    the current session, I intend to fix that later today.

    Other than that, I have no further already agreed on code fix to make.
    What's your list?

    Regards,
    --
    Dimitri Fontaine
    http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
  • Anssi Kääriäinen at Jan 18, 2011 at 12:14 pm

    On 01/18/2011 01:03 PM, Dimitri Fontaine wrote:
    I'd appreciate a list of yet-to-fix items. What I have is the
    search_path issue where CREATE EXTENSION foo; can leave it changed for
    the current session, I intend to fix that later today.

    Other than that, I have no further already agreed on code fix to make.
    What's your list?
    There is only documentation fixes, and I am not sure if even those are
    agreed to be necessary. It might be good if the documentation contained:

    - A warning that you need to have the files for your extensions
    readily available to be able to restore from a dump. This might be
    obvious, but better safe than sorry...
    - A warning that you will be restored to the extension's version if
    you ALTER or CREATE OR REPLACE a function.

    From the current documentation, it is maybe too easy to miss these
    risks. I am seeing this from non-experienced user's angle, and thus see
    these as potential foot guns.

    Other than that, I don't think there is anything more. I am a little
    nervous of restoring to extension's version of a function when the
    function has been CREATE OR REPLACEd, but that might be just me over
    thinking this. Also, from the previous posts, there is just the control
    file naming issue, and the issue of load order if two extensions contain
    similarly named and signatured functions. But these were agreed to be
    issues not needing any further work.

    Now, I need help what to do next. Should I leave the status as Needs
    Review as the pg_dump part is almost completely non-reviewed? And then
    attach this thread as a comment? Or as a review?

    - Anssi
  • Dimitri Fontaine at Jan 18, 2011 at 1:25 pm

    Anssi Kääriäinen writes:
    On 01/18/2011 01:03 PM, Dimitri Fontaine wrote:
    I'd appreciate a list of yet-to-fix items. What I have is the
    search_path issue where CREATE EXTENSION foo; can leave it changed for
    the current session, I intend to fix that later today.
    After some reading of backend/catalog/namespace.c and some testing, my
    take would be to have extension authors use the following form when that
    is necessary:

    SET LOCAL search_path TO public;

    Now, none of the contribs use that form (they all are relocatable except
    for adminpack which forces its objects to get installed in pg_catalog),
    so I've only updated the documentation.
    There is only documentation fixes, and I am not sure if even those are
    agreed to be necessary. It might be good if the documentation contained:

    - A warning that you need to have the files for your extensions readily
    available to be able to restore from a dump. This might be obvious, but
    better safe than sorry...
    Added a § about that in the docs.
    - A warning that you will be restored to the extension's version if you
    ALTER or CREATE OR REPLACE a function.
    Well I don't want to encourage people to manually edit any extension
    objects directly, so I've added a note about not doing that.
    Other than that, I don't think there is anything more. I am a little nervous
    of restoring to extension's version of a function when the function has been
    CREATE OR REPLACEd, but that might be just me over thinking this. Also, from
    Well with the next patch in the series, ALTER EXTENSION UPGRADE, you
    will have a way to edit extension's objects and have the new version
    installed next time, but there's no magic bullet here, it means work to
    do by the extension's author (preparing upgrade scripts and new
    version's install-from-scratch scripts).
    the previous posts, there is just the control file naming issue, and the
    issue of load order if two extensions contain similarly named and signatured
    functions. But these were agreed to be issues not needing any further work.
    Yes, extension dependencies and conflicts are not meant to be covered yet.
    Now, I need help what to do next. Should I leave the status as Needs Review
    as the pg_dump part is almost completely non-reviewed? And then attach this
    thread as a comment? Or as a review?
    My guess would be to leave it as Needs Review and add this thread as a
    review.

    Regards,
    --
    Dimitri Fontaine
    http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
  • Dimitri Fontaine at Jan 18, 2011 at 10:49 am

    Anssi Kääriäinen writes:
    The only question here is should CREATE OR REPLACE be allowed. I just
    Yes. Think ALTER EXTENSION UPGRADE, the next patch in the series
    (already proposed for this CF too).
    realized this could present a new problem. If I am not mistaken, when
    loading from dump, you suddenly get the extension's version back, not the
    one you defined in CREATE OR REPLACE. If this is the case, this should NOT
    be allowed. And by the same reasoning, ALTER FUNCTION [anything] should not
    be allowed either. Or at least then the function/(or any object for that
    matter) should be restored somehow from the backup, not from the extension
    files.
    Well ideally those will get into extension's upgrade scripts, not be
    typed interactively by superusers. But I don't think we should limit
    the capability of superusers to quickly fix a packaging mistake…
    I still haven't had the time to start pg_dump reviewing, so I haven't
    verified if this is really a problem. But I suspect so...
    Both a problem when badly used and a good thing to have sometime, as in
    the upgrade scripts :)
    --
    Dimitri Fontaine
    http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
  • Robert Haas at Jan 17, 2011 at 4:03 pm

    On Mon, Jan 17, 2011 at 10:41 AM, Anssi Kääriäinen wrote:
    I haven't had time to review the pg_dump part of the patch yet, will do that
    next (tomorrow). I hope it is OK to post a partial review...
    It is, and this is a very good and detailed review!

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Dimitri Fontaine at Jan 17, 2011 at 5:15 pm
    Hi,

    Thanks for your review!

    Anssi Kääriäinen <anssi.kaariainen@thl.fi> writes:
    Does the patch apply cleanly?
    No:
    That was some bitrot, has been fixed, thanks you for working from the
    git repository meanwhile.
    pg_dump.c:3748: warning: too many arguments for format
    Fixed in v25 already sent this morning.
    And, make check gives me the following errors:
    test sanity_check ... FAILED
    test rules ... FAILED
    Fixed too. Was due to git conflict solving where it adds a new line in
    the tests and keep the embedded rowcount the same. I think.
    Does it include reasonable tests, necessary doc patches, etc?

    There is enough documentation, but I think the documentation needs some
    polishing. I am not a native English speaker, so it might be it is my
    English that is failing. The data is there, but the representation might be
    a little off.
    We already made lots of improvements there thanks to David Wheeler
    reviews in the previous commitfest (which shows up already), and I'll be
    happy to continue improving as much as I can. If all it needs is
    english native review, I guess that'll be part of the usual marathon
    Bruce runs every year there?
    I didn't spot any tests. This could be that I don't know what to look for...
    make -C contrib installcheck will exercise CREATE EXTENSION for each
    contrib module.
    Usability review:

    The patch implements a way to create extensions. While the patch is labeled
    extensions support for pg_dump, it actually implements more. It implements a
    new way to package and install extension, and changes contrib extensions to
    use the new way.
    Well, all there's in the patch is infrastructure to be able to issue
    those single lines in your dump :)
    I do think we want these features, and that we do not have those already. As
    far as I understand, there is nothing in the standard regarding this
    feature.

    I wonder if the structure of having all the extensions in share/contrib/ is
    a good idea. It might be better if one could separate the contrib extensions
    in one place, and user created extensions in another place. This could make
    it easy to see what user created extensions is installed in (or installable
    to) the cluster. I think this might be helpful to DBAs upgrading PostgreSQL.
    It's always been this way and I though it wouldn't be in this patch
    scope to re-organise things. Also I think we should include the UPGRADE
    needs when we discuss file system level layout.
    Overall, the system seems intuitive to use. It is relatively easy to create
    extension using this feature, and loading contrib extensions is really
    easy. I haven't tested how easy it is to create C-language extensions using
    this. If I am not mistaken it just adds the compile & install the
    C-functions before installing the extension.
    It's using PGXS which existed before, all you have to do that's new is
    care about the Makefile EXTENSION variable and the control file. Even
    when doing C coded extension work.
    When using the plain CREATE EXTENSION foobar command without schema
    qualifier, the extension is created in schema public (by name) without
    regard to the current search path. This is inconsistent with create table,
    and if the public schema is renamed, the command gives error:

    ERROR: schema "public" does not exist

    This makes the name "public" have a special meaning, and I suspect that is
    not wanted.
    Fixed in git, thanks for reporting!

    ~:5490=# set client_min_messages to debug1;
    SET
    ~:5490=# set search_path to utils;
    SET
    ~:5490=# create extension unaccent;
    DEBUG: parse_extension_control_file(unaccent, '/home/dfontaine/pgsql/exts/share/contrib/unaccent.control')
    DEBUG: CreateExtensionStmt: with user data, schema = 'utils', encoding = ''
    DEBUG: Installing extension 'unaccent' from '/home/dfontaine/pgsql/exts/share/contrib/unaccent.sql', in schema utils, with user data
    CREATE EXTENSION
    ~:5490=# \dx
    List of extensions
    Schema | Name | Version | Description
    --------+---------------+----------+-------------------------------------------------
    utils | citext | 9.1devel | case-insensitive character string type
    utils | hstore | 9.1devel | storing sets of key/value pairs
    utils | int_aggregate | 9.1devel | integer aggregator and an enumerator (obsolete)
    utils | lo | 9.1devel | managing Large Objects
    utils | unaccent | 9.1devel | text search dictionary that removes accents
    (5 rows)
    When issuing CREATE EXTENSION foo SCHEMA bar, and the extension foo is not
    relocatable and it's control file uses the SET search_path TO @extschema@,
    the search_path is set to bar for the session. That is, it is not changed to
    the original search_path after the command has completed.
    It used to work this way with \i, obviously. Should the extension patch
    care about that and how? Do we want to RESET search_path or to manually
    manage it like we do for the client_min_messages and log_min_messages?
    When trying to load extensions which contain identical signatured functions,
    if the loading will succeed is dependent on the search path. If there is a
    conflicting function in search path (first extension loaded to schema
    public), then the second extension load will fail. But if the order is
    reversed (first extension loaded to schema foo which is not in search path),
    then the second extension can be loaded to the public schema.
    Well I think that's an expected limitation here. In the future we might
    want to add suport for inter-extension dependencies and conflicts, but
    we're certainly not there yet.
    While it is not possible to drop functions in extensions, it is possible to
    rename a function, and also to CREATE OR REPLACE a function in an
    extension. After renaming or CORing a function, it is possible to drop the
    function. I also wonder if alter function ... set parameter should be
    allowed?
    Well there's no specific restrictions in what you can put in an
    extension's SQL file. I see that as a feature… think upgrade scripts, too.
    There is no validation for the extension names in share/contrib/. It is
    possible to have extensions control files named ".control", ".name.control",
    "name''.control" etc. While it is stupid to name them so, it might be better
    to give an explicit warning / error in these cases. It is of course possible
    to use these extensions.
    I don't have a strong opinion here, will wait for some votes.
    If there is no comment for a given extension in the .control file, then \dx
    will not show the extension installed even if it is installed.
    Fixed in the git repository.
    I was able to make it crash:

    postgres=# alter extension foo.bar set schema baz;
    Fixed:

    ~:5490=# alter extension foo.bar set schema baz;
    ERROR: syntax error at or near "."
    LINE 1: alter extension foo.bar set schema baz;
    ^

    ~:5490=# alter extension bar set schema baz;
    ERROR: extension "bar" does not exist
    I haven't done anything that could be called review on the code level, but I
    have quickly glanced over the patch. Some things that caught my eye:

    In file /src/backend/commands/extension.c:
    + * The extension control file format is the most simple name = value, we
    + * don't need anything more there. The SQL file to execute commands from is
    + * hardcoded to `pg_config --sharedir`/<extension>.install.sql.

    This seems to be outdated.
    Yes it is. Updated, but now that is covered extensively in the
    documentation, maybe it could just be removed from the file here?
    In file src/bin/pg_dump/pg_dump.c:

    ! /*
    ! * So we want the namespaces, but we want to filter out any
    ! * namespace created by an extension's script. That's unless the
    ! * user went over his head and created objects into the extension's
    ! * schema: we now want the schema not to be filtered out to avoid:
    ! *
    ! * pg_dump: schema with OID 77869 does not exist
    ! */

    I wonder what that last line is doing there...
    That's the error message you can easily get when you want to have
    something more simple than the provided SQL…
    I haven't had time to review the pg_dump part of the patch yet, will do that
    next (tomorrow). I hope it is OK to post a partial review...
    From here, it's very good! Will you continue from the git repository,
    where the fixes are available, or do you want a v26 already?

    Regards,
    --
    Dimitri Fontaine
    http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
  • Anssi Kääriäinen at Jan 18, 2011 at 8:54 am

    On 01/17/2011 06:53 PM, Dimitri Fontaine wrote:

    Usability review:

    The patch implements a way to create extensions. While the patch is labeled
    extensions support for pg_dump, it actually implements more. It implements a
    new way to package and install extension, and changes contrib extensions to
    use the new way.
    Well, all there's in the patch is infrastructure to be able to issue
    those single lines in your dump :)
    Is this supposed to be used mainly by contrib and PGXN extensions? When
    I saw the documentation, I immediately thought that this is a nice way
    to package my application's stored procedures. If this is not one of the
    intended usages, it should be documented. I can see that this could be
    problematic when updating PostgreSQL and when recovering from backups.

    When recovering from backup, you need to have the locally created
    extension available. But it might be that the extension files are lost
    when the system went down in flames. Now, the backup is unusable
    (right?) until extension files can be recovered from source control or
    where ever they might be stored. This is why I suggested having multiple
    locations for the extensions. It would be easy to backup locally created
    extensions if those were in a single directory. All in all, I have a
    nervous feeling that somebody someday will have an unusable dump because
    they used this feature, but do not have the extension files available...

    Also, this part of documentation:

    The goal of using extensions is so that <application>pg_dump</> knows
    not to dump all the object definitions into your backups, but rather
    issue a single <xref linkend="SQL-CREATEEXTENSION"> command.

    From that, it is not entirely clear to me why this is actually wanted
    in PostgreSQL. I suppose this will make dump/restore easier to use. But
    from that paragraph, I get the feeling the only advantage is that your
    dump will be smaller. And I still have a feeling this implements more.
    Not that it is a bad thing at all.
    It used to work this way with \i, obviously. Should the extension patch
    care about that and how? Do we want to RESET search_path or to manually
    manage it like we do for the client_min_messages and log_min_messages?
    It was unintuitive to me to have search_path changed by SQL command as a
    side effect. When using \i, not so much.
    When trying to load extensions which contain identical signatured functions,
    if the loading will succeed is dependent on the search path. If there is a
    conflicting function in search path (first extension loaded to schema
    public), then the second extension load will fail. But if the order is
    reversed (first extension loaded to schema foo which is not in search path),
    then the second extension can be loaded to the public schema.
    Well I think that's an expected limitation here. In the future we might
    want to add suport for inter-extension dependencies and conflicts, but
    we're certainly not there yet.
    OK with this as is. It is just a bit surprising that you can create the
    extensions in one order but not in another.
    There is no validation for the extension names in share/contrib/. It is
    possible to have extensions control files named ".control", ".name.control",
    "name''.control" etc. While it is stupid to name them so, it might be better
    to give an explicit warning / error in these cases. It is of course possible
    to use these extensions.
    I don't have a strong opinion here, will wait for some votes.
    Yeah, this is not a big problem in practice. Just don't name them like
    that. And if you do, you will find out soon enough that you made a
    mistake. By the way, in my comment above "It is of course possible to
    use these extensions." -> "It is of course NOT possible ...".
    I haven't had time to review the pg_dump part of the patch yet, will do that
    next (tomorrow). I hope it is OK to post a partial review...
    From here, it's very good! Will you continue from the git repository,
    where the fixes are available, or do you want a v26 already?
    It is easy for me to continue from the Git repo. I will next continue
    doing the pg_dump part of the review. I hope I have time to complete
    that today.

    - Anssi
  • Dimitri Fontaine at Jan 18, 2011 at 10:02 am

    Anssi Kääriäinen writes:
    Is this supposed to be used mainly by contrib and PGXN extensions? When I
    saw the documentation, I immediately thought that this is a nice way to
    package my application's stored procedures. If this is not one of the
    intended usages, it should be documented. I can see that this could be
    problematic when updating PostgreSQL and when recovering from backups.
    Sure, private application's stored procedure are meant to be fully
    supported by the extension's facility.
    When recovering from backup, you need to have the locally created extension
    available. But it might be that the extension files are lost when the system
    went down in flames. Now, the backup is unusable (right?) until extension
    files can be recovered from source control or where ever they might be
    stored. This is why I suggested having multiple locations for the
    extensions. It would be easy to backup locally created extensions if those
    were in a single directory. All in all, I have a nervous feeling that
    somebody someday will have an unusable dump because they used this feature,
    but do not have the extension files available...
    Well, as said in the documentation, extensions are to be used for
    objects you are *not* maintaining in your database, but elsewhere.
    Typically you are maintaining your stored procedure code in some VCS,
    and you have some "packaging" (cat *.sql > my-ext.sql in the Makefile
    would be the simpler to imagine).

    So yes if you tell PostgreSQL that your procedures are managed elsewhere
    so that their code won't be part of your dumps, and then fail to manage
    them anywhere else, you're hosed.

    My viewpoint here is that when you want to use extensions, you want to
    package them for your OS of choice (mine is debian, and I've been
    working on easing things on this side too with pg_buildext to be found
    in the postgresql-server-dev-all package). If you're an occasional user
    just wanting to use new shining facilities… well, think twice…
    Also, this part of documentation:

    The goal of using extensions is so that <application>pg_dump</> knows
    not to dump all the object definitions into your backups, but rather
    issue a single <xref linkend="SQL-CREATEEXTENSION"> command.
    So maybe we want to extend this little sentence to add the warnings
    around it, that if you're not packaging your extension's delivery to
    your servers, you're likely shooting yourself in the foot?
    From that, it is not entirely clear to me why this is actually wanted in
    PostgreSQL. I suppose this will make dump/restore easier to use. But from
    that paragraph, I get the feeling the only advantage is that your dump will
    be smaller. And I still have a feeling this implements more. Not that it is
    a bad thing at all.
    Well try to upgrade from 8.4 to 9.0 with some "extensions" installed in
    there and used in your tables. Pick any contrib, such as hstore or
    ltree or cube, or some external code, such as ip4r or prefix or such.
    Then compare to upgrade with the extension facility, and tell me what's
    best :)

    Hint: the dump contains the extension's script, but does not contain the
    shared object file. If you're upgrading the OS and the contribs, as
    you often do when upgrading major versions, you're hosed. You would
    think that pg_upgrade alleviate the concerns here, but you still have
    to upgrade manually the extension's .so.

    All in all, those extensions (contribs, ip4r, etc) are *not*
    maintained in your database and pretending they are by putting their
    scripts in your dumps is only building problems. This patch aims to
    offer a solution here.
    It used to work this way with \i, obviously. Should the extension patch
    care about that and how? Do we want to RESET search_path or to manually
    manage it like we do for the client_min_messages and log_min_messages?
    It was unintuitive to me to have search_path changed by SQL command as a
    side effect. When using \i, not so much.
    Agreed. Will code the manual management way (that is already used for
    log settings) later today, unless told to see RESET and how to do that
    at the statement level rather than the transaction one.
    It is easy for me to continue from the Git repo. I will next continue doing
    the pg_dump part of the review. I hope I have time to complete that today.
    Excellent, will try to continue following your pace :)

    Regards,
    --
    Dimitri Fontaine
    http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
  • Alvaro Herrera at Jan 18, 2011 at 12:30 pm

    Excerpts from Dimitri Fontaine's message of mar ene 18 07:01:55 -0300 2011:
    Anssi Kääriäinen <anssi.kaariainen@thl.fi> writes:
    It used to work this way with \i, obviously. Should the extension patch
    care about that and how? Do we want to RESET search_path or to manually
    manage it like we do for the client_min_messages and log_min_messages?
    It was unintuitive to me to have search_path changed by SQL command as a
    side effect. When using \i, not so much.
    If the CREATE EXTENSION stuff all works in a transaction, perhaps it
    would be easier if you used SET LOCAL. At the end of the transaction it
    would reset to the original value automatically.

    --
    Álvaro Herrera <alvherre@commandprompt.com>
    The PostgreSQL Company - Command Prompt, Inc.
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedJan 17, '11 at 3:26p
activeJan 18, '11 at 1:25p
posts17
users4
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2022 Grokbase