Re: table inheritance versus column compression and storage settings

On Wed, Jan 31, 2024 at 1:29 PM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>
> On Tue, Dec 5, 2023 at 6:28 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
> >
> > On 05.12.23 05:26, Ashutosh Bapat wrote:
> > >> - When inheriting from multiple parents with different settings, an
> > >> explicit setting in the child is required.
> > > When no explicit setting for child is specified, it will throw an
> > > error as it does today. Right?
> >
> > Yes, it would throw an error, but a different error than today, saying
> > something like "the settings in the parents conflict, so you need to
> > specify one here to override the conflict".
> >
>
> PFA patch fixing inheritance and compression. It also fixes a crash
> reported in [1].
>
> The storage looks more involved. The way it has been coded, the child
> always inherits the parent's storage properties.
> #create table t1 (a text storage plain);
> CREATE TABLE
> #create table c1 (b text storage main) inherits(t1);
> CREATE TABLE
> #create table c1 (a text storage main) inherits(t1);
> NOTICE: merging column "a" with inherited definition
> CREATE TABLE
> #\d+ t1
> Table "public.t1"
> Column | Type | Collation | Nullable | Default | Storage |
> Compression | Stats target | Description
> --------+------+-----------+----------+---------+---------+-------------+--------------+-------------
> a | text | | | | plain |
> | |
> Child tables: c1
> Access method: heap
> #\d+ c1
> Table "public.c1"
> Column | Type | Collation | Nullable | Default | Storage |
> Compression | Stats target | Description
> --------+------+-----------+----------+---------+---------+-------------+--------------+-------------
> a | text | | | | plain |
> | |
> Inherits: t1
> Access method: heap
>
> Observe that c1.a did not have storage "main" but instead inherits
> "plain" from t1.
>
> According to the code at
> https://github.com/postgres/postgres/blob/6a1ea02c491d16474a6214603dce40b5b122d4d1/src/backend/commands/tablecmds.c#L3253,
> there is supposed to be a conflict error. But that does not happen
> since child's storage specification is in ColumnDef::storage_name
> which is never consulted. The ColumnDef::storage_name is converted to
> ColumnDef::storage only in BuildDescForRelation(), after
> MergeAttribute() has been finished. There are two ways to fix this
> 1. In MergeChildAttribute() resolve ColumnDef::storage_name to
> ColumnDef::storage before comparing it against inherited property. I
> don't like this approach since a. we duplicate the conversion logic in
> MergeChildAttribute() and BuildDescForRelation(), b. the conversion
> happens only for the attributes which are inherited.
>
> 2. Deal with it the same way as compression. Get rid of
> ColumnDef::storage altogether. Instead set ColumnDef::storage_name at
> https://github.com/postgres/postgres/blob/6a1ea02c491d16474a6214603dce40b5b122d4d1/src/backend/commands/tablecmds.c#L2723.
>
> I am inclined to take the second approach. Let me know if you feel otherwise.

Took the second approach. PFA patches
0001 fixes compression inheritance
0002 fixes storage inheritance

The patches may be committed separately or as a single patch. Keeping
them separate in case we decide to commit one but not the other.

We always set storage even if it's not specified, in which case the
column type's default storage is used. This is slightly different from
compression which defaults to the GUC's value if not set. This has led
to slight difference in the tests.

--
Best Wishes,
Ashutosh Bapat