Improve handling of inherited GENERATED expressions.

tglsfdc · tglsfdc · commit 8bf6ec3ba3a4 · 2023-01-11T15:55:02.000-05:00
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 3f7836f, 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
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
@@ -325,27 +325,54 @@ CREATE TABLE people (
      </para>
     </listitem>
     <listitem>
-     <para>For inheritance:</para>
+     <para>For inheritance and partitioning:</para>
      <itemizedlist>
       <listitem>
        <para>
-        If a parent column is a generated column, a child column must also be
-        a generated column using the same expression.  In the definition of
-        the child column, leave off the <literal>GENERATED</literal> clause,
-        as it will be copied from the parent.
+        If a parent column is a generated column, its child column must also
+        be a generated column; however, the child column can have a
+        different generation expression.  The generation expression that is
+        actually applied during insert or update of a row is the one
+        associated with the table that the row is physically in.
+        (This is unlike the behavior for column defaults: for those, the
+        default value associated with the table named in the query applies.)
        </para>
       </listitem>
       <listitem>
        <para>
-        In case of multiple inheritance, if one parent column is a generated
-        column, then all parent columns must be generated columns and with the
-        same expression.
+        If a parent column is not a generated column, its child column must
+        not be generated either.
+       </para>
+      </listitem>
+      <listitem>
+       <para>
+        For inherited tables, if you write a child column definition without
+        any <literal>GENERATED</literal> clause in <command>CREATE TABLE
+        ... INHERITS</command>, then its <literal>GENERATED</literal> clause
+        will automatically be copied from the parent.  <command>ALTER TABLE
+        ... INHERIT</command> will insist that parent and child columns
+        already match as to generation status, but it will not require their
+        generation expressions to match.
        </para>
       </listitem>
       <listitem>
        <para>
-        If a parent column is not a generated column, a child column may be
-        defined to be a generated column or not.
+        Similarly for partitioned tables, if you write a child column
+        definition without any <literal>GENERATED</literal> clause
+        in <command>CREATE TABLE ... PARTITION OF</command>, then
+        its <literal>GENERATED</literal> clause will automatically be copied
+        from the parent.  <command>ALTER TABLE ... ADD PARTITION</command>
+        will insist that parent and child columns already match as to
+        generation status, but it will not require their generation
+        expressions to match.
+       </para>
+      </listitem>
+      <listitem>
+       <para>
+        In case of multiple inheritance, if one parent column is a generated
+        column, then all parent columns must be generated columns.  If they
+        do not all have the same generation expression, then the desired
+        expression for the child must be specified explicitly.
        </para>
       </listitem>
      </itemizedlist>
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
@@ -2320,8 +2320,12 @@ storage_name(char c)
  *		(3) If conflicting defaults are inherited from different parents
  *			(and not overridden by the child), an error is raised.
  *		(4) Otherwise the inherited default is used.
- *		Rule (3) is new in Postgres 7.1; in earlier releases you got a
- *		rather arbitrary choice of which parent default to use.
+ *
+ *		Note that the default-value infrastructure is used for generated
+ *		columns' expressions too, so most of the preceding paragraph applies
+ *		to generation expressions too.  We insist that a child column be
+ *		generated if and only if its parent(s) are, but it need not have
+ *		the same generation expression.
  *----------
  */
 static List *
@@ -2659,7 +2663,7 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
 			}
 
 			/*
-			 * Locate default if any
+			 * Locate default/generation expression if any
 			 */
 			if (attribute->atthasdef)
 			{
@@ -2923,23 +2927,20 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
 				/*
 				 * Check for conflicts related to generated columns.
 				 *
-				 * If the parent column is generated, the child column must be
-				 * unadorned and will be made a generated column.  (We could
-				 * in theory allow the child column definition specifying the
-				 * exact same generation expression, but that's a bit
-				 * complicated to implement and doesn't seem very useful.)  We
-				 * also check that the child column doesn't specify a default
-				 * value or identity, which matches the rules for a single
-				 * column in parse_util.c.
+				 * If the parent column is generated, the child column will be
+				 * made a generated column if it isn't already.  If it is a
+				 * generated column, we'll take its generation expression in
+				 * preference to the parent's.  We must check that the child
+				 * column doesn't specify a default value or identity, which
+				 * matches the rules for a single column in parse_util.c.
+				 *
+				 * Conversely, if the parent column is not generated, the
+				 * child column can't be either.  (We used to allow that, but
+				 * it results in being able to override the generation
+				 * expression via UPDATEs through the parent.)
 				 */
 				if (def->generated)
 				{
-					if (newdef->generated)
-						ereport(ERROR,
-								(errcode(ERRCODE_INVALID_COLUMN_DEFINITION),
-								 errmsg("child column \"%s\" specifies generation expression",
-										def->colname),
-								 errhint("Omit the generation expression in the definition of the child table column to inherit the generation expression from the parent table.")));
 					if (newdef->raw_default && !newdef->generated)
 						ereport(ERROR,
 								(errcode(ERRCODE_INVALID_COLUMN_DEFINITION),
@@ -2951,15 +2952,14 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
 								 errmsg("column \"%s\" inherits from generated column but specifies identity",
 										def->colname)));
 				}
-
-				/*
-				 * If the parent column is not generated, then take whatever
-				 * the child column definition says.
-				 */
 				else
 				{
 					if (newdef->generated)
-						def->generated = newdef->generated;
+						ereport(ERROR,
+								(errcode(ERRCODE_INVALID_COLUMN_DEFINITION),
+								 errmsg("child column \"%s\" specifies generation expression",
+										def->colname),
+								 errhint("A child table column cannot be generated unless its parent column is.")));
 				}
 
 				/* If new def has a default, override previous default */
@@ -2994,8 +2994,8 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
 	/*
 	 * Now that we have the column definition list for a partition, we can
 	 * check whether the columns referenced in the column constraint specs
-	 * actually exist.  Also, we merge NOT NULL and defaults into each
-	 * corresponding column definition.
+	 * actually exist.  Also, we merge parent's NOT NULL constraints and
+	 * defaults into each corresponding column definition.
 	 */
 	if (is_partition)
 	{
@@ -3014,6 +3014,19 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
 					found = true;
 					coldef->is_not_null |= restdef->is_not_null;
 
+					/*
+					 * As above, reject generated columns in partitions that
+					 * are not generated in the parent.
+					 */
+					if (restdef->generated && !coldef->generated)
+						ereport(ERROR,
+								(errcode(ERRCODE_INVALID_COLUMN_DEFINITION),
+								 errmsg("child column \"%s\" specifies generation expression",
+										restdef->colname),
+								 errhint("A child table column cannot be generated unless its parent column is.")));
+					/* Other way around should have been dealt with above */
+					Assert(!(coldef->generated && !restdef->generated));
+
 					/*
 					 * Override the parent's default value for this column
 					 * (coldef->cooked_default) with the partition's local
@@ -3058,7 +3071,8 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
 					ereport(ERROR,
 							(errcode(ERRCODE_INVALID_COLUMN_DEFINITION),
 							 errmsg("column \"%s\" inherits conflicting generation expressions",
-									def->colname)));
+									def->colname),
+							 errhint("To resolve the conflict, specify a generation expression explicitly.")));
 				else
 					ereport(ERROR,
 							(errcode(ERRCODE_INVALID_COLUMN_DEFINITION),
@@ -15038,64 +15052,18 @@ MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel)
 								attributeName)));
 
 			/*
-			 * If parent column is generated, child column must be, too.
+			 * Child column must be generated if and only if parent column is.
 			 */
 			if (attribute->attgenerated && !childatt->attgenerated)
 				ereport(ERROR,
 						(errcode(ERRCODE_DATATYPE_MISMATCH),
 						 errmsg("column \"%s\" in child table must be a generated column",
 								attributeName)));
-
-			/*
-			 * Check that both generation expressions match.
-			 *
-			 * The test we apply is to see whether they reverse-compile to the
-			 * same source string.  This insulates us from issues like whether
-			 * attributes have the same physical column numbers in parent and
-			 * child relations.  (See also constraints_equivalent().)
-			 */
-			if (attribute->attgenerated && childatt->attgenerated)
-			{
-				TupleConstr *child_constr = child_rel->rd_att->constr;
-				TupleConstr *parent_constr = parent_rel->rd_att->constr;
-				char	   *child_expr = NULL;
-				char	   *parent_expr = NULL;
-
-				Assert(child_constr != NULL);
-				Assert(parent_constr != NULL);
-
-				for (int i = 0; i < child_constr->num_defval; i++)
-				{
-					if (child_constr->defval[i].adnum == childatt->attnum)
-					{
-						child_expr =
-							TextDatumGetCString(DirectFunctionCall2(pg_get_expr,
-																	CStringGetTextDatum(child_constr->defval[i].adbin),
-																	ObjectIdGetDatum(child_rel->rd_id)));
-						break;
-					}
-				}
-				Assert(child_expr != NULL);
-
-				for (int i = 0; i < parent_constr->num_defval; i++)
-				{
-					if (parent_constr->defval[i].adnum == attribute->attnum)
-					{
-						parent_expr =
-							TextDatumGetCString(DirectFunctionCall2(pg_get_expr,
-																	CStringGetTextDatum(parent_constr->defval[i].adbin),
-																	ObjectIdGetDatum(parent_rel->rd_id)));
-						break;
-					}
-				}
-				Assert(parent_expr != NULL);
-
-				if (strcmp(child_expr, parent_expr) != 0)
-					ereport(ERROR,
-							(errcode(ERRCODE_DATATYPE_MISMATCH),
-							 errmsg("column \"%s\" in child table has a conflicting generation expression",
-									attributeName)));
-			}
+			if (childatt->attgenerated && !attribute->attgenerated)
+				ereport(ERROR,
+						(errcode(ERRCODE_DATATYPE_MISMATCH),
+						 errmsg("column \"%s\" in child table must not be a generated column",
+								attributeName)));
 
 			/*
 			 * OK, bump the child column's inheritance count.  (If we fail
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
@@ -740,11 +740,6 @@ transformColumnDefinition(CreateStmtContext *cxt, ColumnDef *column)
 					ereport(ERROR,
 							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 							 errmsg("generated columns are not supported on typed tables")));
-				if (cxt->partbound)
-					ereport(ERROR,
-							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-							 errmsg("generated columns are not supported on partitions")));
-
 				if (saw_generated)
 					ereport(ERROR,
 							(errcode(ERRCODE_SYNTAX_ERROR),
diff --git 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,15 +498,17 @@ 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])
 				continue;
 
 			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;
 		}
 	}
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
@@ -8529,17 +8529,11 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
 			{
 				/*
 				 * 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
+				 * 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 when it detects that it belongs to an
-				 * inherited column.
+				 * from the table if possible (that is, if it can be inherited
+				 * from a parent).
 				 */
 				attrdefs[j].separate = false;
 			}
diff --git a/src/test/regress/expected/generated.out b/src/test/regress/expected/generated.out
diff --git a/src/test/regress/sql/generated.sql b/src/test/regress/sql/generated.sql
