Обсуждение: Extensions versus pg_upgrade

It just occurred to me that the extensions patch thoroughly breaks
pg_upgrade, because pg_upgrade imagines that it can control the specific
OIDs assigned to certain SQL objects such as data types.  That is of
course not gonna happen for objects within an extension.  In fact, since
part of the point of an extension is that the set of objects it packages
might change across versions, it wouldn't be sensible to try to force
OID assignments even if there were a practical way to do it.

Offhand the only escape route that I can see is for a binary upgrade to
not try to install a newer version of an extension, but just duplicate
all the contained objects exactly.  Which is probably less than trivial
to do --- we could get part of the way there by having pg_dump not
suppress the member objects in --binary-upgrade mode, but then we need
some awful kluge to re-establish their pg_depend links to the extension.
In any case that would ratchet the priority of ALTER EXTENSION UPGRADE
back up to a must-have-for-9.1, since pg_upgrade would then leave you
with a non-upgraded extension.

Now what?
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:
> In any case that would ratchet the priority of ALTER EXTENSION UPGRADE
> back up to a must-have-for-9.1, since pg_upgrade would then leave you
> with a non-upgraded extension.
>
> Now what?

What would be the problem with pg_upgrade acting the same as a
dump&reload cycle as far as extensions are concerned?  After all those
can be considered as part of the schema, not part of the data, and the
system catalogs are upgraded by the tool.

It would then only break user objects that depend on the extension's
objects OIDs, but that would be the same if they instead recorded the
OID of catalog entries, right?

So a valid answer for me would be that when you pg_upgrade, the
extensions are installed again from their scripts.  If you want to go
further than that, you can insist on having the same version of the
extension on both sides, but that would defeat the purpose of the tool
somehow.  After all you asked for an upgrade…

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

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> Now what?

> What would be the problem with pg_upgrade acting the same as a
> dump&reload cycle as far as extensions are concerned?

Um, how about "it doesn't work"?

The reason that data type OIDs have to be preserved, for example, is
that they might be out on disk.  Suppose that you have the cube
extension installed and there are some user tables containing columns of
type cube[].  Those arrays are going to have type cube's OID embedded in
them.  If cube has a different OID after pg_upgrade then the arrays are
broken.

Even letting an extension script run and create data types that weren't
there at all before is problematic, since those types could easily claim
OIDs that need to be reserved for pre-existing types that appear later
in the dump script.

Similar issues arise for the other cases where pg_upgrade is forcing OID
assignments; it's not doing that just for fun.
        regards, tom lane

On Tue, Feb 8, 2011 at 11:53 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> It just occurred to me that the extensions patch thoroughly breaks
> pg_upgrade, because pg_upgrade imagines that it can control the specific
> OIDs assigned to certain SQL objects such as data types.  That is of
> course not gonna happen for objects within an extension.  In fact, since
> part of the point of an extension is that the set of objects it packages
> might change across versions, it wouldn't be sensible to try to force
> OID assignments even if there were a practical way to do it.
>
> Offhand the only escape route that I can see is for a binary upgrade to
> not try to install a newer version of an extension, but just duplicate
> all the contained objects exactly.  Which is probably less than trivial
> to do --- we could get part of the way there by having pg_dump not
> suppress the member objects in --binary-upgrade mode, but then we need
> some awful kluge to re-establish their pg_depend links to the extension.
> In any case that would ratchet the priority of ALTER EXTENSION UPGRADE
> back up to a must-have-for-9.1, since pg_upgrade would then leave you
> with a non-upgraded extension.

If we're going to commit any part of this for 9.1, it doesn't seem
like there's much choice but to insert the above-mentioned awful
kludge.  It's certainly not going to work if we fail to preserve the
OIDs in cases where pg_upgrade otherwise thinks it's necessary.

I guess I'm sort of coming to the conclusion that ALTER EXTENSION ..
UPGRADE is pretty much a must-have for a useful feature regardless of
this issue.  I had previously thought that we might be able to limp
along with half a feature for one release - if you're not actually
depending on anything in the extension, you could always drop it and
the install the new version.  But the more I think about it, the less
realistic that sounds; dependencies on the extension are going to be
very common, and while it may be practical to drop and recreate the
dependent objects in some cases, it's certainly going to annoy a lot
of people.

So I think the choices, realistically, are boot both halves of this to
9.2, or do it all now.  I don't really have an opinion on which way to
go with it.  We still have more than 40 patches to deal with for this
CommitFest, and certainly booting this would free up a bunch of your
time to go work on other things.  On the other hand, if you are fired
up about getting this done, maybe it makes sense to get it knocked out
while it's fresh in your mind instead of letting it linger for another
six months.  It's clearly a good feature, and I know that Dimitri has
put a lot of work into getting it this far.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Feb 8, 2011, at 9:36 AM, Robert Haas wrote:

> I guess I'm sort of coming to the conclusion that ALTER EXTENSION ..
> UPGRADE is pretty much a must-have for a useful feature regardless of
> this issue.  I had previously thought that we might be able to limp
> along with half a feature for one release - if you're not actually
> depending on anything in the extension, you could always drop it and
> the install the new version.  But the more I think about it, the less
> realistic that sounds; dependencies on the extension are going to be
> very common, and while it may be practical to drop and recreate the
> dependent objects in some cases, it's certainly going to annoy a lot
> of people.

