Re: Multi-parent inherited table with mixed storage options cannot be restored

On Thu, May 14, 2020 at 7:31 PM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>
> CREATE TABLE parent1 (
> col text
> );
>
> CREATE TABLE parent2 (
> col text
> );
>
> CREATE TABLE child () INHERITS (parent1, parent2);
>
> ALTER TABLE ONLY parent1 ALTER COLUMN col SET STORAGE EXTERNAL;
> ALTER TABLE ONLY parent2 ALTER COLUMN col SET STORAGE MAIN;
>
>
> pg_dump dumps those commands in different order (dump attached), with
> the SET STORAGE commands before creating the child table. But that's not
> allowed:
>
> psql:mixed-storage-inheritance.sql:53: ERROR: inherited column "col"
> has a storage parameter conflict
> DETAIL: EXTERNAL versus MAIN
>
> That's not good.
>
> What's the best way to fix it? Perhaps pg_dump should leave out the
> INHERITS clause from the CREATE TABLE statement, and dump separate
> "ALTER TABLE child INHERIT" commands to make it inherited? But then
> 'attislocal' is wrongly to false for the columns. Or perhaps relax the
> check for mixed storage options, so that the CREATE TABLE won't throw
> that error.

Btw, inheriting from the parents using ALTER TABLE doesn't raise the error:

CREATE TABLE public.parent1 (col text);
ALTER TABLE ONLY public.parent1 ALTER COLUMN col SET STORAGE MAIN;

CREATE TABLE public.parent2 (col text);
ALTER TABLE ONLY public.parent2 ALTER COLUMN col SET STORAGE MAIN;

CREATE TABLE public.child (col text);

ALTER TABLE public.child INHERIT public.parent1;
ALTER TABLE public.child INHERIT public.parent2;

It seems that MergeAttributes() (used by CREATE TABLE INHERITS) and
MergeAttributesIntoExisting() (used by ALTER TABLE INHERIT) disagree
as to what the rule regarding attstorage is:

MergeAttributes() has:

/* Copy storage parameter */
if (def->storage == 0)
def->storage = attribute->attstorage;
else if (def->storage != attribute->attstorage)
ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("inherited column \"%s\" has a
storage parameter conflict",
attributeName),
errdetail("%s versus %s",
storage_name(def->storage),
storage_name(attribute->attstorage))))

MergeAttributesInfoExisting() has nothing.

But instead of fixing the latter to enforce the same rule, I'd say we
should relax the check in the former.

If someone wants to say that we should enforce that all the tables
involved in an inheritance relationship have the same attstorage, then
maybe we have more work to do than simply fixing
MergeAttributesIntoExisting(). We will also have to fix
ATExecSetStorage() to check if the table it's operating on is part of
some inheritance tree and prevent attstorage from being changed if it
is, because that table's attstorage will become out of step with its
inheritance peers. I'd say that's overkill though, so prefer the
other solution.

--
Amit Langote
EnterpriseDB: http://www.enterprisedb.com