Thread: BUG #6704: ALTER EXTENSION postgis SET SCHEMA leaves dangling relations
BUG #6704: ALTER EXTENSION postgis SET SCHEMA leaves dangling relations
From
jeff@pgexperts.com
Date:
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:=20=20=20=20=20=20=20=20 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=3D# create schema test; CREATE SCHEMA Time: 27.736 ms ext_test=3D# create EXTENSION postgis with schema test; CREATE EXTENSION Time: 764.102 ms ext_test=3D# alter EXTENSION postgis set schema public; ALTER EXTENSION Time: 221.224 ms ext_test=3D# select oid, nspname from pg_namespace ; oid | nspname=20=20=20=20=20=20=20 ---------+-------------------- 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=3D# select oid, relname, relnamespace from pg_class where relnamespace =3D 6981446; oid | relname | relnamespace=20 ---------+----------------------+-------------- 6981694 | spatial_ref_sys_pkey | 6981446 (1 row) Time: 36.072 ms ext_test=3D# select oid, proname, pronamespace from pg_proc where pronamesp= ace =3D 6981446; oid | proname | pronamespace=20 -----+---------+-------------- (0 rows) Time: 7.797 ms ext_test=3D# select oid, typname, typnamespace from pg_type where typnamesp= ace =3D 6981446; oid | typname | typnamespace=20 ---------+--------------------+-------------- 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=3D# select oid, conname, connamespace from pg_constraint where connamespace =3D 6981446; oid | conname | connamespace=20 ---------+----------------------------+-------------- 6981690 | spatial_ref_sys_srid_check | 6981446 6981695 | spatial_ref_sys_pkey | 6981446 (2 rows) Time: 0.201 ms ext_test=3D# DROP EXTENSION postgis ; DROP EXTENSION Time: 214.645 ms ext_test=3D# select oid, relname, relnamespace from pg_class where relnamespace =3D 6981446; oid | relname | relnamespace=20 -----+---------+-------------- (0 rows) Time: 49.484 ms ext_test=3D# select oid, proname, pronamespace from pg_proc where pronamesp= ace =3D 6981446; oid | proname | pronamespace=20 -----+---------+-------------- (0 rows) Time: 7.698 ms ext_test=3D# select oid, typname, typnamespace from pg_type where typnamesp= ace =3D 6981446; oid | typname | typnamespace=20 -----+---------+-------------- (0 rows) Time: 7.864 ms ext_test=3D# select oid, conname, connamespace from pg_constraint where connamespace =3D 6981446; oid | conname | connamespace=20 -----+---------+-------------- (0 rows) Time: 0.144 ms ext_test=3D#=20 ext_test=3D# \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. >=20 > What it seems to be leaving behind is indexes ... also relation rowtypes. >=20 > 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. >=20 > regards, tom lane Forgot to mention: initially saw this on 9.1.2 and tested 9.1.4 to see if i= t was resolved, but both exhibit same behavior.=20
Re: BUG #6704: ALTER EXTENSION postgis SET SCHEMA leaves dangling relations
From
Bruce Momjian
Date:
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
Re: BUG #6704: ALTER EXTENSION postgis SET SCHEMA leaves dangling relations
From
Alvaro Herrera
Date:
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 EXT= ENSION > >>> postgis SET SCHEMA foo, it leaves a few relations behind. >=20 > >> What it seems to be leaving behind is indexes ... also relation rowt= ypes. >=20 > > Uh, did this get fixed? I can't find a commit related to the fix. >=20 > 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). --=20 =C3=81lvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: BUG #6704: ALTER EXTENSION postgis SET SCHEMA leaves dangling relations
From
Alvaro Herrera
Date:
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 EXTEN= SION > > postgis SET SCHEMA foo, it leaves a few relations behind. >=20 > What it seems to be leaving behind is indexes ... also relation rowtype= s. >=20 > 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. --=20 =C3=81lvaro 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
Re: BUG #6704: ALTER EXTENSION postgis SET SCHEMA leaves dangling relations
From
Alvaro Herrera
Date:
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. >=20 > 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. --=20 =C3=81lvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: BUG #6704: ALTER EXTENSION postgis SET SCHEMA leaves dangling relations
From
Alvaro Herrera
Date:
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
Attachment
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
Re: BUG #6704: ALTER EXTENSION postgis SET SCHEMA leaves dangling relations
From
Alvaro Herrera
Date:
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. >=20 > Looks reasonable, but please try a little harder on the comments for th= e > 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. --=20 =C3=81lvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: BUG #6704: ALTER EXTENSION postgis SET SCHEMA leaves dangling relations
From
Alvaro Herrera
Date:
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. > >=20 > > Looks reasonable, but please try a little harder on the comments for = the > > new function --- IMO it should have a header comment that explains wh= at > > it's supposed to do exactly. >=20 > Sure. >=20 > 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=3D# alter extension isn set schema foo; ERROR: tuple already updated by self Besides being listed with deptype=3Dextension for the extension in question, the sequence has a deptype=3Dauto 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=3Dextension 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=3Dextension pg_depend entries for sequences owned by SERIAL columns, so it's unclear that (1) is a good solution even if it were implementable. --=20 =C3=81lvaro 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
Re: BUG #6704: ALTER EXTENSION postgis SET SCHEMA leaves dangling relations
From
Alvaro Herrera
Date:
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
Attachment
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
Re: BUG #6704: ALTER EXTENSION postgis SET SCHEMA leaves dangling relations
From
Dimitri Fontaine
Date:
Hi, Sorry for being late at the party=E2=80=A6 been distracted away=E2=80=A6 =20=20=20=20=20=20=20 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 EXTENS= ION >> > postgis SET SCHEMA foo, it leaves a few relations behind. >>=20 >> What it seems to be leaving behind is indexes ... also relation rowtypes. >>=20 >> 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 =C3=81lvaro did try with his _oid() attempt that failed. Regards, --=20 Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Re: BUG #6704: ALTER EXTENSION postgis SET SCHEMA leaves dangling relations
From
Alvaro Herrera
Date:
Excerpts from Dimitri Fontaine's message of mi=C3=A9 sep 12 06:51:43 -030= 0 2012: > Hi, >=20 > Sorry for being late at the party=E2=80=A6 been distracted away=E2=80=A6 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 comparis= on, > >> 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. >=20 > 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 i= n > the later model. >=20 > 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 sur= e > if that's not exactly what =C3=81lvaro 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? --=20 =C3=81lvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: BUG #6704: ALTER EXTENSION postgis SET SCHEMA leaves dangling relations
From
Dimitri Fontaine
Date:
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
Re: BUG #6704: ALTER EXTENSION postgis SET SCHEMA leaves dangling relations
From
Dimitri Fontaine
Date:
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) http://archives.postgresql.org/message-id/CACMqXCJjauc9jPa64VxskRN67bYjuYmodz-Mgy-_aoZ6ErG3Yg@mail.gmail.com Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Attachment
Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes: > 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. 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@2ndquadrant.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
Re: BUG #6704: ALTER EXTENSION postgis SET SCHEMA leaves dangling relations
From
Dimitri Fontaine
Date:
Tom Lane <tgl@sss.pgh.pa.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 =E2=80=9CDIRTT=E2=80=9D approach to the problems, I think =C3=81lvaro= '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 =C3=81lvaro, do you want to be working on a master only version of the fix or do you want me to? Regards, --=20 Dimitri Fontaine 06 63 07 10 78 http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Re: BUG #6704: ALTER EXTENSION postgis SET SCHEMA leaves dangling relations
From
Alvaro Herrera
Date:
Excerpts from Dimitri Fontaine's message of lun sep 24 05:26:52 -0300 201= 2: > Tom Lane <tgl@sss.pgh.pa.us> writes: > > Agreed. Quite aside from backwards-compatibility concerns, I think t= hat > > trying to avoid multiple dependency paths is doomed to failure. >=20 > For a =E2=80=9CDIRTT=E2=80=9D approach to the problems, I think =C3=81l= varo'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 als= o > trying to change pg_depend entries, I think that was in two separate > patches versions. Yes, those were different patches. > =C3=81lvaro, do you want to be working on a master only version of the = fix > or do you want me to? Please go ahead :-) --=20 =C3=81lvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: BUG #6704: ALTER EXTENSION postgis SET SCHEMA leaves dangling relations
From
Dimitri Fontaine
Date:
Tom Lane <tgl@sss.pgh.pa.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
Attachment
Re: BUG #6704: ALTER EXTENSION postgis SET SCHEMA leaves dangling relations
From
Dimitri Fontaine
Date:
Dimitri Fontaine <dimitri@2ndQuadrant.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
Attachment
Re: BUG #6704: ALTER EXTENSION postgis SET SCHEMA leaves dangling relations
From
Alvaro Herrera
Date:
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
Attachment
Re: BUG #6704: ALTER EXTENSION postgis SET SCHEMA leaves dangling relations
From
Alvaro Herrera
Date:
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. --=20 =C1lvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services