Re: [HACKERS] BUG #5856: pg_attribute.attinhcount is not correct.

2011-04-13 Thread Robert Haas 
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.

2011-04-10 Thread Noah Misch 
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.

2011-04-10 Thread Robert Haas 
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.

2011-04-10 Thread Noah Misch 
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.

2011-04-10 Thread Robert Haas 
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.

2011-04-10 Thread Noah Misch 
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.

2011-04-03 Thread Robert Haas 
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.

2011-03-31 Thread Noah Misch 
[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.

2011-03-31 Thread Bernd Helmle 



--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.

2011-03-31 Thread Robert Haas 
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.

2011-03-31 Thread Noah Misch 
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;