Re: table inheritance versus column compression and storage settings

I have reverted the patch for now (and re-opened the commitfest entry). 
We should continue to work on this and see if we can at least try to get 
the pg_dump test coverage suitable.


On 19.02.24 12:34, Ashutosh Bapat wrote:
> On Fri, Feb 16, 2024 at 11:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> I wrote:
>>> I find it surprising that the committed patch does not touch
>>> pg_dump.  Is it really true that pg_dump dumps situations with
>>> differing compression/storage settings accurately already?
>>
>> It's worse than I thought.  Run "make installcheck" with
>> today's HEAD, then:
>>
>> $ pg_dump -Fc regression >r.dump
>> $ createdb r2
>> $ pg_restore -d r2 r.dump
>> pg_restore: error: could not execute query: ERROR:  column "a" inherits conflicting storage methods
>> HINT:  To resolve the conflict, specify a storage method explicitly.
>> Command was: CREATE TABLE public.stchild4 (
>>      a text
>> )
>> INHERITS (public.stparent1, public.stparent2);
>> ALTER TABLE ONLY public.stchild4 ALTER COLUMN a SET STORAGE MAIN;
>>
>>
>> pg_restore: error: could not execute query: ERROR:  relation "public.stchild4" does not exist
>> Command was: ALTER TABLE public.stchild4 OWNER TO postgres;
>>
>> pg_restore: error: could not execute query: ERROR:  relation "public.stchild4" does not exist
>> Command was: COPY public.stchild4 (a) FROM stdin;
>> pg_restore: warning: errors ignored on restore: 3
> 
> Thanks for the test. Let's call this Problem1. I expected
> src/bin/pg_upgrade/t/002_pg_upgrade.pl to fail in this case since it
> will execute similar steps as you did. And it actually does, except
> that it uses binary-upgrade mode. In that mode, INHERITed tables are
> dumped in a different manner
> -- For binary upgrade, set up inheritance this way.
> ALTER TABLE ONLY "public"."stchild4" INHERIT "public"."stparent1";
> ALTER TABLE ONLY "public"."stchild4" INHERIT "public"."stparent2";
> ... snip ...
> ALTER TABLE ONLY "public"."stchild4" ALTER COLUMN "a" SET STORAGE MAIN;
> 
> that does not lead to the conflict and pg_upgrade does not fail.
> 
>>
>>
>> What I'd intended to compare was the results of the query added to the
>> regression tests:
>>
>> regression=# SELECT attrelid::regclass, attname, attstorage FROM pg_attribute
>> WHERE (attrelid::regclass::name like 'stparent%'
>> OR attrelid::regclass::name like 'stchild%')
>> and attname = 'a'
>> ORDER BY 1, 2;
>>   attrelid  | attname | attstorage
>> -----------+---------+------------
>>   stparent1 | a       | p
>>   stparent2 | a       | x
>>   stchild1  | a       | p
>>   stchild3  | a       | m
>>   stchild4  | a       | m
>>   stchild5  | a       | x
>>   stchild6  | a       | m
>> (7 rows)
>>
>> r2=# SELECT attrelid::regclass, attname, attstorage FROM pg_attribute
>> WHERE (attrelid::regclass::name like 'stparent%'
>> OR attrelid::regclass::name like 'stchild%')
>> and attname = 'a'
>> ORDER BY 1, 2;
>>   attrelid  | attname | attstorage
>> -----------+---------+------------
>>   stparent1 | a       | p
>>   stchild1  | a       | p
>>   stchild3  | a       | m
>>   stparent2 | a       | x
>>   stchild5  | a       | p
>>   stchild6  | a       | m
>> (6 rows)
>>
>> So not only does stchild4 fail to restore altogether, but stchild5
>> ends with the wrong attstorage.
> 
> With binary-upgrade dump and restore stchild5 gets the correct storage value.
> 
> Looks like we need a test which pg_dump s regression database and
> restores it without going through pg_upgrade.
> 
> I think the fix is easy one. Dump the STORAGE and COMPRESSION clauses
> with CREATE TABLE for local attributes. Those for inherited attributes
> will be dumped separately.
> 
> But that will not fix an existing problem described below. Let's call
> it Problem2. With HEAD  at commit
> 57f59396bb51953bb7b957780c7f1b7f67602125 (almost a month back)
> $ createdb regression
> $ psql -d regression
> #create table par1 (a text storage plain);
> #create table par2 (a text storage plain);
> #create table chld (a text) inherits (par1, par2);
> NOTICE:  merging multiple inherited definitions of column "a"
> NOTICE:  merging column "a" with inherited definition
> -- parent storages conflict after child creation
> #alter table par1 alter column a set storage extended;
> #SELECT attrelid::regclass, attname, attstorage FROM pg_attribute
>   WHERE (attrelid::regclass::name like 'par%'
>   OR attrelid::regclass::name like 'chld%')
>   and attname = 'a'
>   ORDER BY 1, 2;
>   attrelid | attname | attstorage
> ----------+---------+------------
>   par1     | a       | x
>   par2     | a       | p
>   chld     | a       | x
> (3 rows)
> 
> $ createdb r2
> $ pg_dump -Fc regression > /tmp/r.dump
> $ pg_restore -d r2 /tmp/r.dump
> pg_restore: error: could not execute query: ERROR:  inherited column
> "a" has a storage parameter conflict
> DETAIL:  EXTENDED versus PLAIN
> Command was: CREATE TABLE public.chld (
>      a text
> )
> INHERITS (public.par1, public.par2);
> 
> pg_restore: error: could not execute query: ERROR:  relation
> "public.chld" does not exist
> Command was: ALTER TABLE public.chld OWNER TO ashutosh;
> 
> pg_restore: error: could not execute query: ERROR:  relation
> "public.chld" does not exist
> Command was: COPY public.chld (a) FROM stdin;
> pg_restore: warning: errors ignored on restore: 3
> 
> Fixing this requires that we dump ALTER TABLE ... ALTER COLUMN SET
> STORAGE and COMPRESSION commands after all the tables (at least
> children) have been created. That seems to break the way we dump the
> whole table together right now. OR dump inherited tables like binary
> upgrade mode.
>