Re: BUG #6704: ALTER EXTENSION postgis SET SCHEMA leaves dangling relations

The following bug has been logged on the website:

Bug reference: 6704
Logged by: Jeff Frost
Email address: jeff(at)pgexperts(dot)com
PostgreSQL version: 9.1.4
Operating system: Windows and Linux
Description:

DROP and CREATE extension appear to work fine, but if you ALTER EXTENSION
postgis SET SCHEMA foo, it leaves a few relations behind. Then if you drop
that schema, you can't pg_dump the DB anymore.

See reproducible test case below. Note a the bottom that even though the
ALTER left items in the original schema, I'm able to drop that schema
without CASCADE and also if I then DROP EXTENSION, it happily gets rid of
those.

Test case:
pgx-test:~ $ createdb ext_test
pgx-test:~ $ psql ext_test
psql (9.1.4)
Type "help" for help.

ext_test=# create schema test;
CREATE SCHEMA
Time: 27.736 ms
ext_test=# create EXTENSION postgis with schema test;
CREATE EXTENSION
Time: 764.102 ms
ext_test=# alter EXTENSION postgis set schema public;
ALTER EXTENSION
Time: 221.224 ms
ext_test=# select oid, nspname from pg_namespace ;
oid | nspname
---------+--------------------
99 | pg_toast
11124 | pg_temp_1
11125 | pg_toast_temp_1
11 | pg_catalog
2200 | public
12257 | information_schema
6981446 | test
(7 rows)

Time: 0.256 ms
ext_test=# select oid, relname, relnamespace from pg_class where
relnamespace = 6981446;

oid | relname | relnamespace
---------+----------------------+--------------
6981694 | spatial_ref_sys_pkey | 6981446
(1 row)

Time: 36.072 ms
ext_test=# select oid, proname, pronamespace from pg_proc where pronamespace
= 6981446;
oid | proname | pronamespace
-----+---------+--------------
(0 rows)

Time: 7.797 ms
ext_test=# select oid, typname, typnamespace from pg_type where typnamespace
= 6981446;
oid | typname | typnamespace
---------+--------------------+--------------
6981689 | spatial_ref_sys | 6981446
6981688 | _spatial_ref_sys | 6981446
6981995 | geography_columns | 6981446
6981994 | _geography_columns | 6981446
6982099 | geometry_columns | 6981446
6982098 | _geometry_columns | 6981446
6982541 | raster_columns | 6981446
6982540 | _raster_columns | 6981446
6982550 | raster_overviews | 6981446
6982549 | _raster_overviews | 6981446
(10 rows)

Time: 7.844 ms
ext_test=# select oid, conname, connamespace from pg_constraint where
connamespace = 6981446;
oid | conname | connamespace
---------+----------------------------+--------------
6981690 | spatial_ref_sys_srid_check | 6981446
6981695 | spatial_ref_sys_pkey | 6981446
(2 rows)

Time: 0.201 ms

ext_test=# DROP EXTENSION postgis ;
DROP EXTENSION
Time: 214.645 ms
ext_test=# select oid, relname, relnamespace from pg_class where
relnamespace = 6981446;

oid | relname | relnamespace
-----+---------+--------------
(0 rows)

Time: 49.484 ms
ext_test=# select oid, proname, pronamespace from pg_proc where pronamespace
= 6981446;
oid | proname | pronamespace
-----+---------+--------------
(0 rows)

Time: 7.698 ms
ext_test=# select oid, typname, typnamespace from pg_type where typnamespace
= 6981446;
oid | typname | typnamespace
-----+---------+--------------
(0 rows)

Time: 7.864 ms
ext_test=# select oid, conname, connamespace from pg_constraint where
connamespace = 6981446;
oid | conname | connamespace
-----+---------+--------------
(0 rows)

Time: 0.144 ms
ext_test=#
ext_test=# \q

jeff(at)pgexperts(dot)com writes:
> DROP and CREATE extension appear to work fine, but if you ALTER EXTENSION
> postgis SET SCHEMA foo, it leaves a few relations behind.

What it seems to be leaving behind is indexes ... also relation rowtypes.

A bit of looking shows that ALTER EXTENSION SET SCHEMA calls
AlterObjectNamespace_oid on the table. AlterObjectNamespace_oid
calls AlterRelationNamespaceInternal, and nothing else. In comparison,
ALTER TABLE SET SCHEMA (AlterTableNamespace) calls
AlterRelationNamespaceInternal and about four other things. I'm not
sure if this was broken before the last round of refactoring in this
area, but for sure it's broken now.

