Feedback-ID: ie0a040ee:Fastmail
Message-ID: <8bbef9cb-ef37-44dc-9172-4e225ae67a4a@eisentraut.org>
Date: Tue, 20 Feb 2024 11:21:12 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: table inheritance versus column compression and storage settings
Content-Language: en-US
To: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>,
 Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
References: <24656cec-d6ef-4d15-8b5b-e8dfc9c833a7@eisentraut.org>
 <CAExHW5uH0q810ZZ7ZP2Bvz_aF33yPTUsZ5NNC=yvA1MsVVtDTw@mail.gmail.com>
 <38546838-8a60-4d50-ba8e-b2c7cf2cb64b@eisentraut.org>
 <CAExHW5uAqaO7RcDgBiwm2pVO58evFY8AKzvVks-2dn3FKyWUkQ@mail.gmail.com>
 <CAExHW5sVAMNBNeD8-nih8HY8n4ySJWe9Uf+QNRTMOD7u-De-CQ@mail.gmail.com>
 <CAExHW5v+Au_kadPJd7ifmKLzFu+fbtkDp5duuzuS6bZdLJsD1w@mail.gmail.com>
 <eb076b15-64b5-4992-b232-9e00603e0849@eisentraut.org>
 <CAExHW5tgaiJynSWA25ufWwnXex8s9475G23+7DNnhifP9NPNvw@mail.gmail.com>
 <c3090836-9da9-48ed-8332-031bd0fee457@eisentraut.org>
 <3426460.1708095481@sss.pgh.pa.us> <3462358.1708107856@sss.pgh.pa.us>
 <CAExHW5vyqv=XLTcNMzCNccOrHiun_XhYPjcRqeV6dLvZSamriQ@mail.gmail.com>
From: Peter Eisentraut <peter@eisentraut.org>
In-Reply-To: <CAExHW5vyqv=XLTcNMzCNccOrHiun_XhYPjcRqeV6dLvZSamriQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/8bbef9cb-ef37-44dc-9172-4e225ae67a4a%40eisentraut.org>
Precedence: bulk

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.
>