While starting to poke at the hashed-enum-partition-key problem
recently discussed [1], I realized that pg_dump's flagInhAttrs()
function has a logic issue: its loop changes state that will be
inspected in other iterations of the loop, and there's no guarantee
about the order in which related tables will be visited. Typically
we'll see parent tables before children because parents tend to have
smaller OIDs, but there are plenty of ways in which that might not
be true.
As far as I can tell, the implications of this are just cosmetic:
we might dump DEFAULT or GENERATED expressions that we don't really
need to because they match properties of the parent. Still, it's
buggy, and somebody might carelessly extend the logic in a way that
introduces more-serious bugs. PFA a proposed patch.
regards, tom lane
[1] https://www.postgresql.org/message-id/1376149.1675268279%40sss.pgh.pa.us
diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index a43f2e5553..cbf00adbae 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -471,6 +471,13 @@ flagInhAttrs(DumpOptions *dopt, TableInfo *tblinfo, int numTables)
j,
k;
+ /*
+ * We scan the tables in OID order, since that's how tblinfo[] is sorted.
+ * Hence we will typically visit parents before their children --- but
+ * that is *not* guaranteed. Thus this loop must be careful that it does
+ * not alter table properties in a way that would change decisions made at
+ * their child tables during other iterations.
+ */
for (i = 0; i < numTables; i++)
{
TableInfo *tbinfo = &(tblinfo[i]);
@@ -519,15 +526,18 @@ flagInhAttrs(DumpOptions *dopt, TableInfo *tblinfo, int numTables)
parent->numatts);
if (inhAttrInd >= 0)
{
+ AttrDefInfo *parentDef = parent->attrdefs[inhAttrInd];
+
foundNotNull |= parent->notnull[inhAttrInd];
- foundDefault |= (parent->attrdefs[inhAttrInd] != NULL &&
+ foundDefault |= (parentDef != NULL &&
+ strcmp(parentDef->adef_expr, "NULL") != 0 &&
!parent->attgenerated[inhAttrInd]);
if (parent->attgenerated[inhAttrInd])
{
/* these pointer nullness checks are just paranoia */
- if (parent->attrdefs[inhAttrInd] != NULL &&
+ if (parentDef != NULL &&
tbinfo->attrdefs[j] != NULL &&
- strcmp(parent->attrdefs[inhAttrInd]->adef_expr,
+ strcmp(parentDef->adef_expr,
tbinfo->attrdefs[j]->adef_expr) == 0)
foundSameGenerated = true;
else
@@ -539,7 +549,14 @@ flagInhAttrs(DumpOptions *dopt, TableInfo *tblinfo, int numTables)
/* Remember if we found inherited NOT NULL */
tbinfo->inhNotNull[j] = foundNotNull;
- /* Manufacture a DEFAULT NULL clause if necessary */
+ /*
+ * Manufacture a DEFAULT NULL clause if necessary. This breaks
+ * the advice given above to avoid changing state that might get
+ * inspected in other loop iterations. We prevent trouble by
+ * having the foundDefault test above check whether adef_expr is
+ * "NULL", so that it will reach the same conclusion before or
+ * after this is done.
+ */
if (foundDefault && tbinfo->attrdefs[j] == NULL)
{
AttrDefInfo *attrDef;
@@ -575,10 +592,10 @@ flagInhAttrs(DumpOptions *dopt, TableInfo *tblinfo, int numTables)
tbinfo->attrdefs[j] = attrDef;
}
- /* Remove generation expression from child if possible */
+ /* No need to dump generation expression if it's inheritable */
if (foundSameGenerated && !foundDiffGenerated &&
!tbinfo->ispartition && !dopt->binary_upgrade)
- tbinfo->attrdefs[j] = NULL;
+ tbinfo->attrdefs[j]->dobj.dump = DUMP_COMPONENT_NONE;
}
}
}
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 527c7651ab..8cd8f10d5e 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -8531,9 +8531,9 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
* 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, and flagInhAttrs() will remove it
- * from the table if possible (that is, if it can be inherited
- * from a parent).
+ * 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;
}
@@ -15351,9 +15351,11 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
bool print_notnull;
/*
- * Default value --- suppress if to be printed separately.
+ * Default value --- suppress if to be printed separately
+ * or not at all.
*/
print_default = (tbinfo->attrdefs[j] != NULL &&
+ tbinfo->attrdefs[j]->dobj.dump &&
!tbinfo->attrdefs[j]->separate);
/*