regards, tom lane

On Jun 22, 2012, at 7:37 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> jeff(at)pgexperts(dot)com writes:
>> DROP and CREATE extension appear to work fine, but if you ALTER EXTENSION
>> postgis SET SCHEMA foo, it leaves a few relations behind.
>
> What it seems to be leaving behind is indexes ... also relation rowtypes.
>
> A bit of looking shows that ALTER EXTENSION SET SCHEMA calls
> AlterObjectNamespace_oid on the table. AlterObjectNamespace_oid
> calls AlterRelationNamespaceInternal, and nothing else. In comparison,
> ALTER TABLE SET SCHEMA (AlterTableNamespace) calls
> AlterRelationNamespaceInternal and about four other things. I'm not
> sure if this was broken before the last round of refactoring in this
> area, but for sure it's broken now.
>
> regards, tom lane

Forgot to mention: initially saw this on 9.1.2 and tested 9.1.4 to see if it was resolved, but both exhibit same behavior.

On Fri, Jun 22, 2012 at 10:37:10PM -0400, Tom Lane wrote:
> jeff(at)pgexperts(dot)com writes:
> > DROP and CREATE extension appear to work fine, but if you ALTER EXTENSION
> > postgis SET SCHEMA foo, it leaves a few relations behind.
>
> What it seems to be leaving behind is indexes ... also relation rowtypes.
>
> A bit of looking shows that ALTER EXTENSION SET SCHEMA calls
> AlterObjectNamespace_oid on the table. AlterObjectNamespace_oid
> calls AlterRelationNamespaceInternal, and nothing else. In comparison,
> ALTER TABLE SET SCHEMA (AlterTableNamespace) calls
> AlterRelationNamespaceInternal and about four other things. I'm not
> sure if this was broken before the last round of refactoring in this
> area, but for sure it's broken now.

Uh, did this get fixed? I can't find a commit related to the fix.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> On Fri, Jun 22, 2012 at 10:37:10PM -0400, Tom Lane wrote:
>> jeff(at)pgexperts(dot)com writes:
>>> DROP and CREATE extension appear to work fine, but if you ALTER EXTENSION
>>> postgis SET SCHEMA foo, it leaves a few relations behind.

>> What it seems to be leaving behind is indexes ... also relation rowtypes.

> Uh, did this get fixed? I can't find a commit related to the fix.

No, it's still an open issue.

regards, tom lane

Excerpts from Tom Lane's message of jue ago 30 23:19:12 -0400 2012:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > On Fri, Jun 22, 2012 at 10:37:10PM -0400, Tom Lane wrote:
> >> jeff(at)pgexperts(dot)com writes:
> >>> DROP and CREATE extension appear to work fine, but if you ALTER EXTENSION
> >>> postgis SET SCHEMA foo, it leaves a few relations behind.
>
> >> What it seems to be leaving behind is indexes ... also relation rowtypes.
>
> > Uh, did this get fixed? I can't find a commit related to the fix.
>
> No, it's still an open issue.

Hmm, I think this might be my bug. I will have a look at this; not sure
if I'll be able to do it tomorrow, though (and certainly not during the
weekend).

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Excerpts from Tom Lane's message of vie jun 22 22:37:10 -0400 2012:
> jeff(at)pgexperts(dot)com writes:
> > DROP and CREATE extension appear to work fine, but if you ALTER EXTENSION
> > postgis SET SCHEMA foo, it leaves a few relations behind.
>
> What it seems to be leaving behind is indexes ... also relation rowtypes.
>
> A bit of looking shows that ALTER EXTENSION SET SCHEMA calls
> AlterObjectNamespace_oid on the table. AlterObjectNamespace_oid
> calls AlterRelationNamespaceInternal, and nothing else. In comparison,
> ALTER TABLE SET SCHEMA (AlterTableNamespace) calls
> AlterRelationNamespaceInternal and about four other things. I'm not
> sure if this was broken before the last round of refactoring in this
> area, but for sure it's broken now.

Aha, I see the bug. It seems the split for AlterObjectNamespace_oid
related to tables was done at the wrong level: there should be a new
AlterTableNamespace_internal call that does all the extra stuff, and
which is to be called from AlterObjectNamespace_oid.

