From: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Dumping/restoring fails on inherited generated column
Date: Thu, 6 Feb 2020 20:36:24 +0100
Message-ID: <b1c831dd-d520-5e7f-0304-0eeed39c9996@2ndquadrant.com> (raw)
In-Reply-To: <24369.1580758361@sss.pgh.pa.us>
References: <15830.1575468847@sss.pgh.pa.us>
<fd559544-274c-f514-d8bf-358d2c539455@2ndquadrant.com>
<31905.1575491779@sss.pgh.pa.us>
<d6ebcc2e-3e6f-6915-6616-70ef665aed66@2ndquadrant.com>
<24369.1580758361@sss.pgh.pa.us>
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
Attachments:v3-0001-pg_dump-Fix-dumping-of-inherited-generated-column.patch (text/plain, 2.3K) download
From 76249c9a64caaaf4c3206d5ca4ff72cc186dc416 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@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.cindex 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.cindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: peter.eisentraut@2ndquadrant.com, tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org
Subject: Re: Dumping/restoring fails on inherited generated column
In-Reply-To: <b1c831dd-d520-5e7f-0304-0eeed39c9996@2ndquadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox