Horton

Thread

  1. Extensions support for pg_dump, patch v27

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-01-24T09:22:23Z

    Hi,

Following recent commit of the hstore Makefile cleaning, that I included
into my extension patch too, I have a nice occasion to push version 27
of the patch.  Please find it enclosed.  This time I'm not including the
contrib-only and doc-src split versions, just ask for them if that's
what you need.

Of course you can also use my git repository:

  http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=shortlog;h=refs/heads/extension

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

  2. Re: Extensions support for pg_dump, patch v27

    Itagaki Takahiro <itagaki.takahiro@gmail.com> — 2011-01-25T13:15:58Z

    On Mon, Jan 24, 2011 at 18:22, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:
> Following recent commit of the hstore Makefile cleaning, that I included
> into my extension patch too, I have a nice occasion to push version 27
> of the patch.  Please find it enclosed.

Hi, I read the patch and test it in some degree.  It works as expected
generally, but still needs a bit more development in edge case.

* commands/extension.h needs cleanup.
- Missing "extern" declarations for create_extension and
create_extension_with_user_data variables.
- ExtensionControlFile and ExtensionList can be hidden from the header.
- extension_objects_fctx is not used at all.

* There are many recordDependencyOn(&myself, &CreateExtensionAddress, ...)
in some places, but I'm not sure we forget something -- TRIGGERs?

* Will we still need uninstaller scripts?
DROP EXTENSION depends on object dependency to drop objects,
but we have a few configurations that cannot be described in the
dependency. For example, rows inserted into pg_am for user AMs
or reverting default settings modified by the extension.

* Extensions installed in pg_catalog:
If we install an extension into pg_catalog,
  CREATE EXTENSION xxx WITH SCHEMA pg_catalog
pg_dump dumps nothing about the extension. We might need special care
for modules that install functions only in pg_catalog.

* I can install all extensions successfully, but there is a warning
in hstore. The warning was not reported to the client, but the whole
contents of hstore.sql.in was recorded in the server log.

  WARNING:  => is deprecated as an operator name

We sets client_min_messages for the issue in hstore.sql.in, but I think
we also need an additional "SET log_min_messages TO ERROR" around it.

* Is it support nested CREATE EXTENSION calls?
It's rare case, but we'd have some error checks for it.

* It passes all regression test for both backend and contrib modules,
but there are a couple of warnings during build and installation.

Three compiler warnings found. Please check.
pg_proc.c: In function 'ProcedureCreate':
pg_proc.c:110:8: warning: 'extensionOid' may be used uninitialized in
this function
pg_shdepend.c: In function 'shdepReassignOwned':
pg_shdepend.c:1335:6: warning: implicit declaration of function
'AlterOperatorOwner_oid'
operatorcmds.c:369:1: warning: no previous prototype for
'AlterOperatorOwner_oid'

* Five warnings also found during make install in contrib directory.
../../config/install-sh: ./uninstall_btree_gist.sql does not exist.
../../config/install-sh: ./uninstall_pageinspect.sql does not exist.
../../config/install-sh: ./uninstall_pgcrypto.sql does not exist.
../../config/install-sh: ./uninstall_pgrowlocks.sql does not exist.
../../config/install-sh: ./uninstall_pgstattuple.sql does not exist.

* You use "const = expr" in some places, ex. if( InvalidOid != ...),
  but we seem to prefer "expr = const".

* [off topic] I found some installer scripts are inconsistent.
They uses CREATE FUNCTION for some of functions, but CREATE OR REPLACE
FUNCTION for others. We'd better write documentation about how to write
installer scripts because CREATE EXTENSION has some implicit assumptions
in them. For example, "Don't use transaction", etc.

-- 
Itagaki Takahiro

  3. Re: Extensions support for pg_dump, patch v27

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-01-25T14:35:31Z

    Hi,

Itagaki Takahiro <itagaki.takahiro@gmail.com> writes:
> Hi, I read the patch and test it in some degree.  It works as expected
> generally, but still needs a bit more development in edge case.

Thanks for the review!

> * commands/extension.h needs cleanup.
> - Missing "extern" declarations for create_extension and
> create_extension_with_user_data variables.
> - ExtensionControlFile and ExtensionList can be hidden from the header.
> - extension_objects_fctx is not used at all.

Fixed in git.  I'm not yet used to the project style where we declare
some structures into the c code rather than the headers… and then it's
cleanup.

> * There are many recordDependencyOn(&myself, &CreateExtensionAddress, ...)
> in some places, but I'm not sure we forget something -- TRIGGERs?

I think we're good here.  The extensions I know that use triggers are
installing the functions, it's still the user who CREATE TRIGGER and
uses the function.  And even if we have an extension that contains some
CREATE TRIGGER commands in its script, the trigger will depend on a
function and the function on the extension.

If there's a use case for CREATE TRIGGER in an extension's script and
having the trigger not depend on another extension's object, like a
function, then I didn't see it.  I'm ok to add the triggers as first
class dependency objects in the extension patch if there's a need…

> * Will we still need uninstaller scripts?
> DROP EXTENSION depends on object dependency to drop objects,
> but we have a few configurations that cannot be described in the
> dependency. For example, rows inserted into pg_am for user AMs
> or reverting default settings modified by the extension.

Well I'm unconvinced by index AM extensions.  Unfortunately, if you want
a crash safe AM, you need to patch core code, it's not an extension any
more.  About reverting default settings, I'm not following.

What I saw is existing extensions that didn't need uninstall script once
you can DROP EXTENSION foo; but of course it would be quite easy to add
a parameter for that in the control file.  Do we need it, though?

> * Extensions installed in pg_catalog:
> If we install an extension into pg_catalog,
>   CREATE EXTENSION xxx WITH SCHEMA pg_catalog
> pg_dump dumps nothing about the extension. We might need special care
> for modules that install functions only in pg_catalog.

I didn't spot that, I just didn't think about that use case.  On a quick
test here it seems like the dependency is not recorded in this
case. Will fix.

> * I can install all extensions successfully, but there is a warning
> in hstore. The warning was not reported to the client, but the whole
> contents of hstore.sql.in was recorded in the server log.
>
>   WARNING:  => is deprecated as an operator name
>
> We sets client_min_messages for the issue in hstore.sql.in, but I think
> we also need an additional "SET log_min_messages TO ERROR" around it.

We can do that, but what's happening now is my understanding of the
consensus we reached on the list.

> * Is it support nested CREATE EXTENSION calls?
> It's rare case, but we'd have some error checks for it.

In fact earthdistance could CREATE EXTENSION cube; itself in its install
script.  As you say, though, it's a rare case and it was agreed that
this feature could wait until a later version, so I didn't spend time on
it.

