BUG #5856: pg_attribute.attinhcount is not correct.

On Mon, Jan 31, 2011 at 6:42 AM, Naoya Anzai
<anzai-naoya@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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

[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-naoya@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

--On 31. März 2011 06:06:49 -0400 Noah Misch <noah@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&gt;
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

On Thu, Mar 31, 2011 at 6:06 AM, Noah Misch <noah@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

On Thu, Mar 31, 2011 at 11:11:49AM -0400, Robert Haas wrote:

On Thu, Mar 31, 2011 at 6:06 AM, Noah Misch <noah@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.

On Fri, Apr 1, 2011 at 12:56 AM, Noah Misch <noah@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 <noah@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

On Sun, Apr 03, 2011 at 09:53:57PM -0400, Robert Haas wrote:

On Fri, Apr 1, 2011 at 12:56 AM, Noah Misch <noah@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 <noah@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', 1000000));

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', 1000000)); -- 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

On Sun, Apr 10, 2011 at 6:36 AM, Noah Misch <noah@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', 1000000));

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', 1000000)); -- 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

On Sun, Apr 10, 2011 at 07:35:53AM -0400, Robert Haas wrote:

On Sun, Apr 10, 2011 at 6:36 AM, Noah Misch <noah@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.

On Sun, Apr 10, 2011 at 6:36 AM, Noah Misch <noah@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 <noah@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 <noah@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', 1000000));

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', 1000000)); -- 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

On Sun, Apr 10, 2011 at 11:19:26AM -0400, Robert Haas wrote:

On Sun, Apr 10, 2011 at 6:36 AM, Noah Misch <noah@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 <noah@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 <noah@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', 1000000));

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', 1000000)); -- 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.

On Sun, Apr 10, 2011 at 5:23 AM, Noah Misch <noah@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 <noah@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