Hi Henson, I looked further into this. I considered a few approaches to resolve these edge cases.
> The > "Indirect loop" repair picks the pair (b, b.g). b.g is already > separate and there is no b -> b.g edge to remove, so it only adds > another b.g -> b edge and the same loop is found again. 1. One idea I tried was to change the loop repair logic to handle 1 & 2a. This would check the edge direction before picking the edge to remove. And therefore b.g -> b would would not be picked. Similarly I also added a new loop repair rule for 2a, for loops that go through the child -> parent-expression edge.. But I decided against this route because it seemed likely that more edge cases would need to be found and fixed. And it would become difficult to maintain. 2. I considered always dumping the child's 'alter ... set expression' at the end. This would dump them irrespective of whether they matched the parent or not and avoid any loops. This is similar to how 'set default' is currently handled for child columns. But this would have broken the restore to v16 and below as 'set expression' doesn't exist in the older versions. 3. I decided to avoid adding new dependencies and instead separately check which child tables needed their own 'alter ... set expression'. I kept some of the code similar to what I had before. If the child has a different expression from the parent, its 'alter ... set expression' is printed anyway because 'separate' is set to true. If a child and parent have the same expression but that happened because the parent's own expression is dumped as a separate ALTER, instead of creating an edge for it, the child would be created before that ALTER runs and inherit the old expression. Instead of creating an edge, we check it separately after the tables have been sorted. This seemed the cleanest approach. In this, I have created a new function flagInhGenerationExprs which checks for the cases where a parent has the separate flag set and is actually dumped. It's run after sorting the tables. I have created a new field in AttrDefInfo called maybe_dump_later_if_altered_expression. This is set in flagInhAttrs() for the cases which might need this special check later. This fixes 1, 2a, and 2b from your cases. Note this still fails for the following multi-parent scenario. CREATE TABLE gp (id int, v int, gs int GENERATED ALWAYS AS (v * 3) STORED); CREATE TABLE p1 () INHERITS (gp); ALTER TABLE p1 ALTER COLUMN gs SET EXPRESSION AS (v * 7); -- p1: (v * 7), inherited-only CREATE TABLE p2 (id int, v int, gs int GENERATED ALWAYS AS (v * 7) STORED); CREATE TABLE c () INHERITS (p1, p2); -- parents agree: both (v * 7) INSERT INTO c VALUES (1, 1); This scenario also fails on master and defaults seem to have the same limitation there. So I haven't tried to fix with any other special case. Let me know what you think of this. It worked with the first approach of handling edge cases in loop repair but I thought that might be risky. I'm attaching the patch and have added this to commitfest (https://commitfest.postgresql.org/patch/7519/). Please take a look. > Since this happens on master without the patch, I think it is > better handled as a separate backend bug, apart from this patch. Regarding this item, are you planning to work on this? If not, I am happy to take it up. Thanks & Regards, Nitin Motiani Google
From 80f8ac97fbc7a5adab172d36b3aba6b9dc300769 Mon Sep 17 00:00:00 2001 From: Nitin Motiani <[email protected]> Date: Wed, 21 Oct 2026 17:45:26 +0000 Subject: [PATCH v2] pg_dump: Dump differing generation expressions of inherited columns. Since v16, a child table's generated column may have a generation expression different from its parent's, and since v17, ALTER TABLE ... SET EXPRESSION can produce that state for a column the child inherited without a local definition. pg_dump did not handle this: it assumed generation expressions could only be dumped as part of the column definition, but it does not print inherited columns in the child's CREATE TABLE, so the child's expression was silently lost and the restored child used the parent's expression instead. Fix by dumping the child's expression as a separate ALTER TABLE ONLY ... SET EXPRESSION command when its column is not printed, as is done for defaults. That is needed in two cases: * The child's expression differs from its parent's. * The child's expression is the same as its parent's, but the parent's own expression is dumped as a separate command, for instance because the parent is itself a child with a differing expression. The child is then created before that command runs and would inherit the parent's old expression, so dump the child's expression too rather than depend on the order of the commands. Breaking a dependency loop can also make an expression separate, so this is decided after sorting the dumpable objects. Otherwise, the child's expression is still left out, since the child inherits it when it is created. Reported-by: Ke <[email protected]> Discussion: https://postgr.es/m/[email protected] Discussion: https://postgr.es/m/CAH5HC97y7PFE6rJh8kVDUszPxGQE030nZvyHu=wZT7FBkOV3qA@mail.gmail.com Backpatch-through: 17 --- src/bin/pg_dump/common.c | 89 +++++++++++++++- src/bin/pg_dump/pg_dump.c | 79 +++++++++----- src/bin/pg_dump/pg_dump.h | 10 ++ src/bin/pg_dump/t/002_pg_dump.pl | 172 +++++++++++++++++++++++++++++++ 4 files changed, 326 insertions(+), 24 deletions(-) diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c index cda9d73a9dd..b9816665203 100644 --- a/src/bin/pg_dump/common.c +++ b/src/bin/pg_dump/common.c @@ -473,6 +473,12 @@ flagInhIndexes(Archive *fout, TableInfo tblinfo[], int numTables) * we are in binary upgrade mode, we dump such expressions anyway because * in those cases inherited tables are recreated standalone first and then * reattached to the parent. (See also the logic in dumpTableSchema().) + * A child's expression that differs from the parent's is dumped, either + * as part of the column definition or, if the column is inherited without + * a local definition, as a separate ALTER TABLE ... SET EXPRESSION. A + * suppressed expression may still have to be dumped if the parent's own + * expression is not yet in place when the child is created; that is + * decided after sorting, by flagInhGenerationExprs(). * * modifies tblinfo */ @@ -620,6 +626,7 @@ flagInhAttrs(Archive *fout, DumpOptions *dopt, TableInfo *tblinfo, int numTables attrDef->adtable = tbinfo; attrDef->adnum = j + 1; attrDef->adef_expr = pg_strdup("NULL"); + attrDef->maybe_dump_later_if_altered_expression = false; /* Will column be dumped explicitly? */ if (shouldPrintColumn(dopt, tbinfo, j)) @@ -639,10 +646,90 @@ flagInhAttrs(Archive *fout, DumpOptions *dopt, TableInfo *tblinfo, int numTables tbinfo->attrdefs[j] = attrDef; } - /* No need to dump generation expression if it's inheritable */ + /* + * No need to dump generation expression if it's inheritable. + * + * This assumes that each parent has its final expression by the + * time the child table is created. That is not so if a parent's + * own expression is dumped as a separate ALTER TABLE ... SET + * EXPRESSION command, which we can only know after the dependency + * sort (see flagInhGenerationExprs()); so remember why we + * suppressed the expression. + */ if (foundSameGenerated && !foundDiffGenerated && !tbinfo->ispartition && !dopt->binary_upgrade) + { tbinfo->attrdefs[j]->dobj.dump = DUMP_COMPONENT_NONE; + tbinfo->attrdefs[j]->maybe_dump_later_if_altered_expression = true; + } + } + } +} + +/* + * flagInhGenerationExprs - + * dump generation expressions that cannot be inherited after all + * + * flagInhAttrs() suppresses a child's generation expression if it is the + * same as its parents', relying on CREATE TABLE ... INHERITS to copy it. + * That only works if each parent has its final expression when the child is + * created, which is not the case if the parent's own expression is dumped as + * a separate ALTER TABLE ... SET EXPRESSION command. Whether that happens + * is only settled after the dependency sort, since breaking a dependency + * loop can make an expression separate. For each such child, dump its own + * expression instead. The suppressed expressions have already been sorted + * like all other objects, so they will be emitted in a valid position. + * + * This must be called after sortDumpableObjects(), with the sorted array, and + * before dumping. In that order, each table comes after its parents, since a + * child table depends on them, so a parent's expression has been dealt with + * before its children are looked at. + */ +void +flagInhGenerationExprs(DumpableObject **dobjs, int numObjs) +{ + for (int i = 0; i < numObjs; i++) + { + TableInfo *tbinfo; + + if (dobjs[i]->objType != DO_TABLE) + continue; + tbinfo = (TableInfo *) dobjs[i]; + + if (!tbinfo->dobj.dump || tbinfo->numParents == 0 || + tbinfo->attrdefs == NULL) + continue; + + for (int j = 0; j < tbinfo->numatts; j++) + { + AttrDefInfo *attrDef = tbinfo->attrdefs[j]; + + if (attrDef == NULL || + !attrDef->maybe_dump_later_if_altered_expression) + continue; + + for (int k = 0; k < tbinfo->numParents; k++) + { + TableInfo *parent = tbinfo->parents[k]; + int inhAttrInd; + AttrDefInfo *parentDef; + + inhAttrInd = strInArray(tbinfo->attnames[j], + parent->attnames, + parent->numatts); + if (inhAttrInd < 0 || parent->attrdefs == NULL) + continue; + parentDef = parent->attrdefs[inhAttrInd]; + + if (parentDef != NULL && parentDef->separate && + (parentDef->dobj.dump & DUMP_COMPONENT_DEFINITION)) + { + /* dump the child's own expression after all */ + attrDef->dobj.dump = tbinfo->dobj.dump; + attrDef->maybe_dump_later_if_altered_expression = false; + break; + } + } } } } diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 69171d6b859..b14fdc803c9 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -1194,6 +1194,13 @@ main(int argc, char **argv) sortDumpableObjects(dobjs, numObjs, boundaryObjs[0].dumpId, boundaryObjs[1].dumpId); + /* + * Now that the sort has settled which generation expressions are dumped + * as separate commands, decide which inherited ones must be dumped after + * all. + */ + flagInhGenerationExprs(dobjs, numObjs); + /* * Create archive TOC entries for all the objects to be dumped, in a safe * order. @@ -9594,6 +9601,7 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables) attrdefs[j].adtable = tbinfo; attrdefs[j].adnum = adnum; attrdefs[j].adef_expr = pg_strdup(adsrc); + attrdefs[j].maybe_dump_later_if_altered_expression = false; attrdefs[j].dobj.name = pg_strdup(tbinfo->dobj.name); attrdefs[j].dobj.namespace = tbinfo->dobj.namespace; @@ -9607,19 +9615,7 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables) * is to put it into the CREATE command, but in some cases that's * not possible. */ - if (tbinfo->attgenerated[adnum - 1]) - { - /* - * Column generation expressions cannot be dumped separately, - * because there is no syntax for it. By setting separate to - * false here we prevent the "default" from being processed as - * its own dumpable object. Later, flagInhAttrs() will mark - * it as not to be dumped at all, if possible (that is, if it - * can be inherited from a parent). - */ - attrdefs[j].separate = false; - } - else if (tbinfo->relkind == RELKIND_VIEW) + if (tbinfo->relkind == RELKIND_VIEW) { /* * Defaults on a VIEW must always be dumped as separate ALTER @@ -9629,7 +9625,13 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables) } else if (!shouldPrintColumn(dopt, tbinfo, adnum - 1)) { - /* column will be suppressed, print default separately */ + /* + * Column will be suppressed, print default separately. For a + * generation expression, this means ALTER TABLE ... SET + * EXPRESSION; but flagInhAttrs() will usually mark it as not + * to be dumped at all, since an inherited column normally has + * the same expression as its parent. + */ attrdefs[j].separate = true; } else @@ -18097,6 +18099,13 @@ dumpTableAttach(Archive *fout, const TableAttachInfo *attachinfo) /* * dumpAttrDef --- dump an attribute's default-value declaration + * + * For a generated column, this emits ALTER TABLE ... SET EXPRESSION rather + * than SET DEFAULT. We get here for such a column if it is inherited without + * a local definition (so it's not printed in CREATE TABLE) and its expression + * is not the one it would inherit when the table is created (see + * flagInhAttrs() and flagInhGenerationExprs()), or if the expression had to + * be dumped separately to break a dependency loop. */ static void dumpAttrDef(Archive *fout, const AttrDefInfo *adinfo) @@ -18114,9 +18123,17 @@ dumpAttrDef(Archive *fout, const AttrDefInfo *adinfo) if (!dopt->dumpSchema) return; - /* Skip if not "separate"; it was dumped in the table's definition */ + /* + * Skip if not "separate"; it was dumped in the table's definition. That + * requires the column itself to be printed there, else the default (or + * generation expression) would be lost entirely. + */ if (!adinfo->separate) + { + Assert(!(adinfo->dobj.dump & DUMP_COMPONENT_DEFINITION) || + shouldPrintColumn(dopt, tbinfo, adnum - 1)); return; + } q = createPQExpBuffer(); delq = createPQExpBuffer(); @@ -18125,17 +18142,33 @@ dumpAttrDef(Archive *fout, const AttrDefInfo *adinfo) foreign = tbinfo->relkind == RELKIND_FOREIGN_TABLE ? "FOREIGN " : ""; - appendPQExpBuffer(q, - "ALTER %sTABLE ONLY %s ALTER COLUMN %s SET DEFAULT %s;\n", - foreign, qualrelname, fmtId(tbinfo->attnames[adnum - 1]), - adinfo->adef_expr); + if (tbinfo->attgenerated[adnum - 1]) + { + /* + * No drop statement: DROP EXPRESSION would turn the column into a + * regular column (and isn't allowed on an inherited column anyway). + * The expression goes away with the table. + */ + appendPQExpBuffer(q, + "ALTER %sTABLE ONLY %s ALTER COLUMN %s SET EXPRESSION AS (%s);\n", + foreign, qualrelname, fmtId(tbinfo->attnames[adnum - 1]), + adinfo->adef_expr); + } + else + { + appendPQExpBuffer(q, + "ALTER %sTABLE ONLY %s ALTER COLUMN %s SET DEFAULT %s;\n", + foreign, qualrelname, fmtId(tbinfo->attnames[adnum - 1]), + adinfo->adef_expr); - appendPQExpBuffer(delq, "ALTER %sTABLE %s ALTER COLUMN %s DROP DEFAULT;\n", - foreign, qualrelname, - fmtId(tbinfo->attnames[adnum - 1])); + appendPQExpBuffer(delq, "ALTER %sTABLE %s ALTER COLUMN %s DROP DEFAULT;\n", + foreign, qualrelname, + fmtId(tbinfo->attnames[adnum - 1])); + } tag = psprintf("%s %s", tbinfo->dobj.name, tbinfo->attnames[adnum - 1]); + /* delq is empty for a generated column (see above): no drop statement */ if (adinfo->dobj.dump & DUMP_COMPONENT_DEFINITION) ArchiveEntry(fout, adinfo->dobj.catId, adinfo->dobj.dumpId, ARCHIVE_OPTS(.tag = tag, @@ -18144,7 +18177,7 @@ dumpAttrDef(Archive *fout, const AttrDefInfo *adinfo) .description = "DEFAULT", .section = SECTION_PRE_DATA, .createStmt = q->data, - .dropStmt = delq->data)); + .dropStmt = delq->len > 0 ? delq->data : NULL)); pfree(tag); destroyPQExpBuffer(q); diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h index 4dcb6c854e8..58c7e663810 100644 --- a/src/bin/pg_dump/pg_dump.h +++ b/src/bin/pg_dump/pg_dump.h @@ -404,6 +404,15 @@ typedef struct _attrDefInfo int adnum; char *adef_expr; /* decompiled DEFAULT expression */ bool separate; /* true if must dump as separate item */ + + /* + * True if this is a generation expression that flagInhAttrs() chose not + * to dump, because CREATE TABLE ... INHERITS will copy it from the + * parents. It may still have to be dumped later, if a parent's own + * expression turns out to be dumped as a separate ALTER TABLE command; + * see flagInhGenerationExprs(). + */ + bool maybe_dump_later_if_altered_expression; } AttrDefInfo; typedef struct _tableDataInfo @@ -754,6 +763,7 @@ typedef struct _SubRelInfo */ extern TableInfo *getSchemaData(Archive *fout, int *numTablesPtr); +extern void flagInhGenerationExprs(DumpableObject **dobjs, int numObjs); extern void AssignDumpId(DumpableObject *dobj); extern void recordAdditionalCatalogID(CatalogId catId, DumpableObject *dobj); diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl index c89d699e816..9c29ced41eb 100644 --- a/src/bin/pg_dump/t/002_pg_dump.pl +++ b/src/bin/pg_dump/t/002_pg_dump.pl @@ -4006,6 +4006,178 @@ my %tests = ( }, }, + # Inherited (non-local) generated columns whose expressions were changed + # with SET EXPRESSION must have those expressions dumped separately. + 'CREATE TABLE test_table_generated_child3 (differing expressions)' => { + create_order => 4, + create_sql => 'CREATE TABLE dump_test.test_table_generated_child3 () + INHERITS (dump_test.test_table_generated); + ALTER TABLE dump_test.test_table_generated_child3 + ALTER COLUMN col2 SET EXPRESSION AS (col1 * 4), + ALTER COLUMN col3 SET EXPRESSION AS (col1 * 5);', + regexp => qr/^ + \QCREATE TABLE dump_test.test_table_generated_child3 (\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, + only_dump_measurement => 1, + }, + }, + + 'ALTER TABLE test_table_generated_child3 SET EXPRESSION (stored)' => { + regexp => qr/^ + \QALTER TABLE ONLY dump_test.test_table_generated_child3 ALTER COLUMN col2 SET EXPRESSION AS ((col1 * 4));\E\n + /xm, + like => + { %full_runs, %dump_test_schema_runs, section_pre_data => 1, }, + unlike => { + binary_upgrade => 1, + exclude_dump_test_schema => 1, + only_dump_measurement => 1, + }, + }, + + 'ALTER TABLE test_table_generated_child3 SET EXPRESSION (virtual)' => { + regexp => qr/^ + \QALTER TABLE ONLY dump_test.test_table_generated_child3 ALTER COLUMN col3 SET EXPRESSION AS ((col1 * 5));\E\n + /xm, + like => + { %full_runs, %dump_test_schema_runs, section_pre_data => 1, }, + unlike => { + binary_upgrade => 1, + exclude_dump_test_schema => 1, + only_dump_measurement => 1, + }, + }, + + # A grandchild with the same expression as its parent cannot inherit it + # when the parent's own expression is dumped separately, since the + # grandchild would then be created before that command runs. + 'ALTER TABLE test_table_generated_grandchild SET EXPRESSION' => { + create_order => 5, + create_sql => 'CREATE TABLE dump_test.test_table_generated_grandchild () + INHERITS (dump_test.test_table_generated_child3);', + regexp => qr/^ + \QALTER TABLE ONLY dump_test.test_table_generated_grandchild ALTER COLUMN col2 SET EXPRESSION AS ((col1 * 4));\E\n + /xm, + like => + { %full_runs, %dump_test_schema_runs, section_pre_data => 1, }, + unlike => { + binary_upgrade => 1, + exclude_dump_test_schema => 1, + only_dump_measurement => 1, + }, + }, + + # Same, where the parent's expression calls a function that depends on + # the grandchild. (In binary upgrade mode, the grandchild's expression + # is dumped separately, too, but to break the dependency loop between + # it and the function.) + 'ALTER TABLE test_table_generated_fn_child2 SET EXPRESSION' => { + create_order => 4, + create_sql => 'CREATE TABLE dump_test.test_table_generated_fn ( + x int, + g int generated always as (x * 2) stored + ); + CREATE TABLE dump_test.test_table_generated_fn_child1 () + INHERITS (dump_test.test_table_generated_fn); + CREATE TABLE dump_test.test_table_generated_fn_child2 () + INHERITS (dump_test.test_table_generated_fn_child1); + CREATE FUNCTION dump_test.generated_fn(int) RETURNS int + IMMUTABLE LANGUAGE sql + BEGIN ATOMIC + SELECT $1 * 3 + (SELECT count(*)::int + FROM dump_test.test_table_generated_fn_child2 WHERE false); + END; + ALTER TABLE dump_test.test_table_generated_fn_child1 + ALTER COLUMN g SET EXPRESSION AS (dump_test.generated_fn(x));', + regexp => qr/^ + \QALTER TABLE ONLY dump_test.test_table_generated_fn_child2 ALTER COLUMN g SET EXPRESSION AS (dump_test.generated_fn(x));\E\n + /xm, + like => + { %full_runs, %dump_test_schema_runs, section_pre_data => 1, }, + unlike => { + exclude_dump_test_schema => 1, + only_dump_measurement => 1, + }, + }, + + # Here the parent's column is local, but its expression has to be dumped + # separately to break a dependency loop, which is only decided while + # sorting; the child must then get its own expression, too. + 'ALTER TABLE test_table_generated_loop_child SET EXPRESSION' => { + create_order => 4, + create_sql => 'CREATE TABLE dump_test.test_table_generated_loop_parent ( + a int, + g int generated always as (a * 2) stored + ); + CREATE TABLE dump_test.test_table_generated_loop (a int); + CREATE FUNCTION dump_test.generated_loop_fn(int) RETURNS int + IMMUTABLE LANGUAGE sql + BEGIN ATOMIC + SELECT $1 * 3 + 0 * (SELECT count(*)::int + FROM dump_test.test_table_generated_loop); + END; + ALTER TABLE dump_test.test_table_generated_loop + ADD COLUMN g int + GENERATED ALWAYS AS (dump_test.generated_loop_fn(a)) STORED; + ALTER TABLE dump_test.test_table_generated_loop + INHERIT dump_test.test_table_generated_loop_parent; + CREATE TABLE dump_test.test_table_generated_loop_child () + INHERITS (dump_test.test_table_generated_loop);', + regexp => qr/^ + \QALTER TABLE ONLY dump_test.test_table_generated_loop_child ALTER COLUMN g SET EXPRESSION AS (dump_test.generated_loop_fn(a));\E\n + /xm, + like => + { %full_runs, %dump_test_schema_runs, section_pre_data => 1, }, + unlike => { + binary_upgrade => 1, + exclude_dump_test_schema => 1, + only_dump_measurement => 1, + }, + }, + + # The last table of this chain is created first and attached later, so + # that its OID is lower than its parent's; its expression must still be + # dumped, since its parent's is. + 'CREATE TABLE test_table_generated_chain_late' => { + create_order => 4, + create_sql => 'CREATE TABLE dump_test.test_table_generated_chain_late (x int); + CREATE TABLE dump_test.test_table_generated_chain ( + x int, + g int generated always as (x * 2) stored + ); + CREATE TABLE dump_test.test_table_generated_chain_child () + INHERITS (dump_test.test_table_generated_chain); + CREATE TABLE dump_test.test_table_generated_chain_grandchild () + INHERITS (dump_test.test_table_generated_chain_child); + ALTER TABLE dump_test.test_table_generated_chain_child + ALTER COLUMN g SET EXPRESSION AS (x * 3); + ALTER TABLE dump_test.test_table_generated_chain_late + ADD COLUMN g int GENERATED ALWAYS AS (x * 3) STORED; + ALTER TABLE dump_test.test_table_generated_chain_late + INHERIT dump_test.test_table_generated_chain_grandchild;', + regexp => qr/^ + \QCREATE TABLE dump_test.test_table_generated_chain_late (\E\n + \s+\Qx integer,\E\n + \s+\Qg integer GENERATED ALWAYS AS ((x * 3)) STORED\E\n + \)\n + \QINHERITS (dump_test.test_table_generated_chain_grandchild);\E\n + /xms, + like => + { %full_runs, %dump_test_schema_runs, section_pre_data => 1, }, + unlike => { + binary_upgrade => 1, + exclude_dump_test_schema => 1, + only_dump_measurement => 1, + }, + }, + 'CREATE TABLE table_with_stats' => { create_order => 98, create_sql => 'CREATE TABLE dump_test.table_index_stats ( --
