Обсуждение: ALTER EXTENSION .. ADD/DROP weirdness

OK, I'm stumped:

rhaas=# create extension pg_stat_statements;
CREATE EXTENSION
rhaas=# drop view pg_stat_statements;
ERROR:  cannot drop view pg_stat_statements because extension
pg_stat_statements requires it
HINT:  You can drop extension pg_stat_statements instead.
rhaas=# alter extension pg_stat_statements drop view pg_stat_statements;
ALTER EXTENSION
rhaas=# drop view pg_stat_statements;
ERROR:  cannot drop view pg_stat_statements because other objects depend on it
DETAIL:  extension pg_stat_statements depends on view pg_stat_statements
HINT:  Use DROP ... CASCADE to drop the dependent objects too.

At the very last, the error message is totally confusing, because the
point is that I just removed that object from the extension, and I'm
being told that I can't remove it because it's part of the extension.
A little snooping around with \dx+ reveals a possible cause: the view
itself has been removed from the extension, but the associated types
are still connected to it:

rhaas=# \dx+ pg_stat_statements
Objects in extension "pg_stat_statements"        Object Description
-------------------------------------function pg_stat_statements()function pg_stat_statements_reset()type
pg_stat_statementstypepg_stat_statements[]
 
(4 rows)

OK, no problem, I'll just disconnect those, too:

rhaas=# alter extension pg_stat_statements drop type pg_stat_statements;
ALTER EXTENSION
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?

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

Robert Haas <robertmhaas@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

On Mon, Oct 10, 2011 at 12:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@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

Robert Haas <robertmhaas@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

On Mon, Oct 10, 2011 at 2:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@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

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Oct 10, 2011 at 2:52 PM, Tom Lane <tgl@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

On Mon, Oct 10, 2011 at 2:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@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

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Oct 10, 2011 at 2:52 PM, Tom Lane <tgl@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
988cccc620dd8c16d77f88ede167b22056176324.  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

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
> 988cccc620dd8c16d77f88ede167b22056176324.  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