FWIW this bug seems to have been introduced in the initial extensions
commit, d9572c4e3b474031060189050e14ef384b94e001. Prior to that we had
ExecAlterObjectSchemaStmt calling AlterTableNamespace directly.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> Aha, I see the bug. It seems the split for AlterObjectNamespace_oid
> related to tables was done at the wrong level: there should be a new
> AlterTableNamespace_internal call that does all the extra stuff, and
> which is to be called from AlterObjectNamespace_oid.

Sounds reasonable to me. Are you going to fix it, or should I? If
it was introduced in the extensions patch then it's my fault ...

regards, tom lane

Excerpts from Tom Lane's message of vie ago 31 12:17:59 -0400 2012:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> > Aha, I see the bug. It seems the split for AlterObjectNamespace_oid
> > related to tables was done at the wrong level: there should be a new
> > AlterTableNamespace_internal call that does all the extra stuff, and
> > which is to be called from AlterObjectNamespace_oid.
>
> Sounds reasonable to me. Are you going to fix it, or should I? If
> it was introduced in the extensions patch then it's my fault ...

I'm looking into it.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Excerpts from Alvaro Herrera's message of vie ago 31 12:26:50 -0400 2012:
> Excerpts from Tom Lane's message of vie ago 31 12:17:59 -0400 2012:
> > Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> > > Aha, I see the bug. It seems the split for AlterObjectNamespace_oid
> > > related to tables was done at the wrong level: there should be a new
> > > AlterTableNamespace_internal call that does all the extra stuff, and
> > > which is to be called from AlterObjectNamespace_oid.
> >
> > Sounds reasonable to me. Are you going to fix it, or should I? If
> > it was introduced in the extensions patch then it's my fault ...
>
> I'm looking into it.

Here's a patch. Note I'm not attempting to create a regression test
because that seems to involve setting up a fake extension which I don't
know how to do (would be too messy, I think). So I tested it by having
isn--1.0.sql create a table with a primary key: create the extension,
alter it to move to a new schema, drop the original schema (public). If
I then try to dump the database, pg_dump in current HEAD fails with "no
schema with OID 2200" (not verbatim; OID is for the old public schema).
It seems to me that this is exactly what was reported initially. Also I
verified that psql's \d reports the inconsistency of the table and its
PK.

Patched code works fine.

For some reason, AlterSeqNamespaces was being passed a schema name.
This wasn't used and was not possible to keep in the patched code so I
just removed it.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> Here's a patch.

Looks reasonable, but please try a little harder on the comments for the
new function --- IMO it should have a header comment that explains what
it's supposed to do exactly.

> For some reason, AlterSeqNamespaces was being passed a schema name.
> This wasn't used and was not possible to keep in the patched code so I
> just removed it.

Probably a hangover from some previous state of the code. If it's not
used I see no reason not to remove it.

regards, tom lane

Excerpts from Tom Lane's message of vie ago 31 16:01:03 -0400 2012:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> > Here's a patch.
>
> Looks reasonable, but please try a little harder on the comments for the
> new function --- IMO it should have a header comment that explains what
> it's supposed to do exactly.

Sure.

This doesn't actually work though: it's trying to move sequences twice.
Not sure what the right fix for this is ... still looking.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Excerpts from Alvaro Herrera's message of vie ago 31 16:25:40 -0400 2012:
> Excerpts from Tom Lane's message of vie ago 31 16:01:03 -0400 2012:
> > Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> > > Here's a patch.
> >
> > Looks reasonable, but please try a little harder on the comments for the
> > new function --- IMO it should have a header comment that explains what
> > it's supposed to do exactly.
>
> Sure.
>
> This doesn't actually work though: it's trying to move sequences twice.
> Not sure what the right fix for this is ... still looking.

The error message:

alvherre=# alter extension isn set schema foo;
ERROR: tuple already updated by self

Besides being listed with deptype=extension for the extension in
question, the sequence has a deptype=auto entry for the column, which
leads to it being moved twice.

I don't see any clean fix for this:

1. At extension creation time, skip generating dependencies for
sequences that are part of a table. I don't see how to do this: We
would have to reach within heap_create_with_catalog and tell it not to
add an deptype=extension dependency if it's called for a sequence that's
part of a serial column. I don't think heap_create_with_catalog even
knows that at all.

