Re: [HACKERS] BUG #5856: pg_attribute.attinhcount is not correct.
On Sun, Apr 10, 2011 at 5:23 AM, Noah Misch n...@leadboat.com wrote: On Sun, Apr 10, 2011 at 07:35:53AM -0400, Robert Haas wrote: On Sun, Apr 10, 2011 at 6:36 AM, Noah Misch n...@leadboat.com wrote: 3. Make AlterTableCreateToastTable acquire only ShareUpdateExclusiveLock and remove the pass-usage heuristic from ATRewriteCatalogs. For this to be valid, toast_insert_or_update() must behave reasonably in the face of a relation concurrently acquiring a TOAST table. Since it takes reltoastrelid from the relcache, toast_insert_or_update() will not act on the change in the middle of a single call. Even if it did, I don't see any risks. I'd lean toward #3 if someone else is also confident in its correctness. Otherwise, #1 seems like the way to go. Preferences? Other ideas? I haven't scrutinized the code but I would prefer #3 if it's viable without too much of a code footprint. It's certainly compact; patch attached. Thanks. Committed. It occurred to me to worry that it would be quite unsafe if a TOAST table got *removed* while holding less than AccessExclusiveLock, but it appears we're safe enough from that; I believe it can only happen on a table rewrite, and there's not much chance of that ever requiring a lesser lock strength. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BUG #5856: pg_attribute.attinhcount is not correct.
On Sun, Apr 03, 2011 at 09:53:57PM -0400, Robert Haas wrote: On Fri, Apr 1, 2011 at 12:56 AM, Noah Misch n...@leadboat.com wrote: On Thu, Mar 31, 2011 at 11:11:49AM -0400, Robert Haas wrote: On Thu, Mar 31, 2011 at 6:06 AM, Noah Misch n...@leadboat.com wrote: The best way I can see is to make ATExecAddColumn more like ATExecDropColumn, ATAddCheckConstraint, and ATExecDropConstraint. ?Namely, recurse at Exec-time rather than Prep-time, and cease recursing when we satisfy the ADD COLUMN with a merge. ?Did you have something else in mind? I had exactly what you just said in mind. Patch attached, then. Committed. Thanks. This turns out to have caused that TOAST creation regression: On Fri, Apr 08, 2011 at 08:12:19PM -0400, Noah Misch wrote: [pg_upgrade/typed table business] I also tested a regular dump+reload of the regression database, and a pg_upgrade of the same. The latter failed further along, due (indirectly) to this failure to create a TOAST table: create table p (); create table ch () inherits (p); alter table p add column a text; select oid::regclass,reltoastrelid from pg_class where oid::regclass IN ('p','ch'); insert into ch values (repeat('x', 100)); If I drop table a_star cascade in the regression database before attempting pg_upgrade, it completes cleanly. Since ATExecAddColumn now handles the recursion, child table work queue entries no longer have AT_PASS_ADD_COL subcommands. Consequently, this heuristic in ATRewriteCatalogs skips over them: if (tab-relkind == RELKIND_RELATION (tab-subcmds[AT_PASS_ADD_COL] || tab-subcmds[AT_PASS_ALTER_TYPE] || tab-subcmds[AT_PASS_COL_ATTRS])) AlterTableCreateToastTable(tab-relid, (Datum) 0); SET STORAGE uses AT_PASS_MISC, so this test case also omits a TOAST table: set client_min_messages = debug1; -- display toast creation create table t (a text); -- makes toast alter table t alter a set storage plain; alter table t add b int default 0; -- rewrite the table - no toast alter table t alter a set storage extended; -- no toast added? insert into t (a) values (repeat('x', 100)); -- fails My first thought was to figure that the cost of needs_toast_table() is not a concern and simply remove the pass-usage heuristic. However, we don't want AlterTableCreateToastTable acquiring an AccessExclusiveLock for ALTER TABLE recipes that only acquired ShareUpdateExclusiveLock. I see these options: 1. Upgrade AT_SetStorage to take AccessExclusiveLock. Add a maybe_create_toast field to AlteredTableInfo. Have the AT_SetStorage, AT_AlterColumnType and AT_AddColumn implementations set it. 2. Upgrade AT_SetStorage to take AccessExclusiveLock. In ATRewriteCatalogs, replace the pass-usage heuristic with a test for locklevel == AccessExclusiveLock. This smells more like a hack, but it might be less vulnerable to omissions. On the other hand, the set of operations that could add TOAST tables are not particularly liable to grow. 3. Make AlterTableCreateToastTable acquire only ShareUpdateExclusiveLock and remove the pass-usage heuristic from ATRewriteCatalogs. For this to be valid, toast_insert_or_update() must behave reasonably in the face of a relation concurrently acquiring a TOAST table. Since it takes reltoastrelid from the relcache, toast_insert_or_update() will not act on the change in the middle of a single call. Even if it did, I don't see any risks. I'd lean toward #3 if someone else is also confident in its correctness. Otherwise, #1 seems like the way to go. Preferences? Other ideas? Thanks, nm -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BUG #5856: pg_attribute.attinhcount is not correct.
On Sun, Apr 10, 2011 at 6:36 AM, Noah Misch n...@leadboat.com wrote: I had exactly what you just said in mind. Patch attached, then. Committed. Thanks. This turns out to have caused that TOAST creation regression: Crap. I am not going to be able to look at this today; I am getting on a plane to Santa Clara. I will look at it while I'm there if I can, but it's going to be a busy week for me so if anyone else can step in... On Fri, Apr 08, 2011 at 08:12:19PM -0400, Noah Misch wrote: [pg_upgrade/typed table business] I also tested a regular dump+reload of the regression database, and a pg_upgrade of the same. The latter failed further along, due (indirectly) to this failure to create a TOAST table: create table p (); create table ch () inherits (p); alter table p add column a text; select oid::regclass,reltoastrelid from pg_class where oid::regclass IN ('p','ch'); insert into ch values (repeat('x', 100)); If I drop table a_star cascade in the regression database before attempting pg_upgrade, it completes cleanly. Since ATExecAddColumn now handles the recursion, child table work queue entries no longer have AT_PASS_ADD_COL subcommands. Consequently, this heuristic in ATRewriteCatalogs skips over them: if (tab-relkind == RELKIND_RELATION (tab-subcmds[AT_PASS_ADD_COL] || tab-subcmds[AT_PASS_ALTER_TYPE] || tab-subcmds[AT_PASS_COL_ATTRS])) AlterTableCreateToastTable(tab-relid, (Datum) 0); SET STORAGE uses AT_PASS_MISC, so this test case also omits a TOAST table: set client_min_messages = debug1; -- display toast creation create table t (a text); -- makes toast alter table t alter a set storage plain; alter table t add b int default 0; -- rewrite the table - no toast alter table t alter a set storage extended; -- no toast added? insert into t (a) values (repeat('x', 100)); -- fails My first thought was to figure that the cost of needs_toast_table() is not a concern and simply remove the pass-usage heuristic. However, we don't want AlterTableCreateToastTable acquiring an AccessExclusiveLock for ALTER TABLE recipes that only acquired ShareUpdateExclusiveLock. I see these options: 1. Upgrade AT_SetStorage to take AccessExclusiveLock. Add a maybe_create_toast field to AlteredTableInfo. Have the AT_SetStorage, AT_AlterColumnType and AT_AddColumn implementations set it. 2. Upgrade AT_SetStorage to take AccessExclusiveLock. In ATRewriteCatalogs, replace the pass-usage heuristic with a test for locklevel == AccessExclusiveLock. This smells more like a hack, but it might be less vulnerable to omissions. On the other hand, the set of operations that could add TOAST tables are not particularly liable to grow. 3. Make AlterTableCreateToastTable acquire only ShareUpdateExclusiveLock and remove the pass-usage heuristic from ATRewriteCatalogs. For this to be valid, toast_insert_or_update() must behave reasonably in the face of a relation concurrently acquiring a TOAST table. Since it takes reltoastrelid from the relcache, toast_insert_or_update() will not act on the change in the middle of a single call. Even if it did, I don't see any risks. I'd lean toward #3 if someone else is also confident in its correctness. Otherwise, #1 seems like the way to go. Preferences? Other ideas? I haven't scrutinized the code but I would prefer #3 if it's viable without too much of a code footprint. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BUG #5856: pg_attribute.attinhcount is not correct.
On Sun, Apr 10, 2011 at 07:35:53AM -0400, Robert Haas wrote: On Sun, Apr 10, 2011 at 6:36 AM, Noah Misch n...@leadboat.com wrote: 3. Make AlterTableCreateToastTable acquire only ShareUpdateExclusiveLock and remove the pass-usage heuristic from ATRewriteCatalogs. For this to be valid, toast_insert_or_update() must behave reasonably in the face of a relation concurrently acquiring a TOAST table. Since it takes reltoastrelid from the relcache, toast_insert_or_update() will not act on the change in the middle of a single call. Even if it did, I don't see any risks. I'd lean toward #3 if someone else is also confident in its correctness. Otherwise, #1 seems like the way to go. Preferences? Other ideas? I haven't scrutinized the code but I would prefer #3 if it's viable without too much of a code footprint. It's certainly compact; patch attached. diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c index 5d5496d..cb0df40 100644 --- a/src/backend/catalog/toasting.c +++ b/src/backend/catalog/toasting.c @@ -59,11 +59,11 @@ AlterTableCreateToastTable(Oid relOid, Datum reloptions) Relationrel; /* -* Grab an exclusive lock on the target table, which we will NOT release -* until end of transaction. (This is probably redundant in all present -* uses...) +* Grab a DDL-exclusive lock on the target table, since we'll update the +* pg_class tuple. This is redundant for all present users. Tuple toasting +* behaves safely in the face of a concurrent TOAST table add. */ - rel = heap_open(relOid, AccessExclusiveLock); + rel = heap_open(relOid, ShareUpdateExclusiveLock); /* create_toast_table does all the work */ (void) create_toast_table(rel, InvalidOid, InvalidOid, reloptions); diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 886b656..03d1efa 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -3014,18 +3014,12 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode) } } - /* -* Check to see if a toast table must be added, if we executed any -* subcommands that might have added a column or changed column storage. -*/ + /* Check to see if a toast table must be added. */ foreach(ltab, *wqueue) { AlteredTableInfo *tab = (AlteredTableInfo *) lfirst(ltab); - if (tab-relkind == RELKIND_RELATION - (tab-subcmds[AT_PASS_ADD_COL] || -tab-subcmds[AT_PASS_ALTER_TYPE] || -tab-subcmds[AT_PASS_COL_ATTRS])) + if (tab-relkind == RELKIND_RELATION) AlterTableCreateToastTable(tab-relid, (Datum) 0); } } diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index d7d1b64..315b915 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -1522,6 +1522,19 @@ ERROR: composite type recur1 cannot be made a member of itself alter table recur1 add column f2 int; alter table recur1 alter column f2 type recur2; -- fails ERROR: composite type recur1 cannot be made a member of itself +-- SET STORAGE may need to add a TOAST table +create table test_storage (a text); +alter table test_storage alter a set storage plain; +alter table test_storage add b int default 0; -- rewrite table to remove its TOAST table +alter table test_storage alter a set storage extended; -- re-add TOAST table +select reltoastrelid 0 as has_toast_table +from pg_class +where oid = 'test_storage'::regclass; + has_toast_table +- + t +(1 row) + -- -- lock levels -- diff --git a/src/test/regress/input/misc.source b/src/test/regress/input/misc.source index 0930a6a..7cd26cb 100644 --- a/src/test/regress/input/misc.source +++ b/src/test/regress/input/misc.source @@ -153,6 +153,12 @@ SELECT * FROM e_star*; ALTER TABLE a_star* ADD COLUMN a text; +-- That ALTER TABLE should have added TOAST tables. +SELECT relname, reltoastrelid 0 AS has_toast_table + FROM pg_class + WHERE oid::regclass IN ('a_star', 'c_star') + ORDER BY 1; + --UPDATE b_star* -- SET a = text 'gazpacho' -- WHERE aa 4; diff --git a/src/test/regress/output/misc.source b/src/test/regress/output/misc.source index c225d0f..34bde31 100644 --- a/src/test/regress/output/misc.source +++ b/src/test/regress/output/misc.source @@ -376,6 +376,17 @@ SELECT * FROM e_star*; ALTER TABLE a_star* ADD COLUMN a text; NOTICE: merging definition of column a for child d_star +-- That ALTER TABLE should have added TOAST tables. +SELECT relname, reltoastrelid 0 AS has_toast_table + FROM pg_class + WHERE oid::regclass IN ('a_star', 'c_star') + ORDER BY 1; + relname | has_toast_table +-+- + a_star | t + c_star |
Re: [HACKERS] BUG #5856: pg_attribute.attinhcount is not correct.
On Sun, Apr 10, 2011 at 6:36 AM, Noah Misch n...@leadboat.com wrote: On Sun, Apr 03, 2011 at 09:53:57PM -0400, Robert Haas wrote: On Fri, Apr 1, 2011 at 12:56 AM, Noah Misch n...@leadboat.com wrote: On Thu, Mar 31, 2011 at 11:11:49AM -0400, Robert Haas wrote: On Thu, Mar 31, 2011 at 6:06 AM, Noah Misch n...@leadboat.com wrote: The best way I can see is to make ATExecAddColumn more like ATExecDropColumn, ATAddCheckConstraint, and ATExecDropConstraint. ?Namely, recurse at Exec-time rather than Prep-time, and cease recursing when we satisfy the ADD COLUMN with a merge. ?Did you have something else in mind? I had exactly what you just said in mind. Patch attached, then. Committed. Thanks. This turns out to have caused that TOAST creation regression: On Fri, Apr 08, 2011 at 08:12:19PM -0400, Noah Misch wrote: [pg_upgrade/typed table business] I also tested a regular dump+reload of the regression database, and a pg_upgrade of the same. The latter failed further along, due (indirectly) to this failure to create a TOAST table: create table p (); create table ch () inherits (p); alter table p add column a text; select oid::regclass,reltoastrelid from pg_class where oid::regclass IN ('p','ch'); insert into ch values (repeat('x', 100)); If I drop table a_star cascade in the regression database before attempting pg_upgrade, it completes cleanly. Since ATExecAddColumn now handles the recursion, child table work queue entries no longer have AT_PASS_ADD_COL subcommands. Consequently, this heuristic in ATRewriteCatalogs skips over them: if (tab-relkind == RELKIND_RELATION (tab-subcmds[AT_PASS_ADD_COL] || tab-subcmds[AT_PASS_ALTER_TYPE] || tab-subcmds[AT_PASS_COL_ATTRS])) AlterTableCreateToastTable(tab-relid, (Datum) 0); SET STORAGE uses AT_PASS_MISC, so this test case also omits a TOAST table: set client_min_messages = debug1; -- display toast creation create table t (a text); -- makes toast alter table t alter a set storage plain; alter table t add b int default 0; -- rewrite the table - no toast alter table t alter a set storage extended; -- no toast added? insert into t (a) values (repeat('x', 100)); -- fails Checking my understanding here, the first of these is a regression introduced by the patch for $SUBJECT, but the second one is a pre-existing bug. Is that right? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BUG #5856: pg_attribute.attinhcount is not correct.
On Sun, Apr 10, 2011 at 11:19:26AM -0400, Robert Haas wrote: On Sun, Apr 10, 2011 at 6:36 AM, Noah Misch n...@leadboat.com wrote: On Sun, Apr 03, 2011 at 09:53:57PM -0400, Robert Haas wrote: On Fri, Apr 1, 2011 at 12:56 AM, Noah Misch n...@leadboat.com wrote: On Thu, Mar 31, 2011 at 11:11:49AM -0400, Robert Haas wrote: On Thu, Mar 31, 2011 at 6:06 AM, Noah Misch n...@leadboat.com wrote: The best way I can see is to make ATExecAddColumn more like ATExecDropColumn, ATAddCheckConstraint, and ATExecDropConstraint. ?Namely, recurse at Exec-time rather than Prep-time, and cease recursing when we satisfy the ADD COLUMN with a merge. ?Did you have something else in mind? I had exactly what you just said in mind. Patch attached, then. Committed. Thanks. ?This turns out to have caused that TOAST creation regression: On Fri, Apr 08, 2011 at 08:12:19PM -0400, Noah Misch wrote: [pg_upgrade/typed table business] I also tested a regular dump+reload of the regression database, and a pg_upgrade of the same. ?The latter failed further along, due (indirectly) to this failure to create a TOAST table: ? create table p (); ? create table ch () inherits (p); ? alter table p add column a text; ? select oid::regclass,reltoastrelid from pg_class where oid::regclass IN ('p','ch'); ? insert into ch values (repeat('x', 100)); If I drop table a_star cascade in the regression database before attempting pg_upgrade, it completes cleanly. Since ATExecAddColumn now handles the recursion, child table work queue entries no longer have AT_PASS_ADD_COL subcommands. ?Consequently, this heuristic in ATRewriteCatalogs skips over them: ? ? ? ? ? ? ? ?if (tab-relkind == RELKIND_RELATION ? ? ? ? ? ? ? ? ? ? ? ?(tab-subcmds[AT_PASS_ADD_COL] || ? ? ? ? ? ? ? ? ? ? ? ? tab-subcmds[AT_PASS_ALTER_TYPE] || ? ? ? ? ? ? ? ? ? ? ? ? tab-subcmds[AT_PASS_COL_ATTRS])) ? ? ? ? ? ? ? ? ? ? ? ?AlterTableCreateToastTable(tab-relid, (Datum) 0); SET STORAGE uses AT_PASS_MISC, so this test case also omits a TOAST table: ?set client_min_messages = debug1; -- display toast creation ?create table t (a text); -- makes toast ?alter table t alter a set storage plain; ?alter table t add b int default 0; -- rewrite the table - no toast ?alter table t alter a set storage extended; -- no toast added? ?insert into t (a) values (repeat('x', 100)); -- fails Checking my understanding here, the first of these is a regression introduced by the patch for $SUBJECT, but the second one is a pre-existing bug. Is that right? The patch for $SUBJECT just introduced the first regression; commit 04e17bae50a73af524731fa11210d5c3f7d8e1f9 introduced the second regression near the beginning of the 9.1 development cycle. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BUG #5856: pg_attribute.attinhcount is not correct.
On Fri, Apr 1, 2011 at 12:56 AM, Noah Misch n...@leadboat.com wrote: On Thu, Mar 31, 2011 at 11:11:49AM -0400, Robert Haas wrote: On Thu, Mar 31, 2011 at 6:06 AM, Noah Misch n...@leadboat.com wrote: I think this is a manifestation the same problem mentioned here: http://git.postgresql.org/gitweb?p=postgresql.git;a=commit;h=31b6fc06d83c6de3644c8f2921eb7de0eb92fac3 I believe this requires some refactoring to fix. ?It would be good to do that. The best way I can see is to make ATExecAddColumn more like ATExecDropColumn, ATAddCheckConstraint, and ATExecDropConstraint. ?Namely, recurse at Exec-time rather than Prep-time, and cease recursing when we satisfy the ADD COLUMN with a merge. ?Did you have something else in mind? I had exactly what you just said in mind. Patch attached, then. Committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BUG #5856: pg_attribute.attinhcount is not correct.
[moving to pgsql-hackers] On Thu, Feb 03, 2011 at 11:24:42AM -0500, Robert Haas wrote: On Mon, Jan 31, 2011 at 6:42 AM, Naoya Anzai anzai-na...@mxu.nes.nec.co.jp wrote: In PostgreSQL8.4.5, I found that the catalog pg_attribute.attinhcount is not correct. I executed the following queries. -- create table 3_grandchild(); create table 2_child(); create table 1_parent(); alter table 3_grandchild inherit 2_child; alter table 2_child inherit 1_parent; alter table 3_grandchild add column c1 text; alter table 2_child add column c1 text; alter table 1_parent add column c1 text; select c.relname,a.attname,a.attinhcount from pg_attribute a,pg_class c where a.attrelid=c.oid and relname in ('1_parent','2_child','3_grandchild') and attname not in('xmax','xmin','cmin','cmax','tableoid','ctid') order by relname; ? ?relname ? ?| attname | attinhcount ?--+-+- ?1_parent ? ? | c1 ? ? ?| ? ? ? ? ? 0 ?2_child ? ? ?| c1 ? ? ?| ? ? ? ? ? 1 ?3_grandchild | c1 ? ? ?| ? ? ? ? ? 2 ?(3 rows) -- 3_grandchild's attinhcount should be 1. I think this is a manifestation the same problem mentioned here: http://git.postgresql.org/gitweb?p=postgresql.git;a=commit;h=31b6fc06d83c6de3644c8f2921eb7de0eb92fac3 I believe this requires some refactoring to fix. It would be good to do that. The best way I can see is to make ATExecAddColumn more like ATExecDropColumn, ATAddCheckConstraint, and ATExecDropConstraint. Namely, recurse at Exec-time rather than Prep-time, and cease recursing when we satisfy the ADD COLUMN with a merge. Did you have something else in mind? Incidentally, when we satisfy an ADD COLUMN with a merge, we do not check or update attnotnull: create table parent(); create table child(c1 text) inherits (parent); alter table parent add column c1 text not null; \d child We could either update attnotnull (and schedule a phase-3 scan of the table) or throw an error. For ALTER TABLE ... INHERIT, we throw the error. For CREATE TABLE ... INHERITS, we add the NOT NULL (and no scan is needed). I'd weakly lean toward throwing the error. Opinions? nm -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BUG #5856: pg_attribute.attinhcount is not correct.
--On 31. März 2011 06:06:49 -0400 Noah Misch n...@leadboat.com wrote: The best way I can see is to make ATExecAddColumn more like ATExecDropColumn, ATAddCheckConstraint, and ATExecDropConstraint. Namely, recurse at Exec-time rather than Prep-time, and cease recursing when we satisfy the ADD COLUMN with a merge. Did you have something else in mind? Incidentally, when we satisfy an ADD COLUMN with a merge, we do not check or update attnotnull: create table parent(); create table child(c1 text) inherits (parent); alter table parent add column c1 text not null; \d child We could either update attnotnull (and schedule a phase-3 scan of the table) or throw an error. For ALTER TABLE ... INHERIT, we throw the error. For CREATE TABLE ... INHERITS, we add the NOT NULL (and no scan is needed). I'd weakly lean toward throwing the error. Opinions? Hmm this looks like the same kind of problem i'm currently faced with when working on tracking inheritance counters for NOT NULL constraint at the moment (see http://git.postgresql.org/gitweb?p=users/bernd/postgres.git;a=shortlog;h=refs/heads/notnull_constraint for a heavy WIP patch). It currently recurses and seems to do the right thing (tm) for your example above, but i'm far from being certain that the way i'm undertaking here is correct. It indeed discovered a bug i had in my recursion code... -- Thanks Bernd -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BUG #5856: pg_attribute.attinhcount is not correct.
On Thu, Mar 31, 2011 at 6:06 AM, Noah Misch n...@leadboat.com wrote: I think this is a manifestation the same problem mentioned here: http://git.postgresql.org/gitweb?p=postgresql.git;a=commit;h=31b6fc06d83c6de3644c8f2921eb7de0eb92fac3 I believe this requires some refactoring to fix. It would be good to do that. The best way I can see is to make ATExecAddColumn more like ATExecDropColumn, ATAddCheckConstraint, and ATExecDropConstraint. Namely, recurse at Exec-time rather than Prep-time, and cease recursing when we satisfy the ADD COLUMN with a merge. Did you have something else in mind? I had exactly what you just said in mind. Incidentally, when we satisfy an ADD COLUMN with a merge, we do not check or update attnotnull: create table parent(); create table child(c1 text) inherits (parent); alter table parent add column c1 text not null; \d child We could either update attnotnull (and schedule a phase-3 scan of the table) or throw an error. For ALTER TABLE ... INHERIT, we throw the error. For CREATE TABLE ... INHERITS, we add the NOT NULL (and no scan is needed). I'd weakly lean toward throwing the error. Opinions? Not sure. I think that anything we do here is bound to have some corner cases that are not quite right for so long as NOT NULL constraints aren't represented in pg_constraint, and it's way too late to dredge up that issue again for 9.1. I'm somewhat inclined to just defer fixing it until we get that work committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BUG #5856: pg_attribute.attinhcount is not correct.
On Thu, Mar 31, 2011 at 11:11:49AM -0400, Robert Haas wrote: On Thu, Mar 31, 2011 at 6:06 AM, Noah Misch n...@leadboat.com wrote: I think this is a manifestation the same problem mentioned here: http://git.postgresql.org/gitweb?p=postgresql.git;a=commit;h=31b6fc06d83c6de3644c8f2921eb7de0eb92fac3 I believe this requires some refactoring to fix. ?It would be good to do that. The best way I can see is to make ATExecAddColumn more like ATExecDropColumn, ATAddCheckConstraint, and ATExecDropConstraint. ?Namely, recurse at Exec-time rather than Prep-time, and cease recursing when we satisfy the ADD COLUMN with a merge. ?Did you have something else in mind? I had exactly what you just said in mind. Patch attached, then. Incidentally, when we satisfy an ADD COLUMN with a merge, we do not check or update attnotnull: details ... what should we do? Not sure. I think that anything we do here is bound to have some corner cases that are not quite right for so long as NOT NULL constraints aren't represented in pg_constraint, and it's way too late to dredge up that issue again for 9.1. I'm somewhat inclined to just defer fixing it until we get that work committed. OK. diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 737ab1a..c64ffac 100644 *** a/src/backend/commands/tablecmds.c --- b/src/backend/commands/tablecmds.c *** *** 285,300 static void ATSimplePermissions(Relation rel, int allowed_targets); static void ATWrongRelkindError(Relation rel, int allowed_targets); static void ATSimpleRecursion(List **wqueue, Relation rel, AlterTableCmd *cmd, bool recurse, LOCKMODE lockmode); - static void ATOneLevelRecursion(List **wqueue, Relation rel, - AlterTableCmd *cmd, LOCKMODE lockmode); static void ATTypedTableRecursion(List **wqueue, Relation rel, AlterTableCmd *cmd, LOCKMODE lockmode); static List *find_typed_table_dependencies(Oid typeOid, const char *typeName, DropBehavior behavior); static void ATPrepAddColumn(List **wqueue, Relation rel, bool recurse, bool recursing, AlterTableCmd *cmd, LOCKMODE lockmode); ! static void ATExecAddColumn(AlteredTableInfo *tab, Relation rel, ! ColumnDef *colDef, bool isOid, LOCKMODE lockmode); static void add_column_datatype_dependency(Oid relid, int32 attnum, Oid typid, Oid collid); static void ATPrepAddOids(List **wqueue, Relation rel, bool recurse, AlterTableCmd *cmd, LOCKMODE lockmode); --- 285,299 static void ATWrongRelkindError(Relation rel, int allowed_targets); static void ATSimpleRecursion(List **wqueue, Relation rel, AlterTableCmd *cmd, bool recurse, LOCKMODE lockmode); static void ATTypedTableRecursion(List **wqueue, Relation rel, AlterTableCmd *cmd, LOCKMODE lockmode); static List *find_typed_table_dependencies(Oid typeOid, const char *typeName, DropBehavior behavior); static void ATPrepAddColumn(List **wqueue, Relation rel, bool recurse, bool recursing, AlterTableCmd *cmd, LOCKMODE lockmode); ! static void ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel, ! ColumnDef *colDef, bool isOid, ! bool recurse, bool recursing, LOCKMODE lockmode); static void add_column_datatype_dependency(Oid relid, int32 attnum, Oid typid, Oid collid); static void ATPrepAddOids(List **wqueue, Relation rel, bool recurse, AlterTableCmd *cmd, LOCKMODE lockmode); *** *** 2775,2789 ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, case AT_AddColumn: /* ADD COLUMN */ ATSimplePermissions(rel, ATT_TABLE|ATT_COMPOSITE_TYPE|ATT_FOREIGN_TABLE); - /* Performs own recursion */ ATPrepAddColumn(wqueue, rel, recurse, recursing, cmd, lockmode); pass = AT_PASS_ADD_COL; break; case AT_AddColumnToView:/* add column via CREATE OR REPLACE * VIEW */ ATSimplePermissions(rel, ATT_VIEW); - /* Performs own recursion */ ATPrepAddColumn(wqueue, rel, recurse, recursing, cmd, lockmode); pass = AT_PASS_ADD_COL; break;