Обсуждение: Multi-parent inherited table with mixed storage options cannot berestored

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.

- Heikki

On Thu, May 14, 2020 at 7:31 PM Heikki Linnakangas <hlinnaka@iki.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

At Fri, 15 May 2020 11:54:09 +0900, Amit Langote <amitlangote09@gmail.com> wrote in 
> On Thu, May 14, 2020 at 7:31 PM Heikki Linnakangas <hlinnaka@iki.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:

I think there's no problem if a table in an inheritance tree has
different storage type from others.  It seems to me that the reason
for the CREATE TABLE's behavior is that there's no reasonable criteria
of from which parent the new child table ought to inherit a
characteristic.

I thought that just building inheritance tree without setting such
characteristics, then later setting such characteristics for all
participants of the tree would work but it seems too invasive.


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

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center