Fix pg_dump for better handling of inherited columns.

Revise pg_dump's handling of inherited columns, which was last looked at
seriously in 2001, to eliminate several misbehaviors associated with
inherited default expressions and NOT NULL flags.  In particular make sure
that a column is printed in a child table's CREATE TABLE command if and
only if it has attislocal = true; the former behavior would sometimes cause
a column to become marked attislocal when it was not so marked in the
source database.  Also, stop relying on textual comparison of default
expressions to decide if they're inherited; instead, don't use
default-expression inheritance at all, but just install the default
explicitly at each level of the hierarchy.  This fixes the
search-path-related misbehavior recently exhibited by Chester Young, and
also removes some dubious assumptions about the order in which ALTER TABLE
SET DEFAULT commands would be executed.

Back-patch to all supported branches.

3 files changed, 147 insertions, 150 deletions

diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index fd7ce1d8f7d..fd8709f1d59 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -269,7 +269,13 @@ flagInhTables(TableInfo *tblinfo, int numTables,
 
 /* flagInhAttrs -
  *	 for each dumpable table in tblinfo, flag its inherited attributes
- * so when we dump the table out, we don't dump out the inherited attributes
+ *
+ * What we need to do here is detect child columns that inherit NOT NULL
+ * bits from their parents (so that we needn't specify that again for the
+ * child) and child columns that have DEFAULT NULL when their parents had
+ * some non-null default.  In the latter case, we make up a dummy AttrDefInfo
+ * object so that we'll correctly emit the necessary DEFAULT NULL clause;
+ * otherwise the backend will apply an inherited default to the column.
  *
  * modifies tblinfo
  */
@@ -285,7 +291,6 @@ flagInhAttrs(TableInfo *tblinfo, int numTables)
 		TableInfo  *tbinfo = &(tblinfo[i]);
 		int			numParents;
 		TableInfo **parents;
-		TableInfo  *parent;
 
 		/* Sequences and views never have parents */
 		if (tbinfo->relkind == RELKIND_SEQUENCE ||
@@ -302,132 +307,70 @@ flagInhAttrs(TableInfo *tblinfo, int numTables)
 		if (numParents == 0)
 			continue;			/* nothing to see here, move along */
 
-		/*----------------------------------------------------------------
-		 * For each attr, check the parent info: if no parent has an attr
-		 * with the same name, then it's not inherited. If there *is* an
-		 * attr with the same name, then only dump it if:
-		 *
-		 * - it is NOT NULL and zero parents are NOT NULL
-		 *	 OR
-		 * - it has a default value AND the default value does not match
-		 *	 all parent default values, or no parents specify a default.
-		 *
-		 * See discussion on -hackers around 2-Apr-2001.
-		 *----------------------------------------------------------------
-		 */
+		/* For each column, search for matching column names in parent(s) */
 		for (j = 0; j < tbinfo->numatts; j++)
 		{
-			bool		foundAttr;		/* Attr was found in a parent */
 			bool		foundNotNull;	/* Attr was NOT NULL in a parent */
-			bool		defaultsMatch;	/* All non-empty defaults match */
-			bool		defaultsFound;	/* Found a default in a parent */
-			AttrDefInfo *attrDef;
-
-			foundAttr = false;
-			foundNotNull = false;
-			defaultsMatch = true;
-			defaultsFound = false;
+			bool		foundDefault;	/* Found a default in a parent */
 
-			attrDef = tbinfo->attrdefs[j];
+			/* no point in examining dropped columns */
+			if (tbinfo->attisdropped[j])
+				continue;
 
+			foundNotNull = false;
+			foundDefault = false;
 			for (k = 0; k < numParents; k++)
 			{
+				TableInfo  *parent = parents[k];
 				int			inhAttrInd;
 
-				parent = parents[k];
 				inhAttrInd = strInArray(tbinfo->attnames[j],
 										parent->attnames,
 										parent->numatts);
-
-				if (inhAttrInd != -1)
+				if (inhAttrInd >= 0)
 				{
-					AttrDefInfo *inhDef = parent->attrdefs[inhAttrInd];
-
-					foundAttr = true;
 					foundNotNull |= parent->notnull[inhAttrInd];
-					if (inhDef != NULL)
-					{
-						defaultsFound = true;
-
-						/*
-						 * If any parent has a default and the child doesn't,
-						 * we have to emit an explicit DEFAULT NULL clause for
-						 * the child, else the parent's default will win.
-						 */
-						if (attrDef == NULL)
-						{
-							attrDef = (AttrDefInfo *) malloc(sizeof(AttrDefInfo));
-							attrDef->dobj.objType = DO_ATTRDEF;
-							attrDef->dobj.catId.tableoid = 0;
-							attrDef->dobj.catId.oid = 0;
-							AssignDumpId(&attrDef->dobj);
-							attrDef->adtable = tbinfo;
-							attrDef->adnum = j + 1;
-							attrDef->adef_expr = strdup("NULL");
-
-							attrDef->dobj.name = strdup(tbinfo->dobj.name);
-							attrDef->dobj.namespace = tbinfo->dobj.namespace;
-
-							attrDef->dobj.dump = tbinfo->dobj.dump;
-
-							attrDef->separate = false;
-							addObjectDependency(&tbinfo->dobj,
-												attrDef->dobj.dumpId);
-
-							tbinfo->attrdefs[j] = attrDef;
-						}
-						if (strcmp(attrDef->adef_expr, inhDef->adef_expr) != 0)
-						{
-							defaultsMatch = false;
-
-							/*
-							 * Whenever there is a non-matching parent
-							 * default, add a dependency to force the parent
-							 * default to be dumped first, in case the
-							 * defaults end up being dumped as separate
-							 * commands.  Otherwise the parent default will
-							 * override the child's when it is applied.
-							 */
-							addObjectDependency(&attrDef->dobj,
-												inhDef->dobj.dumpId);
-						}
-					}
+					foundDefault |= (parent->attrdefs[inhAttrInd] != NULL);
 				}
 			}
 
-			/*
-			 * Based on the scan of the parents, decide if we can rely on the
-			 * inherited attr
-			 */
-			if (foundAttr)		/* Attr was inherited */
+			/* Remember if we found inherited NOT NULL */
+			tbinfo->inhNotNull[j] = foundNotNull;
+
+			/* Manufacture a DEFAULT NULL clause if necessary */
+			if (foundDefault && tbinfo->attrdefs[j] == NULL)
 			{
-				/* Set inherited flag by default */
-				tbinfo->inhAttrs[j] = true;
-				tbinfo->inhAttrDef[j] = true;
-				tbinfo->inhNotNull[j] = true;
-
-				/*
-				 * Clear it if attr had a default, but parents did not, or
-				 * mismatch
-				 */
-				if ((attrDef != NULL) && (!defaultsFound || !defaultsMatch))
+				AttrDefInfo *attrDef;
+
+				attrDef = (AttrDefInfo *) malloc(sizeof(AttrDefInfo));
+				attrDef->dobj.objType = DO_ATTRDEF;
+				attrDef->dobj.catId.tableoid = 0;
+				attrDef->dobj.catId.oid = 0;
+				AssignDumpId(&attrDef->dobj);
+				attrDef->dobj.name = strdup(tbinfo->dobj.name);
+				attrDef->dobj.namespace = tbinfo->dobj.namespace;
+				attrDef->dobj.dump = tbinfo->dobj.dump;
+
+				attrDef->adtable = tbinfo;
+				attrDef->adnum = j + 1;
+				attrDef->adef_expr = strdup("NULL");
+
+				/* Will column be dumped explicitly? */
+				if (shouldPrintColumn(tbinfo, j))
 				{
-					tbinfo->inhAttrs[j] = false;
-					tbinfo->inhAttrDef[j] = false;
+					attrDef->separate = false;
+					/* No dependency needed: NULL cannot have dependencies */
 				}
-
-				/*
-				 * Clear it if NOT NULL and none of the parents were NOT NULL
-				 */
-				if (tbinfo->notnull[j] && !foundNotNull)
+				else
 				{
-					tbinfo->inhAttrs[j] = false;
-					tbinfo->inhNotNull[j] = false;
+					/* column will be suppressed, print default separately */
+					attrDef->separate = true;
+					/* ensure it comes out after the table */
+					addObjectDependency(&attrDef->dobj,
+										tbinfo->dobj.dumpId);
 				}
 
-				/* Clear it if attr has local definition */
-				if (tbinfo->attislocal[j])
-					tbinfo->inhAttrs[j] = false;
+				tbinfo->attrdefs[j] = attrDef;
 			}
 		}
 	}
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 068664ba1a7..48f51615b3e 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -4796,6 +4796,9 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
 			 * attstattarget doesn't exist in 7.1.  It does exist in 7.2, but
 			 * we don't dump it because we can't tell whether it's been
 			 * explicitly set or was just a default.
+			 *
+			 * attislocal doesn't exist before 7.3, either; in older databases
+			 * we just assume that inherited columns had no local definition.
 			 */
 			appendPQExpBuffer(q, "SELECT a.attnum, a.attname, "
 						   "a.atttypmod, -1 AS attstattarget, a.attstorage, "
@@ -4858,10 +4861,8 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
 		tbinfo->attalign = (char *) malloc(ntups * sizeof(char));
 		tbinfo->attislocal = (bool *) malloc(ntups * sizeof(bool));
 		tbinfo->notnull = (bool *) malloc(ntups * sizeof(bool));
-		tbinfo->attrdefs = (AttrDefInfo **) malloc(ntups * sizeof(AttrDefInfo *));
-		tbinfo->inhAttrs = (bool *) malloc(ntups * sizeof(bool));
-		tbinfo->inhAttrDef = (bool *) malloc(ntups * sizeof(bool));
 		tbinfo->inhNotNull = (bool *) malloc(ntups * sizeof(bool));
+		tbinfo->attrdefs = (AttrDefInfo **) malloc(ntups * sizeof(AttrDefInfo *));
 		hasdefaults = false;
 
 		for (j = 0; j < ntups; j++)
@@ -4887,8 +4888,6 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
 			if (PQgetvalue(res, j, i_atthasdef)[0] == 't')
 				hasdefaults = true;
 			/* these flags will be set in flagInhAttrs() */
-			tbinfo->inhAttrs[j] = false;
-			tbinfo->inhAttrDef[j] = false;
 			tbinfo->inhNotNull[j] = false;
 		}
 
@@ -4952,12 +4951,28 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
 			{
 				int			adnum;
 
+				adnum = atoi(PQgetvalue(res, j, 2));
+
+				if (adnum <= 0 || adnum > ntups)
+				{
+					write_msg(NULL, "invalid adnum value %d for table \"%s\"\n",
+							  adnum, tbinfo->dobj.name);
+					exit_nicely();
+				}
+
+				/*
+				 * dropped columns shouldn't have defaults, but just in case,
+				 * ignore 'em
+				 */
+				if (tbinfo->attisdropped[adnum - 1])
+					continue;
+
 				attrdefs[j].dobj.objType = DO_ATTRDEF;
 				attrdefs[j].dobj.catId.tableoid = atooid(PQgetvalue(res, j, 0));
 				attrdefs[j].dobj.catId.oid = atooid(PQgetvalue(res, j, 1));
 				AssignDumpId(&attrdefs[j].dobj);
 				attrdefs[j].adtable = tbinfo;
-				attrdefs[j].adnum = adnum = atoi(PQgetvalue(res, j, 2));
+				attrdefs[j].adnum = adnum;
 				attrdefs[j].adef_expr = strdup(PQgetvalue(res, j, 3));
 
 				attrdefs[j].dobj.name = strdup(tbinfo->dobj.name);
@@ -4968,9 +4983,8 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
 				/*
 				 * Defaults on a VIEW must always be dumped as separate ALTER
 				 * TABLE commands.	Defaults on regular tables are dumped as
-				 * part of the CREATE TABLE if possible.  To check if it's
-				 * safe, we mark the default as needing to appear before the
-				 * CREATE.
+				 * part of the CREATE TABLE if possible, which it won't be
+				 * if the column is not going to be emitted explicitly.
 				 */
 				if (tbinfo->relkind == RELKIND_VIEW)
 				{
@@ -4979,19 +4993,27 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
 					addObjectDependency(&attrdefs[j].dobj,
 										tbinfo->dobj.dumpId);
 				}
+				else if (!shouldPrintColumn(tbinfo, adnum - 1))
+				{
+					/* column will be suppressed, print default separately */
+					attrdefs[j].separate = true;
+					/* needed in case pre-7.3 DB: */
+					addObjectDependency(&attrdefs[j].dobj,
+										tbinfo->dobj.dumpId);
+				}
 				else
 				{
 					attrdefs[j].separate = false;
+					/*
+					 * Mark the default as needing to appear before the table,
+					 * so that any dependencies it has must be emitted before
+					 * the CREATE TABLE.  If this is not possible, we'll
+					 * change to "separate" mode while sorting dependencies.
+					 */
 					addObjectDependency(&tbinfo->dobj,
 										attrdefs[j].dobj.dumpId);
 				}
 
-				if (adnum <= 0 || adnum > ntups)
-				{
-					write_msg(NULL, "invalid adnum value %d for table \"%s\"\n",
-							  adnum, tbinfo->dobj.name);
-					exit_nicely();
-				}
 				tbinfo->attrdefs[adnum - 1] = &attrdefs[j];
 			}
 			PQclear(res);
@@ -5138,6 +5160,28 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
 	destroyPQExpBuffer(q);
 }
 
