Re: 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 <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