2. During ALTER EXTENSION execution, skip moving objects that have
already been moved. Not really sure how this would be implemented;
perhaps we could have AlterObjectNamespace_oid add each moved object to
a list of moved objects, and skip everything that's already present in
it. Seems very messy to implement.

3. Pass a flag to AlterTableNamespaceInternal, something like
"skip_sequences", and do not call AlterSeqsNamespace it that's set.
This seems really ugly though, and I'm not sure if it might break
something else.

4. Maybe we could have AlterRelationNamespaceInternal check what the
current namespace is for the object, and do nothing if it's already the
target namespace.

One thing to keep in mind is that existing databases might already have
broken catalogs (i.e. extensions have already been moved and objects are
already in nonexistant schemas). This is not very likely unless the
user has not been using pg_dump at all. But many databases already have
deptype=extension pg_depend entries for sequences owned by SERIAL
columns, so it's unclear that (1) is a good solution even if it were
implementable.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> Excerpts from Alvaro Herrera's message of vie ago 31 16:25:40 -0400 2012:
>> This doesn't actually work though: it's trying to move sequences twice.
>> Not sure what the right fix for this is ... still looking.

> Besides being listed with deptype=extension for the extension in
> question, the sequence has a deptype=auto entry for the column, which
> leads to it being moved twice.

Ah so.

> 2. During ALTER EXTENSION execution, skip moving objects that have
> already been moved. Not really sure how this would be implemented;

+1 for this approach. I'm a bit surprised we didn't hit this before,
because in general there can be multiple dependency chains leading from
object A to object B. Most code that is doing more than trivial
dependency-walking has to be prepared to cope with reaching the same
object multiple times.

Implementation like this seems reasonable:

> 4. Maybe we could have AlterRelationNamespaceInternal check what the
> current namespace is for the object, and do nothing if it's already the
> target namespace.

We already have some such shortcut for ALTER OWNER, IIRC, so why not
for SET SCHEMA as well? I suspect that AlterRelationNamespaceInternal
is not the only function that needs it, too.

regards, tom lane

Excerpts from Tom Lane's message of vie ago 31 17:50:41 -0400 2012:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:

> > 2. During ALTER EXTENSION execution, skip moving objects that have
> > already been moved. Not really sure how this would be implemented;
>
> +1 for this approach. I'm a bit surprised we didn't hit this before,
> because in general there can be multiple dependency chains leading from
> object A to object B. Most code that is doing more than trivial
> dependency-walking has to be prepared to cope with reaching the same
> object multiple times.
>
> Implementation like this seems reasonable:
>
> > 4. Maybe we could have AlterRelationNamespaceInternal check what the
> > current namespace is for the object, and do nothing if it's already the
> > target namespace.
>
> We already have some such shortcut for ALTER OWNER, IIRC, so why not
> for SET SCHEMA as well? I suspect that AlterRelationNamespaceInternal
> is not the only function that needs it, too.