+/*
+ * Test whether a column should be printed as part of table's CREATE TABLE.
+ * Column number is zero-based.
+ *
+ * Normally this is always true, but it's false for dropped columns, as well
+ * as those that were inherited without any local definition.  (If we print
+ * such a column it will mistakenly get pg_attribute.attislocal set to true.)
+ * However, in binary_upgrade mode, we must print all such columns anyway and
+ * fix the attislocal/attisdropped state later, so as to keep control of the
+ * physical column order.
+ *
+ * This function exists because there are scattered nonobvious places that
+ * must be kept in sync with this decision.
+ */
+bool
+shouldPrintColumn(TableInfo *tbinfo, int colno)
+{
+	if (binary_upgrade)
+		return true;
+	return (tbinfo->attislocal[colno] && !tbinfo->attisdropped[colno]);
+}
+
 
 /*
  * getTSParsers:
@@ -9876,11 +9920,11 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
 		for (j = 0; j < tbinfo->numatts; j++)
 		{
 			/*
-			 * Normally, dump if it's one of the table's own attrs, and not
-			 * dropped.  But for binary upgrade, dump all the columns.
+			 * Normally, dump if it's locally defined in this table, and not
+			 * dropped.  But for binary upgrade, we'll dump all the columns,
+			 * and then fix up the dropped and nonlocal cases below.
 			 */
