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@pgexperts.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@pgexperts.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@sss.pgh.pa.us> wrote:
jeff@pgexperts.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@pgexperts.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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
Bruce Momjian <bruce@momjian.us> writes:
On Fri, Jun 22, 2012 at 10:37:10PM -0400, Tom Lane wrote:
jeff@pgexperts.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@momjian.us> writes:
On Fri, Jun 22, 2012 at 10:37:10PM -0400, Tom Lane wrote:
jeff@pgexperts.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@pgexperts.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@2ndquadrant.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@2ndquadrant.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@2ndquadrant.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
Attachments:
alter-extension-schema.patchapplication/octet-stream; name=alter-extension-schema.patchDownload+37-28
Alvaro Herrera <alvherre@2ndquadrant.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@2ndquadrant.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@2ndquadrant.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@2ndquadrant.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@2ndquadrant.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
Attachments:
alter-extension-schema-2.patchapplication/octet-stream; name=alter-extension-schema-2.patchDownload+163-144
Alvaro Herrera <alvherre@2ndquadrant.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@momjian.us> writes:
On Fri, Jun 22, 2012 at 10:37:10PM -0400, Tom Lane wrote:
jeff@pgexperts.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@2ndquadrant.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@2ndQuadrant.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)
Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support