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

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