-			if ((!tbinfo->inhAttrs[j] && !tbinfo->attisdropped[j]) ||
-				binary_upgrade)
+			if (shouldPrintColumn(tbinfo, j))
 			{
 				/* Format properly if not first attr */
 				if (actual_atts > 0)
@@ -9919,19 +9963,16 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
 				}
 
 				/*
-				 * Default value --- suppress if inherited (except in
-				 * binary-upgrade case, where we're not doing normal
-				 * inheritance) or if it's to be printed separately.
+				 * Default value --- suppress if to be printed separately.
 				 */
 				if (tbinfo->attrdefs[j] != NULL &&
-					(!tbinfo->inhAttrDef[j] || binary_upgrade) &&
 					!tbinfo->attrdefs[j]->separate)
 					appendPQExpBuffer(q, " DEFAULT %s",
 									  tbinfo->attrdefs[j]->adef_expr);
 
 				/*
-				 * Not Null constraint --- suppress if inherited, except
-				 * in binary-upgrade case.
+				 * Not Null constraint --- suppress if inherited, except in
+				 * binary-upgrade case where that won't work.
 				 */
 				if (tbinfo->notnull[j] &&
 					(!tbinfo->inhNotNull[j] || binary_upgrade))
@@ -10106,16 +10147,36 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
 			}
 		}
 