It doesn't work :-( The problem is that the outer sysscan in
extension.c gets to the table first, recurses there and updates the
sequence pg_depend tuple; then it gets out and the outer scan gets to
the sequence directly. But this fails to notice that it has already
been updated, because we haven't done a CommandCounterIncrement.
However, if I add one, we get into Halloween problem because the
sequence is updated, command counter incremented, and the outer scan
sees the updated tuple (because it's using SnapshotNow) for the table so
it recurses again, and instead of "tuple updated by self" we get this:

alvherre=# alter extension isn set schema baz;
ERROR: relation "test_b_seq" already exists in schema "baz"

So I think my other proposal is the way to fix the problem: each
AlterFooNamespace routine must update an ObjectAddresses array of
relocated objects, and the outer scan (extension.c) must skip objects
that are already in that list.

I have tested this theory (attached patch) and it solves the problem at
hand. The patch is not complete: I haven't updated all the
AlterFooNamespace routines, only those necessary to fix this problem.
If we agree that this is the way to go I can complete and push.

Putting this kind of change this late in the 9.2 release process makes
me a bit nervous, but I don't see a simpler way to solve the problem.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> Putting this kind of change this late in the 9.2 release process makes
> me a bit nervous, but I don't see a simpler way to solve the problem.

This is a pre-existing bug, not something new in 9.2, and quite honestly
I don't think we should try to fix it under time pressure if there is
any doubt about how to do so. I don't like the proposed patch very
much either, so let's think about it some more.

regards, tom lane

Hi,

Sorry for being late at the party… been distracted away…

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> On Fri, Jun 22, 2012 at 10:37:10PM -0400, Tom Lane wrote:
>> jeff(at)pgexperts(dot)com writes:
>> > DROP and CREATE extension appear to work fine, but if you ALTER EXTENSION
>> > postgis SET SCHEMA foo, it leaves a few relations behind.
>>
>> What it seems to be leaving behind is indexes ... also relation rowtypes.
>>
>> A bit of looking shows that ALTER EXTENSION SET SCHEMA calls
>> AlterObjectNamespace_oid on the table. AlterObjectNamespace_oid
>> calls AlterRelationNamespaceInternal, and nothing else. In comparison,
>> ALTER TABLE SET SCHEMA (AlterTableNamespace) calls
>> AlterRelationNamespaceInternal and about four other things. I'm not
>> sure if this was broken before the last round of refactoring in this
>> area, but for sure it's broken now.

Looking at that code, my theory of how we got there is that in the
submitted extension patch I did only use DEPENDENCY_INTERNAL and Tom
introduced the much better DEPENDENCY_EXTENSION tracking. With the
former, indexes and sequences and constraints where found in the
dependency walking code, but only the main relation is now registered in
the later model.

I need to do some testing about dependency tracking on SERIAL generated
sequences compared to manually created sequences in extension scripts, I
think we track sequences directly only in the manual case.

I think we need to share more code in between
AlterRelationNamespaceInternal and AlterTableNamespace, but I'm not sure
if that's not exactly what Álvaro did try with his _oid() attempt that
failed.

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

Excerpts from Dimitri Fontaine's message of mié sep 12 06:51:43 -0300 2012:
> Hi,
>
> Sorry for being late at the party… been distracted away…

Welcome ;-)

> > On Fri, Jun 22, 2012 at 10:37:10PM -0400, Tom Lane wrote:

> >> A bit of looking shows that ALTER EXTENSION SET SCHEMA calls
> >> AlterObjectNamespace_oid on the table. AlterObjectNamespace_oid
> >> calls AlterRelationNamespaceInternal, and nothing else. In comparison,
> >> ALTER TABLE SET SCHEMA (AlterTableNamespace) calls
> >> AlterRelationNamespaceInternal and about four other things. I'm not
> >> sure if this was broken before the last round of refactoring in this
> >> area, but for sure it's broken now.
>
> Looking at that code, my theory of how we got there is that in the
> submitted extension patch I did only use DEPENDENCY_INTERNAL and Tom
> introduced the much better DEPENDENCY_EXTENSION tracking. With the
> former, indexes and sequences and constraints where found in the
> dependency walking code, but only the main relation is now registered in
> the later model.
>
> I need to do some testing about dependency tracking on SERIAL generated
> sequences compared to manually created sequences in extension scripts, I
> think we track sequences directly only in the manual case.

Well, what I saw was that both the table and its SERIAL-generated
sequence got an DEPENDENCY_EXTENSION row in pg_depend, which is exactly
what (IMV) causes the problem. One of my proposals is to tweak the code
to avoid that row (but if we do that, then we need to do something about
databases that contain such rows today).

> I think we need to share more code in between
> AlterRelationNamespaceInternal and AlterTableNamespace, but I'm not sure
> if that's not exactly what Álvaro did try with his _oid() attempt that
> failed.

What I did was create an AlterTableNamespaceInternal that goes through
all the things that must be moved (this means calling
AlterRelationNamespaceInternal for the table, and then doing some more
calls to move the sequences, indexes, constraints). With this change,
both AlterTableNamespace and AlterObjectNamespace_oid can call it.
Previously, AlterObjectNamespace_oid was calling
AlterRelationNamespaceInternal directly for the relation only.

As far as I can see, that split of AlterTableNamespace is still needed.
The problem here is that we need something *beyond* that fix. Did you
look at my patches?

Am I making sense?

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> Well, what I saw was that both the table and its SERIAL-generated
> sequence got an DEPENDENCY_EXTENSION row in pg_depend, which is exactly
> what (IMV) causes the problem. One of my proposals is to tweak the code
> to avoid that row (but if we do that, then we need to do something about
> databases that contain such rows today).

Ah yes, indeed.

