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(at)sss(dot)pgh(dot)pa(dot)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.
>