> * It passes all regression test for both backend and contrib modules,
> but there are a couple of warnings during build and installation.
>
> Three compiler warnings found. Please check.
> pg_proc.c: In function 'ProcedureCreate':
> pg_proc.c:110:8: warning: 'extensionOid' may be used uninitialized in
> this function
> pg_shdepend.c: In function 'shdepReassignOwned':
> pg_shdepend.c:1335:6: warning: implicit declaration of function
> 'AlterOperatorOwner_oid'
> operatorcmds.c:369:1: warning: no previous prototype for
> 'AlterOperatorOwner_oid'

Oops sorry about that, I miss them too easily.  What's the trick to turn
warnings into errors already please?

Fixed in the git repository.

> * Five warnings also found during make install in contrib directory.
> ../../config/install-sh: ./uninstall_btree_gist.sql does not exist.
> ../../config/install-sh: ./uninstall_pageinspect.sql does not exist.
> ../../config/install-sh: ./uninstall_pgcrypto.sql does not exist.
> ../../config/install-sh: ./uninstall_pgrowlocks.sql does not exist.
> ../../config/install-sh: ./uninstall_pgstattuple.sql does not exist.

That's the DATA = uninstall_ lines in the Makefile.  Removed in the git
repository.  Cleared.

> * You use "const = expr" in some places, ex. if( InvalidOid != ...),
>   but we seem to prefer "expr = const".

It allows to get a compiler error when you mistakenly use = rather than
== because the Left Hand Side is a constant, so I got used to writing
things this way.  Should I review my patch and adapt?  Will do after
some votes :)

> * [off topic] I found some installer scripts are inconsistent.
> They uses CREATE FUNCTION for some of functions, but CREATE OR REPLACE
> FUNCTION for others. We'd better write documentation about how to write
> installer scripts because CREATE EXTENSION has some implicit assumptions
> in them. For example, "Don't use transaction", etc.

Will add some more documentation, ok.  As far as contrib goes, I didn't
rework the install scripts.

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

  4. Re: Extensions support for pg_dump, patch v27

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-01-25T15:23:41Z

    Itagaki Takahiro <itagaki.takahiro@gmail.com> writes:
> * Extensions installed in pg_catalog:
> If we install an extension into pg_catalog,
>   CREATE EXTENSION xxx WITH SCHEMA pg_catalog
> pg_dump dumps nothing about the extension. We might need special care
> for modules that install functions only in pg_catalog.

In src/backend/catalog/pg_depend.c we find the code for
recordDependencyOn() which is in fact in recordMultipleDependencies(),
and sayth so:

		/*
		 * If the referenced object is pinned by the system, there's no real
		 * need to record dependencies on it.  This saves lots of space in
		 * pg_depend, so it's worth the time taken to check.
		 */

So I'm not sure about what we can do here other than error on using
pg_catalog in CREATE EXTENSION?  How do we want to manage adminpack?

Other than adminpack, I think it makes sense to say that extensions are
not going into pg_catalog…

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

  5. Re: Extensions support for pg_dump, patch v27

    David Fetter <david@fetter.org> — 2011-01-25T15:27:29Z

    On Tue, Jan 25, 2011 at 04:23:41PM +0100, Dimitri Fontaine wrote:
> Itagaki Takahiro <itagaki.takahiro@gmail.com> writes:
> > * Extensions installed in pg_catalog:
> > If we install an extension into pg_catalog,
> >   CREATE EXTENSION xxx WITH SCHEMA pg_catalog
> > pg_dump dumps nothing about the extension. We might need special care
> > for modules that install functions only in pg_catalog.
> 
> In src/backend/catalog/pg_depend.c we find the code for
> recordDependencyOn() which is in fact in recordMultipleDependencies(),
> and sayth so:
> 
> 		/*
> 		 * If the referenced object is pinned by the system, there's no real
> 		 * need to record dependencies on it.  This saves lots of space in
> 		 * pg_depend, so it's worth the time taken to check.
> 		 */
> 
> So I'm not sure about what we can do here other than error on using
> pg_catalog in CREATE EXTENSION?  How do we want to manage adminpack?
> 
> Other than adminpack, I think it makes sense to say that extensions
> are not going into pg_catalog…

Given this, we should maybe see about either making adminpack part of
PostgreSQL's core distribution (probably a good idea) or moving it out
of pg_catalog so we don't have an exception to the rule.

Cheers,
David.
-- 
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

  6. Re: Extensions support for pg_dump, patch v27

    David Wheeler <david@kineticode.com> — 2011-01-25T17:57:09Z

    On Jan 25, 2011, at 7:27 AM, David Fetter wrote:

>> Other than adminpack, I think it makes sense to say that extensions
>> are not going into pg_catalog…
> 
> Given this, we should maybe see about either making adminpack part of
> PostgreSQL's core distribution (probably a good idea) or moving it out
> of pg_catalog so we don't have an exception to the rule.

+1

David

  7. Re: Extensions support for pg_dump, patch v27

    Itagaki Takahiro <itagaki.takahiro@gmail.com> — 2011-01-26T02:54:02Z

    On Wed, Jan 26, 2011 at 02:57, David E. Wheeler <david@kineticode.com> wrote:
>>> Other than adminpack, I think it makes sense to say that extensions
>>> are not going into pg_catalog…
>>
>> Given this, we should maybe see about either making adminpack part of
>> PostgreSQL's core distribution (probably a good idea) or moving it out
>> of pg_catalog so we don't have an exception to the rule.
>
> +1

I doubt it can solve the real problem.
It is my understanding that we install them in pg_catalog because
they are designed to be installed in template1 for pgAdmin, right?

We have a problem in pg_dump when we install extension modules in
template1. If we create a database, installed functions are copied
from template1. However, they are also dumped with pg_dump unless
they are in pg_catalog. So, we encounter "function already exists"
errors when pg_restore.

Since pg_dump won't dump user objects in pg_catalog, adminpack can
avoid the above errors by installing functions in pg_catalog.
CREATE EXTENSION might have the same issue -- Can EXTENSION work
without errors when we install extensions in template databases?
To avoid errors, pg_dump might need to dump extensions as
"CREATE OR REPLACE EXTENSION" or "CREATE EXTENSION IF NOT EXISTS"
rather than "CREATE EXTENSION".

-- 
Itagaki Takahiro

  8. Re: Extensions support for pg_dump, patch v27

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-01-26T10:18:45Z

    Itagaki Takahiro <itagaki.takahiro@gmail.com> writes:
> Since pg_dump won't dump user objects in pg_catalog, adminpack can
> avoid the above errors by installing functions in pg_catalog.
> CREATE EXTENSION might have the same issue -- Can EXTENSION work
> without errors when we install extensions in template databases?

