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', 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