-		/* Loop dumping statistics and storage statements */
+		/*
+		 * Dump additional per-column properties that we can't handle in the
+		 * main CREATE TABLE command.
+		 */
 		for (j = 0; j < tbinfo->numatts; j++)
 		{
+			/* None of this applies to dropped columns */
+			if (tbinfo->attisdropped[j])
+				continue;
+
+			/*
+			 * If we didn't dump the column definition explicitly above, and
+			 * it is NOT NULL and did not inherit that property from a parent,
+			 * we have to mark it separately.
+			 */
+			if (!shouldPrintColumn(tbinfo, j) &&
+				tbinfo->notnull[j] && !tbinfo->inhNotNull[j])
+			{
+				appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
+								  fmtId(tbinfo->dobj.name));
+				appendPQExpBuffer(q, "ALTER COLUMN %s SET NOT NULL;\n",
+								  fmtId(tbinfo->attnames[j]));
+			}
+
 			/*
 			 * Dump per-column statistics information. We only issue an ALTER
 			 * TABLE statement if the attstattarget entry for this column is
 			 * non-negative (i.e. it's not the default value)
 			 */
-			if (tbinfo->attstattarget[j] >= 0 &&
-				!tbinfo->attisdropped[j])
+			if (tbinfo->attstattarget[j] >= 0)
 			{
 				appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
 								  fmtId(tbinfo->dobj.name));
