Re: [HACKERS] ALTER EXTENSION .. ADD/DROP weirdness
On Mon, Oct 10, 2011 at 2:52 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: But there's a bigger problem: it seems to me that we have an inconsistency between what happens when you create an extension from scratch and when you upgrade it from unpackaged. Both pg_buffercache and pg_stat_statements just do this in the upgrade from unpackaged case: ALTER EXTENSION ext-name ADD view view-name; They do *not* add the type and the array type. But when the 1.0 script is run, the type and array type end up belonging to the extension. This seems bad. Hmm, yeah, we need to make those consistent. The underlying issue here is whether objects dependent on an extension member should have direct dependencies on the extension too, and if not, how do we prevent that? The recordDependencyOnCurrentExtension calls don't have enough information to know what to do, I think. After looking at this code, it seems that we've generally made that the caller's problem - e.g. in heap_create_with_catalog(), we skip recordDependencyOnCurrentExtension() if we're dealing with a composite type. So I think the fix here is just to move the recordDependencyOnCurrentExtension() call in pg_type.c inside the if-block that precedes it, as in the attached patch. Of course, this won't fix any damage already done, but it seems like the right thing going forward. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company extension-type-dependencies.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER EXTENSION .. ADD/DROP weirdness
Robert Haas robertmh...@gmail.com writes: On Mon, Oct 10, 2011 at 2:52 PM, Tom Lane t...@sss.pgh.pa.us wrote: The underlying issue here is whether objects dependent on an extension member should have direct dependencies on the extension too, and if not, how do we prevent that? The recordDependencyOnCurrentExtension calls don't have enough information to know what to do, I think. After looking at this code, it seems that we've generally made that the caller's problem - e.g. in heap_create_with_catalog(), we skip recordDependencyOnCurrentExtension() if we're dealing with a composite type. So I think the fix here is just to move the recordDependencyOnCurrentExtension() call in pg_type.c inside the if-block that precedes it, as in the attached patch. Hmm. I'm afraid that's going to break something, because I had had it like that originally and changed it in commit 988620dd8c16d77f88ede167b22056176324. However, I'm not quite sure *what* it will break, because it seems like in general extension dependencies ought to act pretty nearly like owner dependencies. In a quick look, this seems to be the only place where we're doing it differently (without a clear reason) for recordDependencyOnOwner and recordDependencyOnCurrentExtension. Let me poke at it a bit more. The proposed patch is a bit short on comment fixes, anyway. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER EXTENSION .. ADD/DROP weirdness
I wrote: Hmm. I'm afraid that's going to break something, because I had had it like that originally and changed it in commit 988620dd8c16d77f88ede167b22056176324. However, I'm not quite sure *what* it will break, because it seems like in general extension dependencies ought to act pretty nearly like owner dependencies. In a quick look, this seems to be the only place where we're doing it differently (without a clear reason) for recordDependencyOnOwner and recordDependencyOnCurrentExtension. After studying the code a bit more, I think I was worrying about some corner cases involving shell type replacement; but they're not interesting enough to justify making the main-line cases harder to work with. So I think this is a good fix, and I applied it with some comment adjustments. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER EXTENSION .. ADD/DROP weirdness
Robert Haas robertmh...@gmail.com writes: On Mon, Oct 10, 2011 at 2:52 PM, Tom Lane t...@sss.pgh.pa.us wrote: The underlying issue here is whether objects dependent on an extension member should have direct dependencies on the extension too, and if not, how do we prevent that? The recordDependencyOnCurrentExtension calls don't have enough information to know what to do, I think. I think the original patch, that didn't have the DEPENDENCY_EXTENSION tracking but relied on the INTERNAL stuff, did only record first level objects as a dependency. Given the way INTERNAL dependencies following are done, that kind of worked in a limited set of cases. Well, I'm not an expert on this code, but from a user perspective, I think it would be nicer if only the view ended up being a member of the extension, and the generated types did not. Otherwise, writing an extension upgrade script requires detailed knowledge of what other objects are going to be generated internally. In fact, it doesn't seem implausible that the set of internally generated objects from a given DDL command could change between releases, which would really be rather ugly here. The reason why the original patch got changed by Tom is, of course, that it failed to work properly in some interesting cases. Specifically, handling both your use case and extension dependencies (earthdistance depends on cube) is not so easy. How do you know you're crossing a dependency unit when recursing in pg_depends is a nice exercise if you want to be very familiar with WITH RECURSIVE catalog queries. Been there, done that :) The main test case is DROP EXTENSION earthdistance;, adding CASCADE is easier because you then don't care about stopping at the right place. Of course I'm just trying to help you figure out why the problem is not already solved, please feel free to come back with a design that make it simple enough :) Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER EXTENSION .. ADD/DROP weirdness
Robert Haas robertmh...@gmail.com writes: rhaas=# alter extension pg_stat_statements drop type pg_stat_statements[]; ERROR: syntax error at or near [ LINE 1: ...extension pg_stat_statements drop type pg_stat_statements[]; ^ Hmm. So just how do I do this? alter extension pg_stat_statements drop type _pg_stat_statements, probably. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER EXTENSION .. ADD/DROP weirdness
On Mon, Oct 10, 2011 at 12:34 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: rhaas=# alter extension pg_stat_statements drop type pg_stat_statements[]; ERROR: syntax error at or near [ LINE 1: ...extension pg_stat_statements drop type pg_stat_statements[]; ^ Hmm. So just how do I do this? alter extension pg_stat_statements drop type _pg_stat_statements, probably. *tests* Yeah, that works. But it seems undesirable for people writing upgrade scripts to need to count on the way we generate internal type names for array types. But there's a bigger problem: it seems to me that we have an inconsistency between what happens when you create an extension from scratch and when you upgrade it from unpackaged. Both pg_buffercache and pg_stat_statements just do this in the upgrade from unpackaged case: ALTER EXTENSION ext-name ADD view view-name; They do *not* add the type and the array type. But when the 1.0 script is run, the type and array type end up belonging to the extension. This seems bad. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER EXTENSION .. ADD/DROP weirdness
Robert Haas robertmh...@gmail.com writes: But there's a bigger problem: it seems to me that we have an inconsistency between what happens when you create an extension from scratch and when you upgrade it from unpackaged. Both pg_buffercache and pg_stat_statements just do this in the upgrade from unpackaged case: ALTER EXTENSION ext-name ADD view view-name; They do *not* add the type and the array type. But when the 1.0 script is run, the type and array type end up belonging to the extension. This seems bad. Hmm, yeah, we need to make those consistent. The underlying issue here is whether objects dependent on an extension member should have direct dependencies on the extension too, and if not, how do we prevent that? The recordDependencyOnCurrentExtension calls don't have enough information to know what to do, I think. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER EXTENSION .. ADD/DROP weirdness
On Mon, Oct 10, 2011 at 2:52 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: But there's a bigger problem: it seems to me that we have an inconsistency between what happens when you create an extension from scratch and when you upgrade it from unpackaged. Both pg_buffercache and pg_stat_statements just do this in the upgrade from unpackaged case: ALTER EXTENSION ext-name ADD view view-name; They do *not* add the type and the array type. But when the 1.0 script is run, the type and array type end up belonging to the extension. This seems bad. Hmm, yeah, we need to make those consistent. The underlying issue here is whether objects dependent on an extension member should have direct dependencies on the extension too, and if not, how do we prevent that? The recordDependencyOnCurrentExtension calls don't have enough information to know what to do, I think. Well, I'm not an expert on this code, but from a user perspective, I think it would be nicer if only the view ended up being a member of the extension, and the generated types did not. Otherwise, writing an extension upgrade script requires detailed knowledge of what other objects are going to be generated internally. In fact, it doesn't seem implausible that the set of internally generated objects from a given DDL command could change between releases, which would really be rather ugly here. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
