Re: BUG #5856: pg_attribute.attinhcount is not correct.

On Sun, Apr 10, 2011 at 6:36 AM, Noah Misch <noah(at)leadboat(dot)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