Re: tablecmds.c/MergeAttributes() cleanup

On Tue, Apr 30, 2024 at 2:19 AM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> On Mon, Apr 29, 2024 at 6:46 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Sat, Apr 20, 2024 at 12:17 AM Ashutosh Bapat
>> <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>> > Yes please. Probably this issue surfaced again after we reverted compression and storage fix? Please If that's the case, please add it to the open items.
>>
>> This is still on the open items list and I'm not clear who, if anyone,
>> is working on fixing it.
>>
>> It would be good if someone fixed it. :-)
>
> Here's a patch fixing it.
>
> I have added the reproducer provided by Alexander as a test. I thought of improving that test further to test the compression of the inherited table but did not implement it since we haven't documented the behaviour of compression with inheritance. Defining and implementing compression behaviour for inherited tables was the goal of 0413a556990ba628a3de8a0b58be020fd9a14ed0, which has been reverted.

I took a look at this patch. Currently this case crashes:

CREATE TABLE cmdata(f1 text COMPRESSION pglz);
CREATE TABLE cmdata3(f1 text);
CREATE TABLE cminh() INHERITS (cmdata, cmdata3);

The patch makes this succeed, but I was initially unclear why it
didn't make it fail with an error instead: you can argue that cmdata
has pglz and cmdata3 has default and those are different. It seems
that prior precedent goes both ways -- we treat the absence of a
STORAGE specification as STORAGE EXTENDED and it conflicts with an
explicit storage specification on some other inheritance parent - but
on the other hand, we treat the absence of a default as compatible
with any explicit default, similar to what happens here. But I
eventually realized that you're just putting back behavior that we had
in previous releases: pre-v17, the code already works the way this
patch makes it do, and MergeChildAttribute() is already coded similar
to this. As Alexander Lakhin said upthread, 4d969b2f8 seems to have
broken this.

So now I think this is committable, but I can't do it now because I
won't be around for the next few hours in case the buildfarm blows up.
I can do it tomorrow, or perhaps Peter would like to handle it since
it seems to have been his commit that introduced the issue.

--
Robert Haas
EDB: http://www.enterprisedb.com