Well in fact the reason why pg_dump will not dump the extension when
it's installed in pg_catalog is that the pg_depend entry is not
recorded, and the SQL query pg_dump uses gets its data from pg_extension
JOIN pg_depend JOIN pg_namespace.

The missing entry in pg_depend is the reason why the extension is not
part of the dump.  We could fix that using a LEFT JOIN here and COALESCE
to force the namespace as pg_catalog.  Is that not a kludge?  If
acceptable, I will do that next.

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

  9. Re: Extensions support for pg_dump, patch v27

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-01-26T15:31:34Z

    Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> The missing entry in pg_depend is the reason why the extension is not
> part of the dump.  We could fix that using a LEFT JOIN here and COALESCE
> to force the namespace as pg_catalog.  Is that not a kludge?

Yes, it is.  Why is the pg_depend entry missing?

			regards, tom lane

  10. Re: Extensions support for pg_dump, patch v27

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-01-26T15:39:38Z

    Tom Lane <tgl@sss.pgh.pa.us> writes:

> Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
>> The missing entry in pg_depend is the reason why the extension is not
>> part of the dump.  We could fix that using a LEFT JOIN here and COALESCE
>> to force the namespace as pg_catalog.  Is that not a kludge?
>
> Yes, it is.  Why is the pg_depend entry missing?

See src/backend/catalog/pg_depend.c

		/*
		 * If the referenced object is pinned by the system, there's no real
		 * need to record dependencies on it.  This saves lots of space in
		 * pg_depend, so it's worth the time taken to check.
		 */

Certainly, pg_catalog is pinned.

select * 
  from pg_depend
 where refobjid = (select oid
                    from pg_namespace
                   where nspname = 'pg_catalog')
  and refclassid = 'pg_namespace'::regclass;

 classid | objid | objsubid | refclassid | refobjid | refobjsubid | deptype 
---------+-------+----------+------------+----------+-------------+---------
       0 |     0 |        0 |       2615 |       11 |           0 | p
(1 row)

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

  11. Re: Extensions support for pg_dump, patch v27

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-01-26T17:17:00Z

    Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
>>> The missing entry in pg_depend is the reason why the extension is not
>>> part of the dump.  We could fix that using a LEFT JOIN here and COALESCE
>>> to force the namespace as pg_catalog.  Is that not a kludge?

>> Yes, it is.  Why is the pg_depend entry missing?

> See src/backend/catalog/pg_depend.c
> Certainly, pg_catalog is pinned.

OK, so I guess I'm missing why the extension code is looking for stuff
dependent on the pg_catalog schema.  That schema certainly doesn't
belong to any extension.

In any case, your proposed hack above is effectively assuming that
there's only one pinned schema, which is untrue now and is likely to
become even less true in the future.  So I don't think we can go that way.

			regards, tom lane

  12. Re: Extensions support for pg_dump, patch v27

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-01-26T18:47:56Z

    Tom Lane <tgl@sss.pgh.pa.us> writes:
> OK, so I guess I'm missing why the extension code is looking for stuff
> dependent on the pg_catalog schema.  That schema certainly doesn't
> belong to any extension.

Exactly.  We're on the same page here, full agreement.  So the extension
patch is not considering pg_catalog in any special way here, and the
problem comes from contrib/adminpack which installs its functions into
pg_catalog, yet is not part of core.

So in his tests, Itagaki was tempted to issue the following statement:

  CREATE EXTENSION adminpack WITH SCHEMA pg_catalog;

That's supposed to register a dependency from the extension to its
declared hosting schema (adminpack is not relocatable so that entirely
depends on its script — which forces pg_catalog).

Then you get the same problems as with any other object that lives into
this schema, pg_dump will avoid dumping its definition…

> In any case, your proposed hack above is effectively assuming that
> there's only one pinned schema, which is untrue now and is likely to
> become even less true in the future.  So I don't think we can go that way.

Well I proposed is nothing more than a crock.  What I'd like us to do
instead is ERRORing out when you want to use pg_catalog for an
extension.

We could use get_extension_namespace() just after recoding the
dependency and error out if we don't find the arguments we gave to
recordDependencyOn() so that we're not duplicating code.  That will
cover any pinned schema.  I'm preparing a patch to do that.

What do we want to do with adminpack?  Including its functions into
core, or have it use another schema?  I don't think an extension
installing its objects into pg_catalog is that good an idea…

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

  13. Re: Extensions support for pg_dump, patch v27

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-01-26T19:35:26Z

    Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> So in his tests, Itagaki was tempted to issue the following statement:

>   CREATE EXTENSION adminpack WITH SCHEMA pg_catalog;

> That's supposed to register a dependency from the extension to its
> declared hosting schema (adminpack is not relocatable so that entirely
> depends on its script - which forces pg_catalog).

> Then you get the same problems as with any other object that lives into
> this schema, pg_dump will avoid dumping its definition ...

Mph.  Although such an extension should certainly carry a dependency on
the schema it's using, I'm not sure that it makes sense to consider that
the extension (as opposed to its contained objects) belongs to the
schema.  If we think that extensions live inside schemas then it's
logically difficult for an extension to own objects scattered across
multiple schemas, which is surely a restriction we do not want.  So I'd
have expected that extensions use unqualified names, like languages or
tablespaces.  That being the case, there is no reason why pg_dump ought
to be making any such test.

There are places where pg_dump refuses to dump objects contained in
pg_catalog on the grounds that they're system objects, but that
heuristic ought not apply to extensions IMO, even if you don't agree
with the conclusion of the preceding paragraph.  Any extension is, by
definition, not built-in.

> Well I proposed is nothing more than a crock.  What I'd like us to do
> instead is ERRORing out when you want to use pg_catalog for an
> extension.

Right offhand I'm not seeing the need for such a restriction, though
certainly I'd not cry a lot if we had to impose it.  ISTM what we've got
here is some bogus what-to-dump rules in pg_dump.  Extensions should
always be dumped.

> What do we want to do with adminpack?  Including its functions into
> core, or have it use another schema?  I don't think an extension
> installing its objects into pg_catalog is that good an idea

I'm trying to avoid having an opinion on that.  The reasons for it might
or might not be very good, but I'd rather that the extension mechanism
didn't break the ability for people to make such decisions.

			regards, tom lane

  14. Re: Extensions support for pg_dump, patch v27

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-01-26T19:44:36Z

    Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> We could use get_extension_namespace() just after recoding the
> dependency and error out if we don't find the arguments we gave to
> recordDependencyOn() so that we're not duplicating code.  That will
> cover any pinned schema.  I'm preparing a patch to do that.

Kids are falling asleep and the patch there:

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

> What do we want to do with adminpack?  Including its functions into
> core, or have it use another schema?  I don't think an extension
> installing its objects into pg_catalog is that good an idea…

