Fix logic buglets in pg_dump's flagInhAttrs().

As it stands, flagInhAttrs() can make changes in table properties that
change decisions made at other tables during other iterations of its
loop.  This is a pretty bad idea, since we visit the tables in OID
order which is not necessarily related to inheritance relationships.
So far as I can tell, the consequences are just cosmetic: we might
dump DEFAULT or GENERATED expressions that we don't really need to
because they match properties of the parent.  Nonetheless, it's buggy,
and somebody might someday add functionality here that fails less
benignly when the traversal order varies.

One issue is that when we decide we needn't dump a particular
GENERATED expression, we physically unlink the struct for it,
so that it will now look like the table has no such expression,
causing the wrong choice to be made at any child visited later.
We can improve that by instead clearing the dobj.dump flag,
and taking care to check that flag when it comes time to dump
the expression or not.

The other problem is that if we decide we need to fake up a DEFAULT
NULL clause to override a default that would otherwise get inherited,
we modify the data structure in the reverse fashion, creating an
attrdefs entry where there hadn't been one.  It's harder to avoid
doing that, but since the backend won't report a plain "DEFAULT NULL"
property we can modify the code to recognize ones we just added.

Add some commentary to perhaps forestall future mistakes of the
same ilk.

Since the effects of this seem only cosmetic, no back-patch.

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

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 could change decisions made at
+	 * 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;
 		}
 	}
 }

src/bin/pg_dump/pg_dump.c

@@ -8525,9 +8525,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
+				 * its own dumpable object.  Later, flagInhAttrs() will mark
-				 * from the table if possible (that is, if it can be inherited
+				 * it as not to be dumped at all, if possible (that is, if it
-				 * from a parent).
+				 * can be inherited from a parent).
 				 */
 				attrdefs[j].separate = false;
 			}
@@ -15345,9 +15345,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);
 
 					/*