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:
> 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(at)leadboat(dot)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(at)leadboat(dot)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