As we're still waiting on some decision here, and some others in
previous mails of this same thread, I'm waiting some more before to
produce the next patch in the series.  See

  http://archives.postgresql.org/pgsql-hackers/2011-01/msg02385.php
  http://archives.postgresql.org/pgsql-hackers/2011-01/msg02392.php

Of course the git repository is uptodate should you want to try the
newest code without waiting on me for the next patch release.

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

  15. Re: Extensions support for pg_dump, patch v27

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-01-26T19:51:50Z

    Tom Lane <tgl@sss.pgh.pa.us> writes:
> Mph.  Although such an extension should certainly carry a dependency on
> the schema it's using, I'm not sure that it makes sense to consider that
> the extension (as opposed to its contained objects) belongs to the
> schema.  If we think that extensions live inside schemas then it's
> logically difficult for an extension to own objects scattered across
> multiple schemas, which is surely a restriction we do not want.  So I'd
> have expected that extensions use unqualified names, like languages or
> tablespaces.  That being the case, there is no reason why pg_dump ought
> to be making any such test.

Well yes, extension are not living in a schema, we just offer users a
way to influence the script as far as where the extension's objects are
to be found and register a dependency so that it's easy to remember what
the users asked.  So that for example we are able to act the same way on
pg_restore.

Now, pg_dump makes no such test already, but a query is using a JOIN to
list extensions and their target schema, where it's possible that the
target has not been recorded by recordDependencyOn(): in this case the
whole extension's is filtered out of the resultset.  Quick and dirty
fix, I proposed to LEFT JOIN in the pg_dump query…

> There are places where pg_dump refuses to dump objects contained in
> pg_catalog on the grounds that they're system objects, but that
> heuristic ought not apply to extensions IMO, even if you don't agree
> with the conclusion of the preceding paragraph.  Any extension is, by
> definition, not built-in.

Agreed.  The problem we're having here is how to implement all that yet
fully support adminpack.  The design we're talking about is the same.

>> Well I proposed is nothing more than a crock.  What I'd like us to do
>> instead is ERRORing out when you want to use pg_catalog for an
>> extension.
>
> Right offhand I'm not seeing the need for such a restriction, though
> certainly I'd not cry a lot if we had to impose it.  ISTM what we've got
> here is some bogus what-to-dump rules in pg_dump.  Extensions should
> always be dumped.

Agreed.  Trying to solve an implementation detail, and that's the easier
way to prevent users from shooting themselves in the foot here.

>> What do we want to do with adminpack?  Including its functions into
>> core, or have it use another schema?  I don't think an extension
>> installing its objects into pg_catalog is that good an idea
>
> I'm trying to avoid having an opinion on that.  The reasons for it might
> or might not be very good, but I'd rather that the extension mechanism
> didn't break the ability for people to make such decisions.

You still can do one of the following commands, where you're not having
a say on the real schema that adminpack will use because it's not
relocatable and not paying attention to @extschema@, but apart from this
lie everything will just work.  I'd be happy to ship with such a lie,
but I can see why people want something different.

  CREATE EXTENSION adminpack;
  CREATE EXTENSION adminpack WITH SCHEMA utils;

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

  16. Re: Extensions support for pg_dump, patch v27

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-01-26T20:38:48Z

    Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> Mph.  Although such an extension should certainly carry a dependency on
>> the schema it's using, I'm not sure that it makes sense to consider that
>> the extension (as opposed to its contained objects) belongs to the
>> schema.

> Well yes, extension are not living in a schema, we just offer users a
> way to influence the script as far as where the extension's objects are
> to be found and register a dependency so that it's easy to remember what
> the users asked.  So that for example we are able to act the same way on
> pg_restore.

Oh: then you're doing it wrong.  If you want to remember that WITH
SCHEMA was specified, you need to explicitly store that as another
column in pg_extension.  You should not be depending on the dependency
mechanism to remember that for you, any more than we'd use pg_depend to
remember a table's relnamespace.  The dependency mechanism is there
to figure out the consequences of a DROP command, it's not there to
remember arbitrary facts.  (And yes, I know that we've cheated on that
principle a few times before; but you can't do it here.)

			regards, tom lane

  17. Re: Extensions support for pg_dump, patch v27

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-01-26T21:03:07Z

    Tom Lane <tgl@sss.pgh.pa.us> writes:
> Oh: then you're doing it wrong.  If you want to remember that WITH
> SCHEMA was specified, you need to explicitly store that as another
> column in pg_extension.  You should not be depending on the dependency
> mechanism to remember that for you, any more than we'd use pg_depend to
> remember a table's relnamespace.  The dependency mechanism is there
> to figure out the consequences of a DROP command, it's not there to
> remember arbitrary facts.  (And yes, I know that we've cheated on that
> principle a few times before; but you can't do it here.)

The thinking is that we need to have the dependency registered too, so
that DROP SCHEMA will cascade to the extension.  So while at it, I also
used the dependency for tracking the schema.

Even if I get to use a column to track the schema, I will have to
maintain registering the dependency.  Should I do that?

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

  18. Re: Extensions support for pg_dump, patch v27

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-01-27T11:01:46Z

    Tom Lane <tgl@sss.pgh.pa.us> writes:
> Oh: then you're doing it wrong.  If you want to remember that WITH
> SCHEMA was specified, you need to explicitly store that as another
> column in pg_extension.

Ok, done.  Of course, it solves the whole problem Itagaki had with
adminpack because we stop relying on dependencies to get it right now.

I've also added another parameter in the control file, named "schema".
It's only valid to use that when relocatable is false, and it allows to
force the schema where to install the extension.  When this schema does
not already exists, it will be created for the user.

Of course the adminpack extension's control file now has relocatable =
false and schema = 'pg_catalog'.

~=# create extension lo;
CREATE EXTENSION
~=# create extension adminpack;
CREATE EXTENSION
~=# \dx
                             List of extensions
   Schema   |   Name    | Version  |               Description               
------------+-----------+----------+-----------------------------------------
 pg_catalog | adminpack | 9.1devel | Administrative functions for PostgreSQL
 utils      | lo        | 9.1devel | managing Large Objects
(2 rows)

~=# drop extension adminpack;
DROP EXTENSION
~=# create extension adminpack with schema utils;
ERROR:  this extension has to be installed in schema "pg_catalog"
~=# create extension adminpack with schema pg_catalog;
CREATE EXTENSION
~=# alter extension adminpack set schema utils;
ERROR:  this extension does not support SET SCHEMA

The documentation is updated both in the patch and here:

  http://pgsql.tapoueh.org/extensions/doc/html/extend-extension.html

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

  19. Re: Extensions support for pg_dump, patch v27

    Itagaki Takahiro <itagaki.takahiro@gmail.com> — 2011-01-27T12:02:15Z

    On Thu, Jan 27, 2011 at 20:01, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:
> Ok, done.  Of course, it solves the whole problem Itagaki had with
> adminpack because we stop relying on dependencies to get it right now.

I found pg_restore with -c option fails when an extension is created
in pg_catalog. Since pg_catalog is an implicit search target, so we
might need the catalog to search_path explicitly.
Note that I can restore adminpack with not errors because it has
explicit "pg_catalog." prefix in the installer script.

----
postgres=# CREATE EXTENSION cube WITH SCHEMA pg_catalog;
$ pg_dump -Fc postgres > postgres.dump
$ pg_restore -d postgres -c postgres.dump
pg_restore: [archiver (db)] Error while PROCESSING TOC:
pg_restore: [archiver (db)] Error from TOC entry 1; 3996 16678 EXTENSION cube
pg_restore: [archiver (db)] could not execute query: ERROR:  no schema
has been selected to create in
----

BTW, I have a minor comments for the code.
  extern bool extension_relocatable_p(Oid ext_oid);
What is "_p" ?  Also, we could make the function static
because it is used only in extension.c.

-- 
Itagaki Takahiro

  20. Re: Extensions support for pg_dump, patch v27

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-01-27T13:48:08Z

    Itagaki Takahiro <itagaki.takahiro@gmail.com> writes:
> I found pg_restore with -c option fails when an extension is created
> in pg_catalog. Since pg_catalog is an implicit search target, so we
> might need the catalog to search_path explicitly.
> Note that I can restore adminpack with not errors because it has
> explicit "pg_catalog." prefix in the installer script.

Nice catch, thank you very much (again) for finding those :)

Please find inline the patch I've just applied that fixes the issue by
managing the current search path and creation namespace before executing
the script, the same way that schemacmds.c does.

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

> BTW, I have a minor comments for the code.
>   extern bool extension_relocatable_p(Oid ext_oid);
> What is "_p" ?  Also, we could make the function static
> because it is used only in extension.c.

predicate.  Maybe I've done too much Emacs Lisp coding at the time I
added that function, but it looked natural (enough) to me :)

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


