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