I was just thinking about the upgrade issue, and was wondering if this was do-able:

* No upgrade scripts. Nothing to concatenate, include, or maintain.
* No version tracking

Yeah, what I'm saying is, throw out the whole thing. Instead, what would happen is
   ALTER EXTENSION foo UPGRADE;

Would do some trickery behind the scenes to just delete all those objects that are part of the extension (just like
DROPEXTENSION should do), and then run the installation SQL script. So the objects would be exactly as specified in the
installationscript, with no need for the extension maintainer to worry about how to run upgrades, and no need for
PostgreSQLto track version numbers. 

Is this do-able? I recognize that there could be some issues with settings tables and what-not, but surely that's no
differentthan for dump and reload. The question in my mind is whether it would even be possible for the extension code
tounload every bit of an extension and load the new stuff, inside a transaction. 

Thoughts?

Best,

David

On Tue, Feb 8, 2011 at 12:57 PM, David E. Wheeler <david@kineticode.com> wrote:
> On Feb 8, 2011, at 9:36 AM, Robert Haas wrote:
>
>> I guess I'm sort of coming to the conclusion that ALTER EXTENSION ..
>> UPGRADE is pretty much a must-have for a useful feature regardless of
>> this issue.  I had previously thought that we might be able to limp
>> along with half a feature for one release - if you're not actually
>> depending on anything in the extension, you could always drop it and
>> the install the new version.  But the more I think about it, the less
>> realistic that sounds; dependencies on the extension are going to be
>> very common, and while it may be practical to drop and recreate the
>> dependent objects in some cases, it's certainly going to annoy a lot
>> of people.
>
> I was just thinking about the upgrade issue, and was wondering if this was do-able:
>
> * No upgrade scripts. Nothing to concatenate, include, or maintain.
> * No version tracking
>
> Yeah, what I'm saying is, throw out the whole thing. Instead, what would happen is
>
>    ALTER EXTENSION foo UPGRADE;
>
> Would do some trickery behind the scenes to just delete all those objects that are part of the extension (just like
DROPEXTENSION should do), and then run the installation SQL script. So the objects would be exactly as specified in the
installationscript, with no need for the extension maintainer to worry about how to run upgrades, and no need for
PostgreSQLto track version numbers. 
>
> Is this do-able? I recognize that there could be some issues with settings tables and what-not, but surely that's no
differentthan for dump and reload. The question in my mind is whether it would even be possible for the extension code
tounload every bit of an extension and load the new stuff, inside a transaction. 

No, this is not doable, or at least not in a way that provides any
benefit over just dropping and reinstalling.  The problem is that it
is going to fall down all over the place if other objects are
depending on objects provided by the extension.  Like:

CREATE VIEW v AS SELECT extensionfunc(1);

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> I guess I'm sort of coming to the conclusion that ALTER EXTENSION ..
> UPGRADE is pretty much a must-have for a useful feature regardless of
> this issue.  I had previously thought that we might be able to limp
> along with half a feature for one release - if you're not actually
> depending on anything in the extension, you could always drop it and
> the install the new version.  But the more I think about it, the less
> realistic that sounds; dependencies on the extension are going to be
> very common, and while it may be practical to drop and recreate the
> dependent objects in some cases, it's certainly going to annoy a lot
> of people.

> So I think the choices, realistically, are boot both halves of this to
> 9.2, or do it all now.  I don't really have an opinion on which way to
> go with it.  We still have more than 40 patches to deal with for this
> CommitFest, and certainly booting this would free up a bunch of your
> time to go work on other things.  On the other hand, if you are fired
> up about getting this done, maybe it makes sense to get it knocked out
> while it's fresh in your mind instead of letting it linger for another
> six months.  It's clearly a good feature, and I know that Dimitri has
> put a lot of work into getting it this far.

Personally I find the extension stuff to be a bigger deal than anything
else remaining in the commitfest.  Also, I've fixed a number of
pre-existing bugs in passing, and I'd have to go extract those changes
out of the current extensions patch if we abandon it now.  So I'd like
to keep pushing ahead on this.

After further reflection I think that the pg_upgrade situation can be
handled like this:

1. Provide a way for CREATE EXTENSION to not run the script --- either
add a grammar option for that, or just have a back-door flag that
pg_upgrade can set via one of its special kluge functions.  (Hm,
actually we'd want it to install the old version number and comment
too, so maybe just have a kluge function to create a pg_extension
entry that agrees with the old entry.)

2. Invent a command "ALTER EXTENSION extname ADD object-description"
that simply adds a pg_depend entry marking an existing object as
belonging to the extension.  I think this was going to be part of the
plan anyway for ALTER EXTENSION UPGRADE, specifically we need that for
the bootstrap case of collecting some loose pre-existing objects into
an extension.

