Re: Dumping/restoring fails on inherited generated column

On 2020-02-03 20:32, Tom Lane wrote:
 > Things are evidently also going wrong for "gtest1_1".  In that case
 > the generated property is inherited from the parent gtest1, so we
 > shouldn't be emitting anything ... how come the patch fails to
 > make it do that?

This is fixed by the attached new patch.  It needed an additional check 
in flagInhAttrs().

> This is showing us at least two distinct problems.  Now as for
> "gtest30_1", what we have is that in the parent table "gtest30", column b
> exists but it has no default; the generated property is only added
> at the child table gtest30_1.  So we need to emit ALTER COLUMN SET
> GENERATED ALWAYS for gtest30_1.b.  HEAD is already doing the wrong
> thing there (it's emitting the expression, but as a plain default
> not GENERATED).  And this patch makes it emit nothing, even worse.
> I think the key point here is that "attislocal" refers to whether the
> column itself is locally defined, not to whether its default is.

This is a bit of a mess.  Let me explain my thinking on generated 
columns versus inheritance.

If a parent table has a generated column, then any inherited column must 
also be generated and use the same expression.  (Otherwise querying the 
parent table would produce results that are inconsistent with the 
generation expression if the rows come from the child table.)

If a parent table has a column that is not generated, then I think it 
would be semantically sound if a child table had that same column but 
generated.  However, I think it would be very tricky to support this 
correctly, and it doesn't seem useful enough, so I'd rather not do it.

That's what the gtest30_1 case above shows.  It's not even clear whether 
it's possible to dump this correctly in all cases because the syntax 
that you allude to "turn this existing column into a generated column" 
does not exist.

Note that the gtest30 test case is new in master.  I'm a bit confused 
why things were done that way, and I'll need to revisit this.  I've also 
found a few more issues with how certain combinations of DDL can create 
similar situations that arguably don't make sense, and I'll continue to 
look into those.  Basically, my contention is that gtest30_1 should not 
be allowed to exist like that.

However, the pg_dump issue is separate from those because it affects a 
case that is clearly legitimate.  So assuming that we end up agreeing on 
a version of the attached pg_dump patch, I would like to get that into 
the next minor releases and then investigate the other issues separately.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services