Improve handling of inherited GENERATED expressions.

In both partitioning and traditional inheritance, require child
columns to be GENERATED if and only if their parent(s) are.
Formerly we allowed the case of an inherited column being
GENERATED when its parent isn't, but that results in inconsistent
behavior: the column can be directly updated through an UPDATE
on the parent table, leading to it containing a user-supplied
value that might not match the generation expression.  This also
fixes an oversight that we enforced partition-key-columns-can't-
be-GENERATED against parent tables, but not against child tables
that were dynamically attached to them.

Also, remove the restriction that the child's generation expression
be equivalent to the parent's.  In the wake of commit 3f7836ff6,
there doesn't seem to be any reason that we need that restriction,
since generation expressions are always computed per-table anyway.
By removing this, we can also allow a child to merge multiple
inheritance parents with inconsistent generation expressions, by
overriding them with its own expression, much as we've long allowed
for DEFAULT expressions.

Since we're rejecting a case that we used to accept, this doesn't
seem like a back-patchable change.  Given the lack of field
complaints about the inconsistent behavior, it's likely that no
one is doing this anyway, but we won't change it in minor releases.

Amit Langote and Tom Lane

Discussion: https://postgr.es/m/2793383.1672944799@sss.pgh.pa.us

1 files changed, 29 insertions, 14 deletions

diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index 409bc9b8e20..a43f2e5553e 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -452,14 +452,15 @@ flagInhIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
  *   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.  Exceptions: If it's a partition or we are in binary upgrade
- *   mode, we dump them because in those cases inherited tables are recreated
- *   standalone first and then reattached to the parent.  (See also the logic
- *   in dumpTableSchema().)  In that situation, the generation expressions
- *   must match the parent, enforced by ALTER TABLE.
+ * - Detect child columns that have a generation expression and all their
+ *   parents also have the same generation expression, and if so suppress the
+ *   child's expression.  The child will inherit the generation expression
+ *   automatically, so there's no need to dump it.  This improves the dump's
+ *   compatibility with pre-v16 servers, which didn't allow the child's
+ *   expression to be given explicitly.  Exceptions: If it's a partition or
+ *   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().)
  *
  * modifies tblinfo
  */
@@ -497,7 +498,8 @@ 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 */
+			bool		foundSameGenerated; /* Found matching GENERATED */
+			bool		foundDiffGenerated; /* Found non-matching GENERATED */
 
 			/* no point in examining dropped columns */
 			if (tbinfo->attisdropped[j])
@@ -505,7 +507,8 @@ flagInhAttrs(DumpOptions *dopt, TableInfo *tblinfo, int numTables)
 
 			foundNotNull = false;
 			foundDefault = false;
-			foundGenerated = false;
+			foundSameGenerated = false;
+			foundDiffGenerated = false;
 			for (k = 0; k < numParents; k++)
 			{
 				TableInfo  *parent = parents[k];
@@ -517,8 +520,19 @@ flagInhAttrs(DumpOptions *dopt, TableInfo *tblinfo, int numTables)
 				if (inhAttrInd >= 0)
 				{
 					foundNotNull |= parent->notnull[inhAttrInd];
-					foundDefault |= (parent->attrdefs[inhAttrInd] != NULL && !parent->attgenerated[inhAttrInd]);
-					foundGenerated |= parent->attgenerated[inhAttrInd];
+					foundDefault |= (parent->attrdefs[inhAttrInd] != NULL &&
+									 !parent->attgenerated[inhAttrInd]);
+					if (parent->attgenerated[inhAttrInd])
+					{
+						/* these pointer nullness checks are just paranoia */
+						if (parent->attrdefs[inhAttrInd] != NULL &&
+							tbinfo->attrdefs[j] != NULL &&
+							strcmp(parent->attrdefs[inhAttrInd]->adef_expr,
+								   tbinfo->attrdefs[j]->adef_expr) == 0)
+							foundSameGenerated = true;
+						else
+							foundDiffGenerated = true;
+					}
 				}
 			}
 
@@ -561,8 +575,9 @@ flagInhAttrs(DumpOptions *dopt, TableInfo *tblinfo, int numTables)
 				tbinfo->attrdefs[j] = attrDef;
 			}
 
-			/* Remove generation expression from child */
-			if (foundGenerated && !tbinfo->ispartition && !dopt->binary_upgrade)
+			/* Remove generation expression from child if possible */
+			if (foundSameGenerated && !foundDiffGenerated &&
+				!tbinfo->ispartition && !dopt->binary_upgrade)
 				tbinfo->attrdefs[j] = NULL;
 		}
 	}