I think we shouldn't change the content of pg_depend lightly here, and
that we should rather specialize AlterObjectNamespace_oid() to skip
caring about sequences. The other objects that get moved by
AlterTableNamepace other than the table itself and its sequences are
Indexes and Constraints. Owned Sequence (serial) will get cared of by
the extension dependency walking code.

I'm going to have a try at that.

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

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> I think we shouldn't change the content of pg_depend lightly here, and

So here's a patch following that idea.

Even for TIP I don't want us to change how pg_depend tracking is done,
because I want to propose a fix for the pg_dump bug wrt sequences and
pg_extension_config_dump() wherein you can actually register a sequence
(owned by a table or not) but then pg_dump fails to dump it (see report
from Marko Kreen)

http://archives.postgresql.org/message-id/CACMqXCJjauc9jPa64VxskRN67bYjuYmodz-Mgy-_aoZ6ErG3Yg@mail.gmail.com

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

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
>> I think we shouldn't change the content of pg_depend lightly here, and

> So here's a patch following that idea.

I've got to say that I think this is fundamentally the wrong approach:
rather than fixing the generic problem of ALTER EXTENSION not coping
with multiple dependency paths to the same object, it hacks the specific
case of owned sequences, and what's more it does that by assuming that
every owned sequence *will* have a dependency on the extension. That's
not a safe assumption.

Still, this might be the best approach for the back branches, given that
we do not know of any existing multiple-dependency scenarios other than
the owned-sequence case. A real fix is looking mighty invasive.

> Even for TIP I don't want us to change how pg_depend tracking is done,

Agreed. Quite aside from backwards-compatibility concerns, I think that
trying to avoid multiple dependency paths is doomed to failure.

regards, tom lane

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> Excerpts from Tom Lane's message of vie ago 31 17:50:41 -0400 2012:
>> We already have some such shortcut for ALTER OWNER, IIRC, so why not
>> for SET SCHEMA as well? I suspect that AlterRelationNamespaceInternal
>> is not the only function that needs it, too.