commit 5c14c989426c0811e5bf8b993ae9a966fbf903c4 (HEAD, refs/remotes/origin/extension, refs/heads/extension)
Author: Dimitri Fontaine <dim@tapoueh.org>
Date:   Thu Jan 27 14:40:10 2011 +0100

    Fully set the creation namespace before to execute the extenions's script.

	Modified src/backend/commands/extension.c
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 87310fb..4a8c757 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -415,6 +415,8 @@ execute_extension_script(ExtensionControlFile *control,
 	char       *filename = get_extension_absolute_path(control->script);
 	char       *old_cmsgs = NULL, *old_lmsgs = NULL; /* silence compiler */
 	bool        reset_cmsgs = false, reset_lmsgs = false;
+	Oid         target_schema;
+	OverrideSearchPath *overridePath;
 
 	/*
 	 * Set create_extension and create_extension_with_user_data so that the
@@ -453,6 +455,21 @@ execute_extension_script(ExtensionControlFile *control,
 		SetConfigOption("log_min_messages", "warning", PGC_SUSET, PGC_S_SESSION);
 	}
 
+	if (control->relocatable)
+		target_schema = LookupCreationNamespace(schema);
+	else
+		target_schema = get_namespace_oid(schema, false);
+
+	/*
+	 * Temporarily make the new namespace be the front of the search path, as
+	 * well as the default creation target namespace.  This will be undone at
+	 * the end of this routine, or upon error.
+	 */
+	overridePath = GetOverrideSearchPath(CurrentMemoryContext);
+	overridePath->schemas = lcons_oid(target_schema, overridePath->schemas);
+	/* XXX should we clear overridePath->useTemp? */
+	PushOverrideSearchPath(overridePath);
+
 	/*
 	 * On failure, ensure that create_extension does not remain set.
 	 */
@@ -474,7 +491,8 @@ execute_extension_script(ExtensionControlFile *control,
 		if (control->relocatable)
 		{
 			List *search_path = fetch_search_path(false);
-			Oid   first_schema, target_schema;
+			Oid first_schema = linitial_oid(search_path);
+			list_free(search_path);
 
 			if (!execute_sql_string(sql))
 				ereport(ERROR,
@@ -495,10 +513,6 @@ execute_extension_script(ExtensionControlFile *control,
 			 * We skip this step if the target schema happens to be the
 			 * first schema of the current search_path.
 			 */
-			first_schema = linitial_oid(search_path);
-			target_schema = LookupCreationNamespace(schema);
-			list_free(search_path);
-
 			if (first_schema != target_schema)
 				AlterExtensionNamespace_oid(CreateExtensionAddress.objectId,
 											target_schema);
@@ -531,6 +545,9 @@ execute_extension_script(ExtensionControlFile *control,
 	}
 	PG_END_TRY();
 
+	/* Reset search path to normal state */
+	PopOverrideSearchPath();
+
 	if (reset_cmsgs)
 		SetConfigOption("client_min_messages",
 						old_cmsgs, PGC_SUSET, PGC_S_SESSION);

  21. Re: Extensions support for pg_dump, patch v27

    Itagaki Takahiro <itagaki.takahiro@gmail.com> — 2011-01-28T04:24:19Z

    On Thu, Jan 27, 2011 at 22:48, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:
> Itagaki Takahiro <itagaki.takahiro@gmail.com> writes:
>> I found pg_restore with -c option fails when an extension is created
>> in pg_catalog.
> Nice catch, thank you very much (again) for finding those :)

Seems good.

>>   extern bool extension_relocatable_p(Oid ext_oid);
> predicate.  Maybe I've done too much Emacs Lisp coding at the time I
> added that function, but it looked natural (enough) to me :)

Hmmm, I like extension_is_relocatable() or so...
Including the above, I wrote a patch on your patch for minor
cleanup. Please merge reasonable parts in it.

* access() is not portable.
The pre-checking with access() doesn't seems needed because
the same error will be raised in parse_extension_control_file().

* There are some dead code in the patch.
For example, you exported ObjectAddresses to public, but it is not
used in extension.c actually. I reverted some of changes.

* Should we support absolute control file paths?
Absolute paths are supported by get_extension_absolute_path(),
but I'm not sure actual use-cases.

* Each ereport(ERROR) should have a reasonable errcode unless
they are an internal logic error, and whether the error message
follows our guidline (starting with a lower case character, etc.)

* Changed create_extension_with_user_data to a static variable.
CreateExtensionAddress and create_extension are still exported.
We could have better names for them -- CurrentExtensionAddress
and in_create_extension?  Or, in_create_extension might be
replaced with "CreateExtensionAddress.objectId != InvalidOid".

* Added psql tab completion for CREATE/DROP/ALTER EXTENSION.
* Use palloc0() instead of palloc() and memset(0).
* Several code cleanup.

-- 
Itagaki Takahiro

  22. Re: Extensions support for pg_dump, patch v27

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-01-28T09:03:32Z

    Itagaki Takahiro <itagaki.takahiro@gmail.com> writes:
> Hmmm, I like extension_is_relocatable() or so...
> Including the above, I wrote a patch on your patch for minor
> cleanup. Please merge reasonable parts in it.

After review, I included all your proposed changes, thanks a lot!
Please find attached a new version of the patch, v29, including your
changes and merged against HEAD from this morning (there was no
conflict, but still).

> * access() is not portable.
> The pre-checking with access() doesn't seems needed because
> the same error will be raised in parse_extension_control_file().
>
> * There are some dead code in the patch.
> For example, you exported ObjectAddresses to public, but it is not
> used in extension.c actually. I reverted some of changes.

Thanks, those are due to late refactoring, removal of features and
adjustments etc.  Doing that in "free time" rather than full time these
busy days, so I've missed some cleaning.

> * Should we support absolute control file paths?
> Absolute paths are supported by get_extension_absolute_path(),
> but I'm not sure actual use-cases.

Well I don't see no harm in allowing non-core compliant extension
packaging, as this feature is not targeting only BSD compatible Open
Source extensions such as contribs.  It seems to me to be a case of
mechanism versus policy, I don't think our policy here should mean that
we alter the mechanism for others.

> * Each ereport(ERROR) should have a reasonable errcode unless
> they are an internal logic error, and whether the error message
> follows our guidline (starting with a lower case character, etc.)

Done.

> * Changed create_extension_with_user_data to a static variable.
> CreateExtensionAddress and create_extension are still exported.
> We could have better names for them -- CurrentExtensionAddress
> and in_create_extension?  Or, in_create_extension might be
> replaced with "CreateExtensionAddress.objectId != InvalidOid".

Well there has been enough discussion about those names I think, current
one where voted by Alvaro and Robert IIRC.  I'm open to changes, but
would now prefer to include that in the commiter's work :)

> * Added psql tab completion for CREATE/DROP/ALTER EXTENSION.
> * Use palloc0() instead of palloc() and memset(0).
> * Several code cleanup.

Thanks a lot!

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

  23. Re: Extensions support for pg_dump, patch v27

    Itagaki Takahiro <itagaki.takahiro@gmail.com> — 2011-01-31T03:42:47Z

    On Fri, Jan 28, 2011 at 18:03, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:
> After review, I included all your proposed changes, thanks a lot!
> Please find attached a new version of the patch, v29,

Additional questions and discussions:

* "relocatable" and "schema" seems to be duplicated options.
We could treat an extension is relocatable when schema is not
specified instead of relocatable option. Or, If we keep "schema"
option, it would be used as the default schema to be installed
when WITH SCHEMA is not specified.

* "version" field in pg_available_extension might mislead when
a newer version of an extension is available but an older version
is installed. How about returning installed version for "installed"
field instead of booleans? The field will be NULLs if not installed.

* I want to remove O(n^2) behavior in pg_extensions(). It scans
pg_extension catalog to return whether the extension is installed,
but it would be better to change the function to return just whole
extensions and JOIN with pg_extension in pg_available_extensions.
(it's the same technique used in pg_stat_replication)

-- 
Itagaki Takahiro

  24. Re: Extensions support for pg_dump, patch v27

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-01-31T10:21:09Z

    Itagaki Takahiro <itagaki.takahiro@gmail.com> writes:
> * "relocatable" and "schema" seems to be duplicated options.

They are not, really.  If you have a relocatable extension, then there's
no schema option in the control file (setting it is an ERROR).  If you
have a non-relocatable extension, then you can either setup the schema
to force it as the extension's author (or distributor / packager), or
leave it alone and use the @extschema@ facility instead.

> * "version" field in pg_available_extension might mislead when
> a newer version of an extension is available but an older version
> is installed. How about returning installed version for "installed"
> field instead of booleans? The field will be NULLs if not installed.

Good idea, I've done that in the pg_available_extension system view.

> * I want to remove O(n^2) behavior in pg_extensions(). It scans
> pg_extension catalog to return whether the extension is installed,
> but it would be better to change the function to return just whole
> extensions and JOIN with pg_extension in pg_available_extensions.
> (it's the same technique used in pg_stat_replication)

Well, that allows to get rid of the whole extension's listing internal
function.  Less code is good :)  Here's the new system's view:

  http://pgsql.tapoueh.org/extensions/doc/html/view-pg-available-extensions.html

  CREATE VIEW pg_available_extensions AS
      SELECT n.nspname as "schema", E.name, X.extversion as "installed",
             E.version, E.relocatable, E.comment
        FROM pg_available_extensions() AS E
             LEFT JOIN pg_extension as X ON E.name = X.extname
             LEFT JOIN pg_namespace as N on N.oid = X.extnamespace;
  
The new code (and documentation) is published in the git repository, I'm
waiting a little bit more before (for your comments) to prepare the
patch v30, or just tell me and I'll do that.

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

  25. Re: Extensions support for pg_dump, patch v27

    Itagaki Takahiro <itagaki.takahiro@gmail.com> — 2011-02-01T10:55:57Z

    Hi, the attached is a further cleanup of the latest commit
(1db20cdd36cb1c2cc5ef2210a23b3c09f5058690).

* Accept paths under $PGSHARE during CREATE EXTENSION
"in addition to" normal paths in convert_and_check_filename().
I think we don't have to disallow normal file accesses in the case.
* Rewrite pg_available_extensions() to use materialize mode.
* Add a protection for nested CREATE EXTENSION calls.
* Removed some unneeded changes in the patch:
  - utils/genfile.h (use DIR directly)
  - missing merges from master in guc.c
  - only #include changes in a few files

Comments and documentation would need to be checked native
English speakers. I cannot help you In this area, sorry.

There are last two issues before it goes to ready for committer.

On Mon, Jan 31, 2011 at 19:21, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:
>> * "relocatable" and "schema" seems to be duplicated options.
> They are not, really.

I'd suggest to remove "relocatable" option if you won't add
additional meanings to the "relocatable but having schema" case.

Or, I'm still not sure why only adminpack is not relocatable.
Is it possible to make all extensions to relocatable and add
"default_schema = pg_catalog" to adminpack for backward-compatibility?

From older mails:
>> * Should we support absolute control file paths?
> Well I don't see no harm in allowing non-core compliant extension
> packaging,

If you want to support absolute paths, you also need to adjust
convert_and_check_filename() because it only allows to read files
in $PGSHARE. So, absolute path support doesn't work actually.
I prefer to remove absolute path support from script option
because absolute paths are just unportable.

-- 
Itagaki Takahiro

  26. Re: Extensions support for pg_dump, patch v27

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-02-01T14:12:12Z

    Itagaki Takahiro <itagaki.takahiro@gmail.com> writes:
> Hi, the attached is a further cleanup of the latest commit
> (1db20cdd36cb1c2cc5ef2210a23b3c09f5058690).

Thanks!  Given that the patch contains some merging from master's
branch, I'm not sure if I should apply it to my repository then handle
conflicts, or let you manage the patch now?

> * Accept paths under $PGSHARE during CREATE EXTENSION
> "in addition to" normal paths in convert_and_check_filename().
> I think we don't have to disallow normal file accesses in the case.
> * Rewrite pg_available_extensions() to use materialize mode.
> * Add a protection for nested CREATE EXTENSION calls.
> * Removed some unneeded changes in the patch:
>   - utils/genfile.h (use DIR directly)
>   - missing merges from master in guc.c
>   - only #include changes in a few files
>
> Comments and documentation would need to be checked native
> English speakers. I cannot help you In this area, sorry.

Thanks.  I don't see the PATH modifications when reading the patch, though.

> There are last two issues before it goes to ready for committer.
>
> On Mon, Jan 31, 2011 at 19:21, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:
>>> * "relocatable" and "schema" seems to be duplicated options.
>> They are not, really.
>
> I'd suggest to remove "relocatable" option if you won't add
> additional meanings to the "relocatable but having schema" case.

The schema option only makes sense when the extension is *NOT*
relocatable.

> Or, I'm still not sure why only adminpack is not relocatable.
> Is it possible to make all extensions to relocatable and add
> "default_schema = pg_catalog" to adminpack for backward-compatibility?

The adminpack SQL script is hard-coding pg_catalog, so user won't be
able to choose where to install it.  Technically, they could still move
the functions to another schema, but that would break pgAdmin AFAIUI, so
the extension's author here *wants* to forbid the user to relocate the
extension.  And want to prevent to user from installing it where he
wants in the first place.


The option relocatable is allowing ALTER EXTENSION … SET SCHEMA, when
the control files also specify the schema, then you can't choose where
to install the extension in the first place.

I don't think we can go to only 1 options here.

> From older mails:
>>> * Should we support absolute control file paths?
>> Well I don't see no harm in allowing non-core compliant extension
>> packaging,
>
> If you want to support absolute paths, you also need to adjust
> convert_and_check_filename() because it only allows to read files
> in $PGSHARE. So, absolute path support doesn't work actually.
> I prefer to remove absolute path support from script option
> because absolute paths are just unportable.

I have no strong opinion here, ok for me.

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

  27. Re: Extensions support for pg_dump, patch v27

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-02-01T17:19:29Z

    Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> Itagaki Takahiro <itagaki.takahiro@gmail.com> writes:
>> Hi, the attached is a further cleanup of the latest commit
>> (1db20cdd36cb1c2cc5ef2210a23b3c09f5058690).

> Thanks!  Given that the patch contains some merging from master's
> branch, I'm not sure if I should apply it to my repository then handle
> conflicts, or let you manage the patch now?

Actually, I was about to pick up and start working on the whole
extensions patch series, but now I'm confused as to what and where is
the latest version.

			regards, tom lane

  28. Re: Extensions support for pg_dump, patch v27

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-02-01T17:34:06Z

    Tom Lane <tgl@sss.pgh.pa.us> writes:
> Actually, I was about to pick up and start working on the whole
> extensions patch series, but now I'm confused as to what and where is
> the latest version.

Ok, here's what I have, attached, as patch v30.  What Itagaki just sent
applies on top of that.

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

  29. Re: Extensions support for pg_dump, patch v27

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-02-04T18:45:51Z

    Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> Actually, I was about to pick up and start working on the whole
>> extensions patch series, but now I'm confused as to what and where is
>> the latest version.

> Ok, here's what I have, attached, as patch v30.  What Itagaki just sent
> applies on top of that.

What are the guc.c changes in this patch?  They appear completely
unrelated to the topic.

			regards, tom lane

  30. Re: Extensions support for pg_dump, patch v27

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-02-04T19:38:28Z

    Tom Lane <tgl@sss.pgh.pa.us> writes:
> What are the guc.c changes in this patch?  They appear completely
> unrelated to the topic.

Right.  Didn't spot them.  Sorry for the noise in the patch, it must be
a merge problem in my repository.  Do you want me to clean that up or is
it already to late?

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

  31. Re: Extensions support for pg_dump, patch v27

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-02-04T19:48:44Z

    Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> What are the guc.c changes in this patch?  They appear completely
>> unrelated to the topic.

> Right.  Didn't spot them.  Sorry for the noise in the patch, it must be
> a merge problem in my repository.  Do you want me to clean that up or is
> it already to late?

No need, I'll just drop that file from the patch.

			regards, tom lane

  32. Re: Extensions support for pg_dump, patch v27

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-02-04T20:34:47Z

    While I'm looking at this ... what is the rationale for treating rewrite
rules as members of extensions, ie, why does the patch touch
rewriteDefine.c?  ISTM a rule is a property of a table and could not
sensibly be an independent member of an extension.  If there is a use
for that, why are table constraints and triggers not given the same
treatment?

			regards, tom lane

  33. Re: Extensions support for pg_dump, patch v27

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-02-04T20:55:41Z

    Tom Lane <tgl@sss.pgh.pa.us> writes:
> While I'm looking at this ... what is the rationale for treating rewrite
> rules as members of extensions, ie, why does the patch touch
> rewriteDefine.c?  ISTM a rule is a property of a table and could not
> sensibly be an independent member of an extension.  If there is a use
> for that, why are table constraints and triggers not given the same
> treatment?

I remember thinking I needed to do that for CREATE VIEW support while
discovering PostgreSQL internals.

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

  34. Re: Extensions support for pg_dump, patch v27

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-02-08T03:30:40Z

    Attached is an updated patch that incorporates all of the review I've
done so far on the core code.  This omits the contrib changes, which
I've not looked at in any detail, and the docs changes since I've not
yet updated the docs to match today's code changes.  User-visible
changes are that WITH NO DATA is gone, and instead there's
pg_extension_config_dump(regclass, text) as per today's discussion.
The latter is only lightly tested but seems to work as intended.

I believe the core code is now in pretty good shape; the only open issue
I have at the moment is that pg_dump will fail to suppress dumping of
USER MAPPING objects that are in an extension.  Which is something I'm
not too excited about anyway.  (The reason that's an issue is that I
reverted most of the changes in pg_dump.c in favor of using pg_dump's
already existing dependency mechanisms to suppress dumping unwanted
items.  The USER MAPPING code tries to bypass the DumpableObject
representation, which means it doesn't get filtered.)

The documentation still needs a good bit of work, but I hope to have
this committed within a day or so.

			regards, tom lane

  35. Re: Extensions support for pg_dump, patch v27

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-02-08T10:51:11Z

    Tom Lane <tgl@sss.pgh.pa.us> writes:

> Attached is an updated patch that incorporates all of the review I've

And that looks great, thanks.  I've only had time to read the patch,
will play with it later on today, hopefully.


I've spotted a comment that I think you missed updating.  The schema
given in the control file is now created in all cases rather than only
when the extension is not relocatable, right?

+ 	else if (control->schema != NULL)
+ 	{
+ 		/*
+ 		 * The extension is not relocatable and the author gave us a schema
+ 		 * for it.  We create the schema here if it does not already exist.
+ 		 */

I also note that the attached version doesn't contain \dX, is that an
happenstance or a choice you did here?

> done so far on the core code.  This omits the contrib changes, which
> I've not looked at in any detail, and the docs changes since I've not
> yet updated the docs to match today's code changes.  User-visible
> changes are that WITH NO DATA is gone, and instead there's
> pg_extension_config_dump(regclass, text) as per today's discussion.
> The latter is only lightly tested but seems to work as intended.

I think I will have to test it and get my head around it, but as we
said, it's a good change (simpler, only target user tables).

> I believe the core code is now in pretty good shape; the only open issue
> I have at the moment is that pg_dump will fail to suppress dumping of
> USER MAPPING objects that are in an extension.  Which is something I'm
> not too excited about anyway.  (The reason that's an issue is that I
> reverted most of the changes in pg_dump.c in favor of using pg_dump's
> already existing dependency mechanisms to suppress dumping unwanted
> items.  The USER MAPPING code tries to bypass the DumpableObject
> representation, which means it doesn't get filtered.)

Well the pg_dump support code is very different than the one I did, so I
will have to learn about the dependency management there before I can
comment.

> The documentation still needs a good bit of work, but I hope to have
> this committed within a day or so.

Great!  Again, thanks a lot, I like the way you simplified and cleaned
the patch, and I still recognise the code :)

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

  36. Re: Extensions support for pg_dump, patch v27

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-02-08T15:21:03Z

    Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> I've spotted a comment that I think you missed updating.  The schema
> given in the control file is now created in all cases rather than only
> when the extension is not relocatable, right?

Hm, no, that logic is the same as before no?

> I also note that the attached version doesn't contain \dX, is that an
> happenstance or a choice you did here?

I removed it --- it wasn't documented and I didn't see much point anyway
in a \d command that just duplicates a system view, especially a view
only usable by superusers.

			regards, tom lane

  37. Re: Extensions support for pg_dump, patch v27

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-02-08T16:36:40Z

    Tom Lane <tgl@sss.pgh.pa.us> writes:

> Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
>> I've spotted a comment that I think you missed updating.  The schema
>> given in the control file is now created in all cases rather than only
>> when the extension is not relocatable, right?
>
> Hm, no, that logic is the same as before no?

Well I had

		if (!control->relocatable && control->schema != NULL)

And you have 

+ 	else if (control->schema != NULL)

So you're considering the schema option independently of the relocatable
option, which is ok because you changed those options defaults and
conflicts in the control file parsing.  Only the comment needs adjusting.

> I removed it --- it wasn't documented and I didn't see much point anyway
> in a \d command that just duplicates a system view, especially a view
> only usable by superusers.

It used to not be a straight select from system view, and I wanted to
offer something as simple as clicking a check box in pgadmin for people
who want to see what's available and install it.  But well, the system
view is good enough now I guess, we're talking about DBA here after all.

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

  38. Re: Extensions support for pg_dump, patch v27

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-02-08T17:10:24Z

    Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> Hm, no, that logic is the same as before no?

> Well I had

> 		if (!control->relocatable && control->schema != NULL)

> And you have 

> + 	else if (control->schema != NULL)

Yeah, I deleted that relocatable test because it's redundant:
control->schema cannot be set for a relocatable extension,
cf the test in read_extension_control_file.

			regards, tom lane

  39. Re: Extensions support for pg_dump, patch v27

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-02-08T20:35:06Z

    Tom Lane <tgl@sss.pgh.pa.us> writes:
> Yeah, I deleted that relocatable test because it's redundant:
> control->schema cannot be set for a relocatable extension,
> cf the test in read_extension_control_file.

I missed that you kept this test in your version of the patch.  Sorry
for noise.

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

  40. Re: Extensions support for pg_dump, patch v27

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-02-08T21:46:12Z

    I have gone ahead and committed the core and documentation parts of this
patch, but not as yet any of the contrib changes; that is, all the
contrib modules are still building old-style.  I had intended to do it
in two steps all along, so as to get some buildfarm proof for the thesis
that we haven't broken old-style add-on modules.  However, there is now
an additional motivation for delay: so long as we haven't pulled the
trigger on changing the contrib modules, this is an experimental feature
that nothing else depends on.  Worst case, if we can't get to a
satisfactory resolution of the pg_upgrade and ALTER EXTENSION UPGRADE
issues, we can ship 9.1 as-is and just label the EXTENSION commands as
subject to change.  I will however now go work on those issues.

			regards, tom lane

  41. Re: Extensions support for pg_dump, patch v27

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-02-08T22:03:14Z

    Tom Lane <tgl@sss.pgh.pa.us> writes:
> I have gone ahead and committed the core and documentation parts of this

Thank you!

And I'd like to take the time to thank all of you who helped me reach
this goal, but that ranges to about everyone here who I talked to at any
conference those last two or three years (I still remember talking about
that at PGDay 2008 in Prato, but the ball was already rolling if only in
my head).

You might also be interested to know that the research leading to these
results has received funding from the European Union's Seventh Framework
Programme (FP7/2007-2013) under grant agreement 258862.

> patch, but not as yet any of the contrib changes; that is, all the
> contrib modules are still building old-style.  I had intended to do it
> in two steps all along, so as to get some buildfarm proof for the thesis
> that we haven't broken old-style add-on modules.  However, there is now
> an additional motivation for delay: so long as we haven't pulled the
> trigger on changing the contrib modules, this is an experimental feature
> that nothing else depends on.  Worst case, if we can't get to a
> satisfactory resolution of the pg_upgrade and ALTER EXTENSION UPGRADE
> issues, we can ship 9.1 as-is and just label the EXTENSION commands as
> subject to change.  I will however now go work on those issues.

Wise move.  Again :)

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