3. In --binary-upgrade mode, pg_dump doesn't suppress the individual
member objects, but instead emits ALTER EXTENSION ADD after creating
each member object.

So that gets us to the point of being able to run pg_upgrade without
changing the contents of the extension.  After that we can look at
ALTER EXTENSION UPGRADE.
        regards, tom lane

On Feb 8, 2011, at 10:03 AM, Robert Haas wrote:

> No, this is not doable, or at least not in a way that provides any
> benefit over just dropping and reinstalling.  The problem is that it
> is going to fall down all over the place if other objects are
> depending on objects provided by the extension.  Like:
>
> CREATE VIEW v AS SELECT extensionfunc(1);

Ah, right, of course. I don't suppose CREATE OR REPLACE would work, either, in some cases at least?

Best,

David

"David E. Wheeler" <david@kineticode.com> writes:
> On Feb 8, 2011, at 10:03 AM, Robert Haas wrote:
>> No, this is not doable, or at least not in a way that provides any
>> benefit over just dropping and reinstalling.  The problem is that it
>> is going to fall down all over the place if other objects are
>> depending on objects provided by the extension.  Like:
>> 
>> CREATE VIEW v AS SELECT extensionfunc(1);

> Ah, right, of course. I don't suppose CREATE OR REPLACE would work, either, in some cases at least?

But figuring out just what commands to issue is pretty nearly AI-complete.
The whole point of ALTER EXTENSION UPGRADE is to have a human do that
and then give you a script to run.
        regards, tom lane

On Feb 8, 2011, at 10:24 AM, Tom Lane wrote:

>> Ah, right, of course. I don't suppose CREATE OR REPLACE would work, either, in some cases at least?
>
> But figuring out just what commands to issue is pretty nearly AI-complete.
> The whole point of ALTER EXTENSION UPGRADE is to have a human do that
> and then give you a script to run.

Damn. Okay, was just trying to think outside the box a bit here.

Best,

David

Tom Lane <tgl@sss.pgh.pa.us> writes:
> that they might be out on disk.  Suppose that you have the cube
> extension installed and there are some user tables containing columns of
> type cube[].  Those arrays are going to have type cube's OID embedded in
> them.  If cube has a different OID after pg_upgrade then the arrays are
> broken.

Forgot about the internals of arrays.  Sure, that's a problem here.

> Even letting an extension script run and create data types that weren't
> there at all before is problematic, since those types could easily claim
> OIDs that need to be reserved for pre-existing types that appear later
> in the dump script.
>
> Similar issues arise for the other cases where pg_upgrade is forcing OID
> assignments; it's not doing that just for fun.

There's provision to forcing the OID of an extension at CREATE EXTENSION
time in the UPGRADE patch, but it does not handle the extension
objects.

I though system OIDs and user OIDs can't clash because of a 16384
counter magic assignment at initdb, so I'm having a hard time getting to
understand exactly the problem case.  Will spend time on it tomorrow, if
that still helps.

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

Tom Lane <tgl@sss.pgh.pa.us> writes:
> Personally I find the extension stuff to be a bigger deal than anything
> else remaining in the commitfest.  Also, I've fixed a number of
> pre-existing bugs in passing, and I'd have to go extract those changes
> out of the current extensions patch if we abandon it now.  So I'd like
> to keep pushing ahead on this.

Stealing words from Kevin, WOO-HOO :)  I'll try to continue devote as
much time as I did already to assist as much as I can here.

> After further reflection I think that the pg_upgrade situation can be
> handled like this:
>
> 1. Provide a way for CREATE EXTENSION to not run the script --- either
> add a grammar option for that, or just have a back-door flag that
> pg_upgrade can set via one of its special kluge functions.  (Hm,
> actually we'd want it to install the old version number and comment
> too, so maybe just have a kluge function to create a pg_extension
> entry that agrees with the old entry.)

In the upgrade patch there's provision for not running the script:
 CREATE WRAPPER EXTENSION foo;

That's pretty useful indeed.  What it does now is read the control file
to init the comment and other fields, but extversion is forced NULL.
That allows to have special rules in how UPGRADE will pick a script.

There's even code to do
 CREATE WRAPPER EXTENSION foo WITH SYSID 12345;

We could add version and comment here for purposes of pg_upgrade.

> 2. Invent a command "ALTER EXTENSION extname ADD object-description"
> that simply adds a pg_depend entry marking an existing object as
> belonging to the extension.  I think this was going to be part of the
> plan anyway for ALTER EXTENSION UPGRADE, specifically we need that for
> the bootstrap case of collecting some loose pre-existing objects into
> an extension.

In the upgrade patch, that's spelled ALTER OBJECT foo SET EXTENSION bar;
and does exactly what you're proposing.

> 3. In --binary-upgrade mode, pg_dump doesn't suppress the individual
> member objects, but instead emits ALTER EXTENSION ADD after creating
> each member object.
>
> So that gets us to the point of being able to run pg_upgrade without
> changing the contents of the extension.  After that we can look at
> ALTER EXTENSION UPGRADE.