> It doesn't work :-( The problem is that the outer sysscan in
> extension.c gets to the table first, recurses there and updates the
> sequence pg_depend tuple; then it gets out and the outer scan gets to
> the sequence directly. But this fails to notice that it has already
> been updated, because we haven't done a CommandCounterIncrement.
> However, if I add one, we get into Halloween problem because the
> sequence is updated, command counter incremented, and the outer scan
> sees the updated tuple (because it's using SnapshotNow) for the table so
> it recurses again, and instead of "tuple updated by self" we get this:

> alvherre=# alter extension isn set schema baz;
> ERROR: relation "test_b_seq" already exists in schema "baz"

Yeah, you do get that, but the above explanation of why is incorrect.
There is nothing in the ALTER SET SCHEMA code paths that affects the
pg_depend entries that the outer loop in AlterExtensionNamespace is
looking at. Rather the problem is that you haven't covered all the
bases in preventing repeated updates, and so some cases reach a relation
that's already been moved and then this is the error you get.

I've been testing this patch with an extension having this definition
file:

-----
create table t1(f1 serial primary key, f2 text);

create table t2(f1 int primary key, f2 text);

create sequence ss2;

alter sequence ss2 owned by t2.f1;

create sequence ss3;

create table t3(f1 int primary key, f2 text);

alter sequence ss3 owned by t3.f1;
-----

so as to check both the case where the AlterExtensionNamespace loop
arrives at the owned sequence before the table, and the case where it
reaches them in the other order. The current patch handles the latter
case but not the former. To handle the first case I believe that
AlterRelationNamespaceInternal needs not only to add the target rel
to objsMoved, but also to check whether the target rel is already in
objsMoved and do nothing if so. Otherwise, if an owned sequence is
reached first in the AlterExtensionNamespace loop, it will be moved,
and then later when we move the table, it'll call AlterSeqNamespaces
and nothing below that knows to not move the sequence again.

(BTW, the test you added to see if old namespace = new namespace is
quite wrong for this, because it's looking at the non-updated tuple
for lack of any CommandCounterIncrement.)

I tried adding the missing test but it still falls over, because
AlterTypeNamespaceInternal needs the same logic. The reason is that
AlterSeqNamespaces calls both AlterRelationNamespaceInternal and
AlterTypeNamespaceInternal, and both of those have to be willing to do
nothing if the sequence was already moved. We could maybe refactor
so that AlterRelationNamespaceInternal's test covers the type case too,
but I don't think that is the way forward, because it won't cover any
non-sequence cases where a type is reached through two different
dependency paths.

So it appears to me that a real fix involves extending the check and
add logic to at least relations and types, and perhaps eventually to
everything that AlterObjectNamespace_oid can call. That's fairly
invasive, especially if we try to do the whole nine yards immediately.
But perhaps for now we need only fix the relation and type cases.

BTW, I experimented with inserting CommandCounterIncrement calls
into the AlterExtensionNamespace loop, and eventually decided that
that's probably not the best path to a solution. The killer problem is
that it messes up the logic in AlterExtensionNamespace that tries to
verify that all the objects had been in the same namespace. If the
subroutines report that the object is now in the target namespace,
is that okay or not? You can't tell.

I think that the right way to proceed is to *not* do
CommandCounterIncrement in the AlterExtensionNamespace loop, and also
*not* have a test in AlterExtensionNamespace for an object already
having been moved. Rather, since we already know we need that test down
in AlterRelationNamespaceInternal and AlterTypeNamespaceInternal, do it
only at that level. This combination of choices ensures that we'll get
back valid reports of the old namespace for each object, and so the
are-they-all-the-same logic in AlterExtensionNamespace still works.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> I've got to say that I think this is fundamentally the wrong approach:
> rather than fixing the generic problem of ALTER EXTENSION not coping
> with multiple dependency paths to the same object, it hacks the specific
> case of owned sequences, and what's more it does that by assuming that
> every owned sequence *will* have a dependency on the extension. That's
> not a safe assumption.

In general, agreed.

> Still, this might be the best approach for the back branches, given that
> we do not know of any existing multiple-dependency scenarios other than
> the owned-sequence case. A real fix is looking mighty invasive.

That's what I was aiming at, best approach for the back branches.

>> Even for TIP I don't want us to change how pg_depend tracking is done,
>
> Agreed. Quite aside from backwards-compatibility concerns, I think that
> trying to avoid multiple dependency paths is doomed to failure.

For a “DIRTT” approach to the problems, I think Álvaro's work is in the
right direction, and should be pursued without trying to address the
back branches too. I don't remember now if his tracking attempt was also
trying to change pg_depend entries, I think that was in two separate
patches versions.

DIRTT: Do It Right This Time

Álvaro, do you want to be working on a master only version of the fix
or do you want me to?

Regards,
--
Dimitri Fontaine 06 63 07 10 78
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Excerpts from Dimitri Fontaine's message of lun sep 24 05:26:52 -0300 2012:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> > Agreed. Quite aside from backwards-compatibility concerns, I think that
> > trying to avoid multiple dependency paths is doomed to failure.
>
> For a “DIRTT” approach to the problems, I think Álvaro's work is in the
> right direction, and should be pursued without trying to address the
> back branches too. I don't remember now if his tracking attempt was also
> trying to change pg_depend entries, I think that was in two separate
> patches versions.

Yes, those were different patches.

> Álvaro, do you want to be working on a master only version of the fix
> or do you want me to?

Please go ahead :-)

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> I've been testing this patch with an extension having this definition
> file:

Side note: as soon as we have CREATE EXTENSION AS $$ script $$; we will
be able to add those cases as regression tests. That's not the main
usage of that feature, by far, but I can't resits the occasion :)

> -----
> create table t1(f1 serial primary key, f2 text);
>
> create table t2(f1 int primary key, f2 text);
>
> create sequence ss2;
>
> alter sequence ss2 owned by t2.f1;
>
> create sequence ss3;
>
> create table t3(f1 int primary key, f2 text);
>
> alter sequence ss3 owned by t3.f1;
> -----

This exact same script pass with the attached patch, a patch on top
Álvaro's version:

dim=# create extension extseq;
CREATE EXTENSION
dim=# create schema foo;
CREATE SCHEMA
dim=# alter extension extseq set schema foo;
ALTER EXTENSION
dim=# \dx+ extseq
Objects in extension "extseq"
Object Description
------------------------
sequence foo.ss2
sequence foo.ss3
sequence foo.t1_f1_seq
table foo.t1
table foo.t2
table foo.t3
(6 rows)

I have some local failures in `make check` that I'm not sure originate
from that patch. Still wanted to have an opinion about the idea before
cleaning up.

