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

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