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
From 76249c9a64caaaf4c3206d5ca4ff72cc186dc416 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <pe...@eisentraut.org> Date: Thu, 6 Feb 2020 14:50:01 +0100 Subject: [PATCH v3] pg_dump: Fix dumping of inherited generated columns Generation expressions of generated columns are always inherited, so there is no need to set them separately in child tables, and there is no syntax to do so either. The code previously used the code paths for the handling of default values, for which different rules apply; in particular it might want to set a default value explicitly for an inherited column. For generated columns, just skip all that. Discussion: https://www.postgresql.org/message-id/flat/15830.1575468847%40sss.pgh.pa.us --- src/bin/pg_dump/common.c | 8 ++++++++ src/bin/pg_dump/pg_dump.c | 9 +++++++++ 2 files changed, 17 insertions(+) diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c index 3549f7bc08..170c87823d 100644 --- a/src/bin/pg_dump/common.c +++ b/src/bin/pg_dump/common.c @@ -476,6 +476,14 @@ flagInhAttrs(DumpOptions *dopt, TableInfo *tblinfo, int numTables) if (tbinfo->attisdropped[j]) continue; + /* + * Skip generated columns; it is not possible for an inherited + * column to have a different generation expression that the + * parent. + */ + if (tbinfo->attgenerated[j]) + continue; + foundNotNull = false; foundDefault = false; for (k = 0; k < numParents; k++) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 33e58fa287..5fcb89093e 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -8492,6 +8492,15 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables) if (tbinfo->attisdropped[adnum - 1]) continue; + /* + * Ignore generated columns on child tables unless they have a + * local definition. Generation expressions are always + * inherited, so there is no need to set them again in child + * tables, and there is no syntax for it either. + */ + if (tbinfo->attgenerated[adnum - 1] && !tbinfo->attislocal[adnum - 1]) + continue; + attrdefs[j].dobj.objType = DO_ATTRDEF; attrdefs[j].dobj.catId.tableoid = atooid(PQgetvalue(res, j, 0)); attrdefs[j].dobj.catId.oid = atooid(PQgetvalue(res, j, 1)); -- 2.25.0
