Re: Dumping/restoring fails on inherited generated column

[ Pulling Daniel into this older thread seems like the cleanest way to
unify the two threads ]

Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com> writes:
> If we have ALTER TABLE ONLY / DROP EXPRESSION update the attlocal
> column of children to true to fix the issue you raised, my proposed
> patch is not necessary. OTOH if we fix it by prohibiting the command,
> I guess we need both patches to fix the issues.

Right, Peter already mentioned that we need a further pg_dump fix on
top of prohibiting the ONLY ... DROP EXPRESSION case.

Bug #16622 [1] is another report of this same issue, and in that thread,
Daniel argues that the right fix is just

+ /*
+ * Skip if the column isn't local to the table's definition as the attrdef
+ * will be printed with the inheritance parent table definition
+ */
+ if (!tbinfo->attislocal[adnum - 1])
+ return;

without the attgenerated clause that Masahiko-san proposes.
I think however that that's wrong. It is possible to have
a non-attislocal column that has its own default, because
you can do

d3=# create table parent (f1 int default 2);
CREATE TABLE
d3=# create table child (f1 int default 3) inherits(parent);
NOTICE: merging column "f1" with inherited definition
CREATE TABLE
d3=# create table child2() inherits(parent);
CREATE TABLE
d3=# alter table child2 alter column f1 set default 42;
ALTER TABLE

This does not cause child2.f1's attislocal property to become
true. Maybe it should have, but it's probably too late for
that; at least, pg_dump couldn't assume it's true in older
servers. Therefore, since we can't tell whether the default
is inherited or not, we'd better dump it.

(I recall that pg_dump has a hack somewhere that checks for
textual equality of CHECK conditions to avoid dumping redundant
child copies. Maybe we could do something similar here.)

The situation is different for GENERATED columns, since we disallow
a child having a different GENERATED property than the parent.
However, I think Masahiko-san's patch is not quite right either,
because a column can be both inherited and local. An example is

d3=# create table pgen (a int, b int GENERATED ALWAYS AS (a * 2) STORED);
CREATE TABLE
d3=# create table cgen1 (b int) inherits (pgen);
NOTICE: moving and merging column "b" with inherited definition
DETAIL: User-specified column moved to the position of the inherited column.
CREATE TABLE
d3=# select attname, attislocal, attinhcount from pg_attribute where attrelid = 'cgen1'::regclass and attnum>0;
attname | attislocal | attinhcount
---------+------------+-------------
a | f | 1
b | t | 1
(2 rows)

So it appears to me that a more correct coding is

/*
* Do not print a GENERATED default for an inherited column; it will
* be inherited from the parent, and the backend won't accept a
* command to set it separately.
*/
if (tbinfo->attinhcount[adnum - 1] > 0 && tbinfo->attgenerated[adnum - 1])
return;

Unfortunately this has still got a problem: it will mishandle the case of
a child column that is GENERATED while its parent is not. Peter opined
way upthread that we should not allow that, but according to my testing
we do.

This'd all be a lot cleaner if defaults were marked as to whether they
were inherited or locally generated. Maybe we ought to work on that.

In the meantime, maybe the best bet is for pg_dump to try to detect
whether a default is identical to a parent default, and not dump it
if so. That would fix both the GENERATED case where we must not
dump it, and the non-GENERATED case where it's merely inefficient
to do so.

regards, tom lane

[1] https://www.postgresql.org/message-id/flat/16622-779a79851b4e2491%40postgresql.org