I've had another go at this, and I've found a solution that appears to
address all the issues I'm aware of. It's all very similar to the
previously discussed patches. The main difference is that previous
patches had attempted to use something like tbinfo->attislocal to
determine whether a column was inherited, but that's not correct. This
patch uses the existing logic in flagInhAttrs() to find whether there is
a matching parent column with a generation expression. I've added
pg_dump test cases here to check the different variations that the code
addresses.
--
Peter Eisentraut
2ndQuadrant, an EDB company
https://www.2ndquadrant.com/
From 6cace6e6e7935844272fb201fc0096d8f2381d00 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Fri, 29 Jan 2021 13:20:41 +0100
Subject: [PATCH v4] 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. This resulted in unrestorable dumps. For generated
columns, just skip them in inherited tables.
Discussion:
https://www.postgresql.org/message-id/flat/15830.1575468847%40sss.pgh.pa.us
---
src/bin/pg_dump/common.c | 31 ++++++++++++++++-----
src/bin/pg_dump/pg_dump.c | 37 +++++++++++++++++++++----
src/bin/pg_dump/t/002_pg_dump.pl | 46 ++++++++++++++++++++++++++++++++
3 files changed, 102 insertions(+), 12 deletions(-)
diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index b0f02bc1f6..1a261a5545 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -467,12 +467,22 @@ flagInhIndexes(Archive *fout, TableInfo tblinfo[], int
numTables)
/* flagInhAttrs -
* for each dumpable table in tblinfo, flag its inherited attributes
*
- * What we need to do here is detect child columns that inherit NOT NULL
- * bits from their parents (so that we needn't specify that again for the
- * child) and child columns that have DEFAULT NULL when their parents had
- * some non-null default. In the latter case, we make up a dummy AttrDefInfo
- * object so that we'll correctly emit the necessary DEFAULT NULL clause;
- * otherwise the backend will apply an inherited default to the column.
+ * What we need to do here is:
+ *
+ * - Detect child columns that inherit NOT NULL bits from their parents, so
+ * that we needn't specify that again for the child.
+ *
+ * - Detect child columns that have DEFAULT NULL when their parents had some
+ * non-null default. In this case, we make up a dummy AttrDefInfo object so
+ * that we'll correctly emit the necessary DEFAULT NULL clause; otherwise
+ * the backend will apply an inherited default to the column.
+ *
+ * - Detect child columns that have a generation expression when their parents
+ * also have one. 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. (Exception: In binary upgrade mode we dump them because
+ * inherited tables are recreated standalone first and then reattached to
+ * the parent.)
*
* modifies tblinfo
*/
@@ -510,6 +520,7 @@ flagInhAttrs(DumpOptions *dopt, TableInfo *tblinfo, int
numTables)
{
bool foundNotNull; /* Attr was NOT NULL in
a parent */
bool foundDefault; /* Found a default in a
parent */
+ bool foundGenerated; /* Found a generated in
a parent */
/* no point in examining dropped columns */
if (tbinfo->attisdropped[j])
@@ -517,6 +528,7 @@ flagInhAttrs(DumpOptions *dopt, TableInfo *tblinfo, int
numTables)
foundNotNull = false;
foundDefault = false;
+ foundGenerated = false;
for (k = 0; k < numParents; k++)
{
TableInfo *parent = parents[k];
@@ -528,7 +540,8 @@ flagInhAttrs(DumpOptions *dopt, TableInfo *tblinfo, int
numTables)
if (inhAttrInd >= 0)
{
foundNotNull |=
parent->notnull[inhAttrInd];
- foundDefault |=
(parent->attrdefs[inhAttrInd] != NULL);
+ foundDefault |=
(parent->attrdefs[inhAttrInd] != NULL && !parent->attgenerated[inhAttrInd]);
+ foundGenerated |=
parent->attgenerated[inhAttrInd];
}
}
@@ -570,6 +583,10 @@ flagInhAttrs(DumpOptions *dopt, TableInfo *tblinfo, int
numTables)
tbinfo->attrdefs[j] = attrDef;
}
+
+ /* Remove generation expression from child */
+ if (foundGenerated && !dopt->binary_upgrade)
+ tbinfo->attrdefs[j] = NULL;
}
}
}
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 798d14580e..81a83b4a12 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -8839,13 +8839,37 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int
numTables)
attrdefs[j].dobj.dump = tbinfo->dobj.dump;
/*
- * Defaults on a VIEW must always be dumped as
separate ALTER
- * TABLE commands. Defaults on regular tables
are dumped as
- * part of the CREATE TABLE if possible, which
it won't be if
- * the column is not going to be emitted
explicitly.
+ * Figure out whether the default/generation
expression should
+ * be dumped as part of the main CREATE TABLE
(or similar)
+ * command or as a separate ALTER TABLE (or
similar) command.
+ * The preference is to put it into the CREATE
command, but in
+ * some cases that's not possible.
*/
- if (tbinfo->relkind == RELKIND_VIEW)
+ if (tbinfo->attgenerated[adnum - 1])
{
+ /*
+ * Column generation expressions cannot
be dumped
+ * separately, because there is no
syntax for it. The
+ * !shouldPrintColumn case below will
be tempted to set
+ * them to separate if they are
attached to an inherited
+ * column without a local definition,
but that would be
+ * wrong and unnecessary, because
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. By setting separate to
false here we prevent
+ * the "default" from being processed
as its own dumpable
+ * object, and flagInhAttrs() will
remove it from the
+ * table when it detects that it
belongs to an inherited
+ * column.
+ */
+ attrdefs[j].separate = false;
+ }
+ else if (tbinfo->relkind == RELKIND_VIEW)
+ {
+ /*
+ * Defaults on a VIEW must always be
dumped as separate
+ * ALTER TABLE commands.
+ */
attrdefs[j].separate = true;
}
else if (!shouldPrintColumn(dopt, tbinfo, adnum
- 1))
@@ -8856,7 +8880,10 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int
numTables)
else
{
attrdefs[j].separate = false;
+ }
+ if (!attrdefs[j].separate)
+ {
/*
* Mark the default as needing to
appear before the table,
* so that any dependencies it has must
be emitted before
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 798884da36..737e46464a 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -2493,6 +2493,52 @@
unlike => { exclude_dump_test_schema => 1, },
},
+ 'CREATE TABLE test_table_generated_child1 (without local columns)' => {
+ create_order => 4,
+ create_sql => 'CREATE TABLE
dump_test.test_table_generated_child1 ()
+ INHERITS
(dump_test.test_table_generated);',
+ regexp => qr/^
+ \QCREATE TABLE dump_test.test_table_generated_child1
(\E\n
+ \)\n
+ \QINHERITS (dump_test.test_table_generated);\E\n
+ /xms,
+ like =>
+ { %full_runs, %dump_test_schema_runs, section_pre_data => 1,
},
+ unlike => {
+ binary_upgrade => 1,
+ exclude_dump_test_schema => 1,
+ },
+ },
+
+ 'ALTER TABLE test_table_generated_child1' => {
+ regexp =>
+ qr/^\QALTER TABLE ONLY dump_test.test_table_generated_child1
ALTER COLUMN col2 \E/m,
+
+ # should not get emitted
+ like => {},
+ },
+
+ 'CREATE TABLE test_table_generated_child2 (with local columns)' => {
+ create_order => 4,
+ create_sql => 'CREATE TABLE
dump_test.test_table_generated_child2 (
+ col1 int,
+ col2 int
+ ) INHERITS
(dump_test.test_table_generated);',
+ regexp => qr/^
+ \QCREATE TABLE dump_test.test_table_generated_child2
(\E\n
+ \s+\Qcol1 integer,\E\n
+ \s+\Qcol2 integer\E\n
+ \)\n
+ \QINHERITS (dump_test.test_table_generated);\E\n
+ /xms,
+ like =>
+ { %full_runs, %dump_test_schema_runs, section_pre_data => 1,
},
+ unlike => {
+ binary_upgrade => 1,
+ exclude_dump_test_schema => 1,
+ },
+ },
+
'CREATE TABLE table_with_stats' => {
create_order => 98,
create_sql => 'CREATE TABLE dump_test.table_index_stats (
base-commit: b034ef9b376dbe712caa076541d6a750f37d85ce
--
2.30.0
