Thread: Re: 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-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> 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.
Attachment
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
altera set storage plain; alter table t add b int default 0; -- rewrite the table - no toast alter table t alter a set
storageextended; -- 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.
Attachment
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