> nothing if the sequence was already moved. We could maybe refactor
> so that AlterRelationNamespaceInternal's test covers the type case too,
> but I don't think that is the way forward, because it won't cover any
> non-sequence cases where a type is reached through two different
> dependency paths.

I tried to care about that in the attached. Spent so much time rolling
it in my head in every possible angle that I really need another pair of
eyes on it though.

> So it appears to me that a real fix involves extending the check and
> add logic to at least relations and types, and perhaps eventually to
> everything that AlterObjectNamespace_oid can call. That's fairly
> invasive, especially if we try to do the whole nine yards immediately.
> But perhaps for now we need only fix the relation and type cases.

I think INDEX and CONSTRAINTS (the only other things that can be called
from that point) are safe because there's no explicit support for them
in the AlterObjectNamespace_oid() function.

> BTW, I experimented with inserting CommandCounterIncrement calls
> into the AlterExtensionNamespace loop, and eventually decided that
> that's probably not the best path to a solution. The killer problem is
> that it messes up the logic in AlterExtensionNamespace that tries to
> verify that all the objects had been in the same namespace. If the
> subroutines report that the object is now in the target namespace,
> is that okay or not? You can't tell.

Agreed.

> I think that the right way to proceed is to *not* do
> CommandCounterIncrement in the AlterExtensionNamespace loop, and also
> *not* have a test in AlterExtensionNamespace for an object already
> having been moved. Rather, since we already know we need that test down
> in AlterRelationNamespaceInternal and AlterTypeNamespaceInternal, do it
> only at that level. This combination of choices ensures that we'll get
> back valid reports of the old namespace for each object, and so the
> are-they-all-the-same logic in AlterExtensionNamespace still works.

See attached.

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

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> Side note: as soon as we have CREATE EXTENSION AS $$ script $$; we will
> be able to add those cases as regression tests. That's not the main
> usage of that feature, by far, but I can't resits the occasion :)

Oh, I did already mention it :)

> I have some local failures in `make check` that I'm not sure originate
> from that patch. Still wanted to have an opinion about the idea before
> cleaning up.

Sorry for sending unfinished preliminary version, I just had the
opportunity to look at what happened: views will create a composite type
that needs its pg_class row updated when doing ALTER VIEW SET SCHEMA.

That means that we need proper tracking for that operation even when it
happens outside of an extension update script, as in the attached
version 4 of the patch.

I think the way forward is to use the simplest one for back branches and
this one for master only, unless it is appreciated of light enough
impact, right? (provided it's ok, too)

git diff --stat
src/backend/commands/alter.c | 14 +----
src/backend/commands/extension.c | 48 +++++++++------
src/backend/commands/tablecmds.c | 122 +++++++++++++++++++++++++++-----------
src/backend/commands/typecmds.c | 33 +++++++++-
src/include/commands/alter.h | 4 +-
src/include/commands/tablecmds.h | 7 ++-
src/include/commands/typecmds.h | 6 +-
7 files changed, 161 insertions(+), 73 deletions(-)

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

Here's a cleaned up version of this patch, for HEAD. (The patches for
9.1 and 9.2 required minor conflict fixes, but nothing substantial).

The main difference from Dimitri's patch is that I added enough support
code so that AlterRelationNamespaceInternal() is always getting a valid
ObjectAddresses list, and get rid of the checks for a NULL one. It
seems cleaner and simpler to me this way. This means I had to add
support for object types that may never be visited more than once
(indexes), but I don't think this is a problem.

Remaining object types are those that use the generic code path in HEAD
(as patched by KaiGai). We know (because the generic code path checks)
that in these cases, relations are never involved; the only other funny
cases I saw were collations and functions, but those are funny only
because the lookups are unlike the normal cases; they don't have any
subsidiary objects that would cause trouble.

While I am looking at this code: I am uneasy about
AlterObjectNamespace_oid() ignoring object classes that it doesn't
explicitely know about. This means we will fail to cover new cases we
might come up with. For example, can we have event triggers in
extensions, and do event triggers belong to a schema? If so, we already
have a bug here in HEAD. I would like us to get rid of the "default:
break;" case, and instead explicitely list the object classes we ignore.
That way, the compiler will warn us as soon as we add a new object
class and neglect to add it here.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera wrote:
> Here's a cleaned up version of this patch, for HEAD. (The patches for
> 9.1 and 9.2 required minor conflict fixes, but nothing substantial).

Committed.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services