Well, or begin there as the code you need is already written.

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

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> 2. Invent a command "ALTER EXTENSION extname ADD object-description"
>> that simply adds a pg_depend entry marking an existing object as
>> belonging to the extension.  I think this was going to be part of the
>> plan anyway for ALTER EXTENSION UPGRADE, specifically we need that for
>> the bootstrap case of collecting some loose pre-existing objects into
>> an extension.

> In the upgrade patch, that's spelled ALTER OBJECT foo SET EXTENSION bar;
> and does exactly what you're proposing.

OK, that seems like an equally reasonable syntax; although doing it the
way I was thinking might have less of a code and documentation footprint
(I was vaguely imagining that it could share most of the COMMENT
infrastructure --- but haven't looked yet).  In any case it seems like
this is a good piece to do next, since the required functionality is
clear and it's essential for more than one reason.
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:
> OK, that seems like an equally reasonable syntax; although doing it the
> way I was thinking might have less of a code and documentation footprint
> (I was vaguely imagining that it could share most of the COMMENT
> infrastructure --- but haven't looked yet).  In any case it seems like
> this is a good piece to do next, since the required functionality is
> clear and it's essential for more than one reason.

Well the code footprint is quite small already.  When the grammar change
and the alter.c addition is in, it boils down to:

/** ALTER OBJECT SET EXTENSION** This form allows for upgrading from previous PostgreSQL version to one* supporting
extensions,or to upgrade user objects to get to use the* extension infrastructure.** All we have to do is record an
INTERNALdependency between the selected* object and the extension, which must of course already exist.*/ 
void
AlterObjectExtension(const char *extname,                 Oid classId, Oid objectId, Oid objectSubId)
{ObjectAddress   *extension, *object;
if (!superuser())    ereport(ERROR,            (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),             (errmsg("must be
superuserto SET EXTENSION")))); 
extension = (ObjectAddress *)palloc(sizeof(ObjectAddress));extension->classId = ExtensionRelationId;extension->objectId
=get_extension_oid(extname, false);extension->objectSubId = 0; 
object = (ObjectAddress *)palloc(sizeof(ObjectAddress));object->classId = classId;object->objectId =
objectId;object->objectSubId= objectSubId; 
recordDependencyOn(object, extension, DEPENDENCY_INTERNAL);
return;
}

Of course it needs some changes now, but well… I guess you'll be done
with that before I get to rebase my patch, I can only target tomorrow
now.

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

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>>> [ ALTER object SET EXTENSION versus ALTER EXTENSION ADD object ]

>> OK, that seems like an equally reasonable syntax; although doing it the
>> way I was thinking might have less of a code and documentation footprint
>> (I was vaguely imagining that it could share most of the COMMENT
>> infrastructure --- but haven't looked yet).  In any case it seems like
>> this is a good piece to do next, since the required functionality is
>> clear and it's essential for more than one reason.

> Well the code footprint is quite small already.

I was thinking about it more from the documentation side: touch one man
page versus touch nearly all the ALTER pages.  In addition, if it's all
on the ALTER EXTENSION page then we can reference that as a list of the
types of objects managed by extensions, which is something that's
documented nowhere right now.

Has anybody got any strong preference for one of these alternatives on
more abstract grounds?  You could cite ALTER OBJECT SET NAMESPACE/OWNER
as precedents for the one syntax, but COMMENT seems like a precedent
for the other, so that consideration seems like nearly a wash to me.

Any other opinions out there?
        regards, tom lane

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> (I was vaguely imagining that it could share most of the COMMENT
>> infrastructure --- but haven't looked yet).

> Well the code footprint is quite small already.

Having now looked at it a bit closer, I think the syntax choice is a
complete wash from an implementation standpoint: either way, we'll have
a list of bison productions that build AlterObjectExtensionStmt nodes,
and it goes through the same way after that.  I do think that the
implementation will be a lot more compact if it relies on the COMMENT
infrastructure (ie, get_object_address), but that's an independent
choice.

So really it boils down to which syntax seems more natural and/or easier
to document.  As I said, I think a centralized ALTER EXTENSION syntax
has some advantages from the documentation standpoint; but that's not a
terribly strong argument, especially given that Dimitri has already done
a patch to document things the other way.

Preferences anyone?
        regards, tom lane

On Tue, Feb 8, 2011 at 6:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
>> Tom Lane <tgl@sss.pgh.pa.us> writes:
>>> (I was vaguely imagining that it could share most of the COMMENT
>>> infrastructure --- but haven't looked yet).
>
>> Well the code footprint is quite small already.
>
> Having now looked at it a bit closer, I think the syntax choice is a
> complete wash from an implementation standpoint: either way, we'll have
> a list of bison productions that build AlterObjectExtensionStmt nodes,
> and it goes through the same way after that.  I do think that the
> implementation will be a lot more compact if it relies on the COMMENT
> infrastructure (ie, get_object_address), but that's an independent
> choice.
>
> So really it boils down to which syntax seems more natural and/or easier
> to document.  As I said, I think a centralized ALTER EXTENSION syntax
> has some advantages from the documentation standpoint; but that's not a
> terribly strong argument, especially given that Dimitri has already done
> a patch to document things the other way.
>
> Preferences anyone?

The closest exstant parallel is probably:

ALTER SEQUENCE foo OWNED BY bar;

I think paralleling that would probably be the most SQL-ish thing to
do, but I can't get excited about it.  The ALTER EXTENSION syntax will
be a lot more self-contained, with all of it one part of the grammar
and one part of the documentation.  And you could even allow multiple
objects:

ALTER EXTENSION extension_name ADD object-description [, ...];

Which might be handy.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Feb 8, 2011 at 6:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Having now looked at it a bit closer, I think the syntax choice is a
>> complete wash from an implementation standpoint: either way, we'll have
>> a list of bison productions that build AlterObjectExtensionStmt nodes,
>> and it goes through the same way after that.
>>
>> Preferences anyone?

> The closest exstant parallel is probably:

> ALTER SEQUENCE foo OWNED BY bar;

> I think paralleling that would probably be the most SQL-ish thing to
> do, but I can't get excited about it.

ALTER SET SCHEMA is a relatively near match as well, I think, from a
semantic standpoint.

> The ALTER EXTENSION syntax will
> be a lot more self-contained, with all of it one part of the grammar
> and one part of the documentation.  And you could even allow multiple
> objects:

> ALTER EXTENSION extension_name ADD object-description [, ...];

> Which might be handy.

Hmm, that's an interesting thought.  It'd require rather more
refactoring of the grammar and the parsetree representation than I care
to get into right now, but that could be a foreseeable extension in
future.  On the other hand, it's not *that* exciting, and if multi ADD
isn't supported in the very first version then probably nobody will ever
want to rely on it in extension scripts.

I'm still finding that the "document in one place" angle is the most
compelling argument to me.
        regards, tom lane

Robert Haas <robertmhaas@gmail.com> writes:
> ... And you could even allow multiple objects:
> ALTER EXTENSION extension_name ADD object-description [, ...];
> Which might be handy.

I just thought of a different way of coming at the question, which might
help us make a choice.

Like ALTER THING SET SCHEMA, ALTER THING SET EXTENSION is implicitly
assuming that there can be only one owning extension for an object.
Furthermore, it's not really intended for *removal* of an object from an
extension (a concept that doesn't even exist for SET SCHEMA).  We could
take a page from COMMENT ON and use "SET EXTENSION NULL" for that, but
that's surely more of a hack than anything else.

In contrast, ALTER EXTENSION ADD doesn't presuppose that you couldn't
add the object to multiple extensions; and it has a natural inverse,
ALTER EXTENSION DROP.  I am not necessarily suggesting that we will ever
allow either of those things, but I do suggest that we should pick a
syntax that doesn't look like it's being forced to conform if we ever
want to do it.  The DROP case at least seems like it might be wanted
in the relatively near future.

So that looks to me like a fairly good argument for the ADD syntax.
        regards, tom lane

On Tue, Feb 8, 2011 at 9:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> ... And you could even allow multiple objects:
>> ALTER EXTENSION extension_name ADD object-description [, ...];
>> Which might be handy.
>
> I just thought of a different way of coming at the question, which might
> help us make a choice.
>
> Like ALTER THING SET SCHEMA, ALTER THING SET EXTENSION is implicitly
> assuming that there can be only one owning extension for an object.

I would assume that we would enforce that constraint anyway.  No?
Otherwise when you drop one of the two extensions, what happens to the
object?  Seems necessary for sanity.

> Furthermore, it's not really intended for *removal* of an object from an
> extension (a concept that doesn't even exist for SET SCHEMA).  We could
> take a page from COMMENT ON and use "SET EXTENSION NULL" for that, but
> that's surely more of a hack than anything else.

True.

> In contrast, ALTER EXTENSION ADD doesn't presuppose that you couldn't
> add the object to multiple extensions; and it has a natural inverse,
> ALTER EXTENSION DROP.  I am not necessarily suggesting that we will ever
> allow either of those things, but I do suggest that we should pick a
> syntax that doesn't look like it's being forced to conform if we ever
> want to do it.  The DROP case at least seems like it might be wanted
> in the relatively near future.

Yep.

> So that looks to me like a fairly good argument for the ADD syntax.

OK by me.  There's also the operator class stuff, as a parallel.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Feb 8, 2011 at 9:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Like ALTER THING SET SCHEMA, ALTER THING SET EXTENSION is implicitly
>> assuming that there can be only one owning extension for an object.

> I would assume that we would enforce that constraint anyway.  No?
> Otherwise when you drop one of the two extensions, what happens to the
> object?  Seems necessary for sanity.

Not sure --- what about nested extensions, for instance?  Or you could
think about objects that are shared between two extensions, and go away
only if all those extensions are dropped.  (RPM has exactly that
behavior for files owned by multiple packages, to take a handy example.)

My point is that the current restriction to just one containing
extension seems to me to be an implementation restriction, rather than
something inherent in the concept of extensions.  I have no intention of
trying to relax that restriction in the near future --- I'm just
pointing out that it could become an interesting thing to do.
        regards, tom lane

On Tue, Feb 8, 2011 at 10:25 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Tue, Feb 8, 2011 at 9:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Like ALTER THING SET SCHEMA, ALTER THING SET EXTENSION is implicitly
>>> assuming that there can be only one owning extension for an object.
>
>> I would assume that we would enforce that constraint anyway.  No?
>> Otherwise when you drop one of the two extensions, what happens to the
>> object?  Seems necessary for sanity.
>
> Not sure --- what about nested extensions, for instance?  Or you could
> think about objects that are shared between two extensions, and go away
> only if all those extensions are dropped.  (RPM has exactly that
> behavior for files owned by multiple packages, to take a handy example.)
>
> My point is that the current restriction to just one containing
> extension seems to me to be an implementation restriction, rather than
> something inherent in the concept of extensions.  I have no intention of
> trying to relax that restriction in the near future --- I'm just
> pointing out that it could become an interesting thing to do.

OK.  My point was that I think we should definitely *enforce* that
restriction until we have a very clear vision of what it means to do
anything else, so it sounds like we're basically in agreement.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Feb 8, 2011 at 10:25 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> My point is that the current restriction to just one containing
>> extension seems to me to be an implementation restriction, rather than
>> something inherent in the concept of extensions. �I have no intention of
>> trying to relax that restriction in the near future --- I'm just
>> pointing out that it could become an interesting thing to do.

> OK.  My point was that I think we should definitely *enforce* that
> restriction until we have a very clear vision of what it means to do
> anything else, so it sounds like we're basically in agreement.

Oh, for certain.  I've been busy revising Dimitri's patch to use the
get_object_address infrastructure, and here's what I've got for the
actual implementation as distinct from syntax:


+ /*
+  * Execute ALTER THING SET EXTENSION
+  */
+ void
+ ExecAlterObjectExtensionStmt(AlterObjectExtensionStmt *stmt)
+ {
+     ObjectAddress    object;
+     ObjectAddress    extension;
+     Relation        relation;
+ 
+     /*
+      * For now, insist on superuser privilege.  Later we might want to
+      * relax this to ownership of the target object and the extension.
+      */
+     if (!superuser())
+         ereport(ERROR,
+                 (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+                  (errmsg("must be superuser to use ALTER SET EXTENSION"))));
+ 
+     /*
+      * Translate the parser representation that identifies this object into
+      * an ObjectAddress.  get_object_address() will throw an error if the
+      * object does not exist, and will also acquire a lock on the target
+      * to guard against concurrent DROP and SET EXTENSION operations.
+      */
+     object = get_object_address(stmt->objtype, stmt->objname, stmt->objargs,
+                                 &relation, ShareUpdateExclusiveLock);
+ 
+     /*
+      * Complain if object is already attached to some extension.
+      */
+     if (getExtensionOfObject(object.classId, object.objectId) != InvalidOid)
+         ereport(ERROR,
+                 (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+                  errmsg("%s is already a member of an extension",
+                         getObjectDescription(&object))));
+ 
+     /*
+      * OK, add the dependency.
+      */
+     extension.classId = ExtensionRelationId;
+     extension.objectId = get_extension_oid(stmt->extname, false);
+     extension.objectSubId = 0;
+ 
+     recordDependencyOn(&object, &extension, DEPENDENCY_EXTENSION);
+ 
+     /*
+      * If get_object_address() opened the relation for us, we close it to keep
+      * the reference count correct - but we retain any locks acquired by
+      * get_object_address() until commit time, to guard against concurrent
+      * activity.
+      */
+     if (relation != NULL)
+         relation_close(relation, NoLock);
+ }

        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:
> Like ALTER THING SET SCHEMA, ALTER THING SET EXTENSION is implicitly
> assuming that there can be only one owning extension for an object.

Yes, I worked from the SET SCHEMA variant and mentally mapped SET
EXTENSION there, if looked like the same idea applied to another
"property" of the object.

> Furthermore, it's not really intended for *removal* of an object from an
> extension (a concept that doesn't even exist for SET SCHEMA).  We could
> take a page from COMMENT ON and use "SET EXTENSION NULL" for that, but
> that's surely more of a hack than anything else.
>
> In contrast, ALTER EXTENSION ADD doesn't presuppose that you couldn't
> add the object to multiple extensions; and it has a natural inverse,

Well I wouldn't want to get there.  I'm not seeing what use case we
would solve by having more than one extension install the same object, I
would rather have a common extension that the others depend on.

> ALTER EXTENSION DROP.  I am not necessarily suggesting that we will ever
> allow either of those things, but I do suggest that we should pick a
> syntax that doesn't look like it's being forced to conform if we ever
> want to do it.  The DROP case at least seems like it might be wanted
> in the relatively near future.

I didn't think of that case because I would think the upgrade script
will just DROP OBJECT instead.  But in some cases I can see extension
authors wanting to ALTER EXTENSION DROP OBJECT in their upgrade script
and provide a second-stage script or procedure to clean up the database
once upgraded.  Only when you don't need the object anymore you can drop
it entirely.

I'm not sure how contrived the use case is here, but I agree that being
prepared for it makes sense.

Adding more that one object in one command is not of a great value I
think, because you still have to lock each object individually, and
that's transaction bound.  Unlike ALTER TABLE … ADD COLUMN where it's a
huge benefit to be able to lock and update the table only once for a
number of columns (add and drops).

But at the same time once the work is done, adding some syntax
flexibility and a loop or two doesn't look too bad if you wanted to get
there.  Well no strong opinion as I'm not doing the work :)

As far as upgrade script for contrib extensions are concerned, we will
be able to produce them from SQL, right?  The trick is to install the
extension first, of course.
 CREATE EXTENSION foo;
 CREATE SCHEMA empty_place; SET search_path TO empty_place;
 SELECT 'ALTER EXTENSION ' || E.extname || ' ADD '     || replace(pg_describe_object(classid, objid, 0),
N.nspname,'@extschema@')     || ';'   FROM pg_depend D        JOIN pg_extension E ON D.refobjid = E.oid
         AND D.refclassid = E.tableoid        JOIN pg_namespace N ON E.extnamespace = N.oid WHERE deptype = 'e' AND
E.extname= 'foo'; 

I think it would be a good idea to have that in the documentation to
help authors prepare their first upgrade script.  Well to the extend
that a previous form of it is included in the docs I've put in the
upgrade patch :)

So replacing those scripts I've been working on to switch to the new
syntax would be a matter of running a shell script.  The time consuming
part is definitely the testing, but that too can be scripted.
 DROP EXTENSION foo; \i path/to/share/contrib/foo.sql create wrapper extension foo; alter extension foo upgrade; \dx
foo

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

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> As far as upgrade script for contrib extensions are concerned, we will
> be able to produce them from SQL, right?

Hm, interesting idea, but I'm afraid that pg_describe_object doesn't
produce exactly the syntax you need.

I had personally been thinking of generating the contrib upgrade scripts
via search-and-replace on the existing uninstall scripts.
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:
> Hm, interesting idea, but I'm afraid that pg_describe_object doesn't
> produce exactly the syntax you need.

It's very close.  I've produced the previous set like that and the only
problem I had were with operator class and family objects, and with
array types.  In both case a very simple replace can be used, like
replace int[] with _int and "for access method" with "using".

So you just add a CASE in the SELECT I proposed.  Well, I didn't do it
because I was not sure that it would still be needed with the API you're
using.

> I had personally been thinking of generating the contrib upgrade scripts
> via search-and-replace on the existing uninstall scripts.

Maybe that would work too.

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

On Feb 8, 2011, at 6:48 PM, Tom Lane wrote:

> Like ALTER THING SET SCHEMA, ALTER THING SET EXTENSION is implicitly
> assuming that there can be only one owning extension for an object.
> Furthermore, it's not really intended for *removal* of an object from an
> extension (a concept that doesn't even exist for SET SCHEMA).  We could
> take a page from COMMENT ON and use "SET EXTENSION NULL" for that, but
> that's surely more of a hack than anything else.
>
> In contrast, ALTER EXTENSION ADD doesn't presuppose that you couldn't
> add the object to multiple extensions; and it has a natural inverse,
> ALTER EXTENSION DROP.  I am not necessarily suggesting that we will ever
> allow either of those things, but I do suggest that we should pick a
> syntax that doesn't look like it's being forced to conform if we ever
> want to do it.  The DROP case at least seems like it might be wanted
> in the relatively near future.
>
> So that looks to me like a fairly good argument for the ADD syn

It feels a lot more natural to me, frankly. I'd tend to think about what's grouped into an extension, and look for the
documentationrelated to extensions for how to add an object to an extension. I don't think it would occur to me, on
firstpass, to look in the ALTER FUNCTION docs for how to add a function to an extension. 

In my mind, I'm modifying the extension, not the function.

Best,

David

Dimitri Fontaine wrote:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
> > In any case that would ratchet the priority of ALTER EXTENSION UPGRADE
> > back up to a must-have-for-9.1, since pg_upgrade would then leave you
> > with a non-upgraded extension.
> >
> > Now what?
> 
> What would be the problem with pg_upgrade acting the same as a
> dump&reload cycle as far as extensions are concerned?  After all those
> can be considered as part of the schema, not part of the data, and the
> system catalogs are upgraded by the tool.
> 
> It would then only break user objects that depend on the extension's
> objects OIDs, but that would be the same if they instead recorded the
> OID of catalog entries, right?
> 
> So a valid answer for me would be that when you pg_upgrade, the
> extensions are installed again from their scripts.  If you want to go
> further than that, you can insist on having the same version of the
> extension on both sides, but that would defeat the purpose of the tool
> somehow.  After all you asked for an upgrade?

The C comment in pg_upgrade.c explains the problem:
*  We control all assignments of pg_type.oid because these oids are stored*  in user composite type values.

(Wow, I am glad I recorded all these details.)

The problem is that pg_dump --binary-upgrade knows to call
binary_upgrade.set_next_pg_type_oid() before CREATE TYPE (you can test
it yourself to see), and I am afraid we will need to do something like
that in the extension code, perhaps by supporting a --binary-upgrade
flag like we do for pg_dump.  That seems to be the cleanest approach.
A worse approach would be to somehow pass oids to pg_upgrade and have it
renumber things but that seems hopelessly error-prone.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Feb 8, 2011 at 9:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> In contrast, ALTER EXTENSION ADD doesn't presuppose that you couldn't
>> add the object to multiple extensions; and it has a natural inverse,
>> ALTER EXTENSION DROP. �I am not necessarily suggesting that we will ever
>> allow either of those things, but I do suggest that we should pick a
>> syntax that doesn't look like it's being forced to conform if we ever
>> want to do it. �The DROP case at least seems like it might be wanted
>> in the relatively near future.

> Yep.

Actually, it occurs to me that the need for ALTER EXTENSION DROP could
be upon us sooner than we think.  The cases where an extension upgrade
script would need that are
(1) you want to remove some deprecated piece of the extension's API;
(2) you want to remove some no-longer-needed internal function.
Without ALTER EXTENSION DROP it's flat out impossible to do either.

Deprecated API is not exactly far to seek in our contrib modules,
either --- the example that just reminded me of this is hstore's =>
operator, which we're already going so far as to print warnings about.
We're not going to get to remove that until at least one release after
we support ALTER EXTENSION DROP.

So I'm thinking it'd be smart to expend the small amount of additional
effort needed to support DROP right off the bat.  I think that
AlterExtensionAddStmt could be extended with an add/drop boolean for
a net addition of only a few dozen lines of code, most of that being a
suitable search-and-delete function in pg_depend.c.

Any objections?
        regards, tom lane

On Thu, Feb 10, 2011 at 10:41 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Tue, Feb 8, 2011 at 9:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> In contrast, ALTER EXTENSION ADD doesn't presuppose that you couldn't
>>> add the object to multiple extensions; and it has a natural inverse,
>>> ALTER EXTENSION DROP.  I am not necessarily suggesting that we will ever
>>> allow either of those things, but I do suggest that we should pick a
>>> syntax that doesn't look like it's being forced to conform if we ever
>>> want to do it.  The DROP case at least seems like it might be wanted
>>> in the relatively near future.
>
>> Yep.
>
> Actually, it occurs to me that the need for ALTER EXTENSION DROP could
> be upon us sooner than we think.  The cases where an extension upgrade
> script would need that are
> (1) you want to remove some deprecated piece of the extension's API;
> (2) you want to remove some no-longer-needed internal function.
> Without ALTER EXTENSION DROP it's flat out impossible to do either.
>
> Deprecated API is not exactly far to seek in our contrib modules,
> either --- the example that just reminded me of this is hstore's =>
> operator, which we're already going so far as to print warnings about.
> We're not going to get to remove that until at least one release after
> we support ALTER EXTENSION DROP.
>
> So I'm thinking it'd be smart to expend the small amount of additional
> effort needed to support DROP right off the bat.  I think that
> AlterExtensionAddStmt could be extended with an add/drop boolean for
> a net addition of only a few dozen lines of code, most of that being a
> suitable search-and-delete function in pg_depend.c.
>
> Any objections?

No, I was pretty much just waiting for you to arrive at the same
conclusion I'd already reached.  :-)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Tom Lane <tgl@sss.pgh.pa.us> writes:
> Actually, it occurs to me that the need for ALTER EXTENSION DROP could
> be upon us sooner than we think.  The cases where an extension upgrade
> script would need that are
> (1) you want to remove some deprecated piece of the extension's API;
> (2) you want to remove some no-longer-needed internal function.
> Without ALTER EXTENSION DROP it's flat out impossible to do either.

What if you just DROP FUNCTION in the upgrade script?

That said if the function is used in some expression index or worse,
triggers, you certainly want to give users the opportunity to delay the
step where the function is no more part of the extension from the step
where you get rid of it.

But if the function is implemented in C and the newer shared object has
removed it…

> So I'm thinking it'd be smart to expend the small amount of additional
> effort needed to support DROP right off the bat.  I think that
> AlterExtensionAddStmt could be extended with an add/drop boolean for
> a net addition of only a few dozen lines of code, most of that being a
> suitable search-and-delete function in pg_depend.c.

Given your phrasing about the size of this project, I can't see any
downside here.

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

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> Actually, it occurs to me that the need for ALTER EXTENSION DROP could
>> be upon us sooner than we think.  The cases where an extension upgrade
>> script would need that are
>> (1) you want to remove some deprecated piece of the extension's API;
>> (2) you want to remove some no-longer-needed internal function.
>> Without ALTER EXTENSION DROP it's flat out impossible to do either.

> What if you just DROP FUNCTION in the upgrade script?

That would be rejected because you're not allowed to drop an individual
member object of an extension.  (And no, I don't want to have a kluge in
dependency.c that makes that test work differently when
creating_extension.)
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:
> That would be rejected because you're not allowed to drop an individual
> member object of an extension.  (And no, I don't want to have a kluge in
> dependency.c that makes that test work differently when
> creating_extension.)

Fair enough, all the more as soon as we have ALTER EXTENSION DROP :)

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