@@ -10129,7 +10190,7 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
 			 * Dump per-column storage information.  The statement is only
 			 * dumped if the storage has been changed from the type's default.
 			 */
-			if (!tbinfo->attisdropped[j] && tbinfo->attstorage[j] != tbinfo->typstorage[j])
+			if (tbinfo->attstorage[j] != tbinfo->typstorage[j])
 			{
 				switch (tbinfo->attstorage[j])
 				{
@@ -10206,18 +10267,18 @@ dumpAttrDef(Archive *fout, AttrDefInfo *adinfo)
 	PQExpBuffer q;
 	PQExpBuffer delq;
 
-	/* Only print it if "separate" mode is selected */
-	if (!tbinfo->dobj.dump || !adinfo->separate || dataOnly)
+	/* Skip if table definition not to be dumped */
+	if (!tbinfo->dobj.dump || dataOnly)
 		return;
 
-	/* Don't print inherited defaults, either, except for binary upgrade */
-	if (tbinfo->inhAttrDef[adnum - 1] && !binary_upgrade)
+	/* Skip if not "separate"; it was dumped in the table's definition */
+	if (!adinfo->separate)
 		return;
 
 	q = createPQExpBuffer();
 	delq = createPQExpBuffer();
 
-	appendPQExpBuffer(q, "ALTER TABLE %s ",
+	appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
 					  fmtId(tbinfo->dobj.name));
 	appendPQExpBuffer(q, "ALTER COLUMN %s SET DEFAULT %s;\n",
 					  fmtId(tbinfo->attnames[adnum - 1]),
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 390b20cf1aa..64e73b6e8ef 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -251,17 +251,9 @@ typedef struct _tableInfo
 	int		   *attlen;			/* attribute length, used by binary_upgrade */
 	char	   *attalign;		/* attribute align, used by binary_upgrade */
 	bool	   *attislocal;		/* true if attr has local definition */
-
-	/*
-	 * Note: we need to store per-attribute notnull, default, and constraint
-	 * stuff for all interesting tables so that we can tell which constraints
-	 * were inherited.
-	 */
-	bool	   *notnull;		/* Not null constraints on attributes */
-	struct _attrDefInfo **attrdefs;		/* DEFAULT expressions */
-	bool	   *inhAttrs;		/* true if each attribute is inherited */
-	bool	   *inhAttrDef;		/* true if attr's default is inherited */
+	bool	   *notnull;		/* NOT NULL constraints on attributes */
 	bool	   *inhNotNull;		/* true if NOT NULL is inherited */
+	struct _attrDefInfo **attrdefs;		/* DEFAULT expressions */
 	struct _constraintInfo *checkexprs; /* CHECK constraints */
 
 	/*
@@ -274,7 +266,7 @@ typedef struct _tableInfo
 
 typedef struct _attrDefInfo
 {
-	DumpableObject dobj;
+	DumpableObject dobj;		/* note: dobj.name is name of table */
 	TableInfo  *adtable;		/* link to table of attribute */
 	int			adnum;
 	char	   *adef_expr;		/* decompiled DEFAULT expression */
@@ -505,6 +497,7 @@ extern void getTriggers(TableInfo tblinfo[], int numTables);
 extern ProcLangInfo *getProcLangs(int *numProcLangs);
 extern CastInfo *getCasts(int *numCasts);
 extern void getTableAttrs(TableInfo *tbinfo, int numTables);
+extern bool shouldPrintColumn(TableInfo *tbinfo, int colno);
 extern TSParserInfo *getTSParsers(int *numTSParsers);
 extern TSDictInfo *getTSDictionaries(int *numTSDicts);
 extern TSTemplateInfo *getTSTemplates(int *numTSTemplates);