Test instability when pg_dump orders by OID

Started by Noah Misch6 months ago25 messages
#1Noah Misch
noah@leadboat.com
2 attachment(s)

A 002_pg_upgrade.pl run got swapped order of tags "notnull_tbl1_upg nn" and
"notnull_parent_upg nn" for the schema diff test that commit
172259afb563d35001410dc6daad78b250924038 added in v18:

@@ -436873,14 +436873,14 @@
 ALTER TABLE public.insert_tbl
     ADD CONSTRAINT ne_insert_tbl_con CHECK (((x + z) = 1)) NOT ENFORCED;
 --
--- Name: notnull_tbl1_upg nn; Type: CONSTRAINT; Schema: public; Owner: nm
+-- Name: notnull_parent_upg nn; Type: CONSTRAINT; Schema: public; Owner: nm
 --
-ALTER TABLE public.notnull_tbl1_upg
+ALTER TABLE public.notnull_parent_upg
     ADD CONSTRAINT nn NOT NULL a NOT VALID;
 --
--- Name: notnull_parent_upg nn; Type: CONSTRAINT; Schema: public; Owner: nm
+-- Name: notnull_tbl1_upg nn; Type: CONSTRAINT; Schema: public; Owner: nm
 --
-ALTER TABLE public.notnull_parent_upg
+ALTER TABLE public.notnull_tbl1_upg

pg_dump uses pg_constraint.oid as one sort key, and "pg_restore -j" opens the
door for OID assignment order to vary. The first attached patch demonstrates
this by simulation. It yields that diff and some operator order diffs.

Let's get rid of pg_dump's need to sort by OID, apart from catalog corruption
scenarios. Adding an assert found a total of seven affected object types.
See the second attached patch. The drawback is storing five more fields in
pg_dump memory: oprleft, oprright, opcmethod, opfmethod, and collencoding.
That seems minor relative to existing pg_dump memory efficiency. Since this
is a source of test flakes in v18, I'd like to back-patch to v18. I'm not
sure why the buildfarm hasn't seen the above diff, but I expect the diff could
happen there. This is another nice win for the new test from commit
172259afb563d35001410dc6daad78b250924038. The order instability was always
bad for users, but the test brought it to the forefront. One might argue for
back-patching $SUBJECT further, too.

Thanks,
nm

Attachments:

dobjcmp10-demo-v0.patch.nocitext/plain; charset=us-asciiDownload
From: Noah Misch <noah@leadboat.com>

Demo 002_pg_upgrade.pl schema diffs from using OID in pg_dump sort.

Not for commit.  This reverses the pg_dump OID sort for one side of the
diff, simulating a worst case for "pg_restore -j" perturbing OID order.

diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index 0b09777..a3d49a5 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -319,7 +319,8 @@ DOTypeNameCompare(const void *p1, const void *p2)
 	}
 
 	/* Usually shouldn't get here, but if we do, sort by OID */
-	return oidcmp(obj1->catId.oid, obj2->catId.oid);
+	return (oidcmp(obj1->catId.oid, obj2->catId.oid) *
+			(getenv("PGTEST_DUMP_FALLBACK_REVERSE") ? -1 : 1));
 }
 
 
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 7d82593..6a22731 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -413,8 +413,12 @@ SKIP:
 	# Dump original and restored database for comparison.
 	my $src_dump =
 	  get_dump_for_comparison($oldnode, 'regression', 'src_dump', 1);
-	my $dst_dump =
-	  get_dump_for_comparison($dstnode, 'regression', 'dest_dump', 0);
+	my $dst_dump;
+	{
+		local $ENV{PGTEST_DUMP_FALLBACK_REVERSE} = 1;
+		$dst_dump =
+		  get_dump_for_comparison($dstnode, 'regression', 'dest_dump', 0);
+	}
 
 	compare_files($src_dump, $dst_dump,
 		'dump outputs from original and restored regression databases match');
dobjcmp20-disambiguate-v1.patchtext/plain; charset=us-asciiDownload
From: Noah Misch <noah@leadboat.com>

Sort dump objects independent of OIDs, for the 7 holdout object types.

pg_dump sorts objects by their logical names, e.g. (nspname, relname,
tgname), before dependency-driven reordering.  That removes one source
of logically-identical databases differing in their schema-only dumps.
In other words, it helps with schema diffing.  The logical name sort
ignored essential sort keys for constraints, operators, PUBLICATION
... FOR TABLE, PUBLICATION ... FOR TABLES IN SCHEMA, operator classes,
and operator families.  pg_dump's sort then depended on object OID,
yielding spurious schema diffs.  After this change, OIDs affect dump
order only in the event of catalog corruption.  While pg_dump also
wrongly ignored pg_collation.collencoding, CREATE COLLATION restrictions
have been keeping that imperceptible in practical use.

Use techniques like we use for object types already having full sort key
coverage.  Where the pertinent queries weren't fetching the ignored sort
keys, this adds columns to those queries and stores those keys in memory
for the long term.

The ignorance of sort keys became more problematic when commit
172259afb563d35001410dc6daad78b250924038 added a schema diff test
sensitive to it.  Hence, back-patch as far as that commit.

Reviewed-by: FIXME
Discussion: https://postgr.es/m/FIXME
Backpatch-through: 18

diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index aa1589e..a1976fa 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -17,6 +17,7 @@
 
 #include <ctype.h>
 
+#include "catalog/pg_am_d.h"
 #include "catalog/pg_class_d.h"
 #include "catalog/pg_collation_d.h"
 #include "catalog/pg_extension_d.h"
@@ -945,6 +946,24 @@ findOprByOid(Oid oid)
 }
 
 /*
+ * findAccessMethodByOid
+ *	  finds the DumpableObject for the access method with the given oid
+ *	  returns NULL if not found
+ */
+AccessMethodInfo *
+findAccessMethodByOid(Oid oid)
+{
+	CatalogId	catId;
+	DumpableObject *dobj;
+
+	catId.tableoid = AccessMethodRelationId;
+	catId.oid = oid;
+	dobj = findObjectByCatalogId(catId);
+	Assert(dobj == NULL || dobj->objType == DO_ACCESS_METHOD);
+	return (AccessMethodInfo *) dobj;
+}
+
+/*
  * findCollationByOid
  *	  finds the DumpableObject for the collation with the given oid
  *	  returns NULL if not found
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 1937997..cf75e24 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -6181,6 +6181,8 @@ getOperators(Archive *fout)
 	int			i_oprnamespace;
 	int			i_oprowner;
 	int			i_oprkind;
+	int			i_oprleft;
+	int			i_oprright;
 	int			i_oprcode;
 
 	/*
@@ -6192,6 +6194,8 @@ getOperators(Archive *fout)
 						 "oprnamespace, "
 						 "oprowner, "
 						 "oprkind, "
+						 "oprleft, "
+						 "oprright, "
 						 "oprcode::oid AS oprcode "
 						 "FROM pg_operator");
 
@@ -6207,6 +6211,8 @@ getOperators(Archive *fout)
 	i_oprnamespace = PQfnumber(res, "oprnamespace");
 	i_oprowner = PQfnumber(res, "oprowner");
 	i_oprkind = PQfnumber(res, "oprkind");
+	i_oprleft = PQfnumber(res, "oprleft");
+	i_oprright = PQfnumber(res, "oprright");
 	i_oprcode = PQfnumber(res, "oprcode");
 
 	for (i = 0; i < ntups; i++)
@@ -6220,6 +6226,8 @@ getOperators(Archive *fout)
 			findNamespace(atooid(PQgetvalue(res, i, i_oprnamespace)));
 		oprinfo[i].rolname = getRoleName(PQgetvalue(res, i, i_oprowner));
 		oprinfo[i].oprkind = (PQgetvalue(res, i, i_oprkind))[0];
+		oprinfo[i].oprleft = atooid(PQgetvalue(res, i, i_oprleft));
+		oprinfo[i].oprright = atooid(PQgetvalue(res, i, i_oprright));
 		oprinfo[i].oprcode = atooid(PQgetvalue(res, i, i_oprcode));
 
 		/* Decide whether we want to dump it */
@@ -6248,6 +6256,7 @@ getCollations(Archive *fout)
 	int			i_collname;
 	int			i_collnamespace;
 	int			i_collowner;
+	int			i_collencoding;
 
 	query = createPQExpBuffer();
 
@@ -6258,7 +6267,8 @@ getCollations(Archive *fout)
 
 	appendPQExpBufferStr(query, "SELECT tableoid, oid, collname, "
 						 "collnamespace, "
-						 "collowner "
+						 "collowner, "
+						 "collencoding "
 						 "FROM pg_collation");
 
 	res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
@@ -6272,6 +6282,7 @@ getCollations(Archive *fout)
 	i_collname = PQfnumber(res, "collname");
 	i_collnamespace = PQfnumber(res, "collnamespace");
 	i_collowner = PQfnumber(res, "collowner");
+	i_collencoding = PQfnumber(res, "collencoding");
 
 	for (i = 0; i < ntups; i++)
 	{
@@ -6283,6 +6294,7 @@ getCollations(Archive *fout)
 		collinfo[i].dobj.namespace =
 			findNamespace(atooid(PQgetvalue(res, i, i_collnamespace)));
 		collinfo[i].rolname = getRoleName(PQgetvalue(res, i, i_collowner));
+		collinfo[i].collencoding = atoi(PQgetvalue(res, i, i_collencoding));
 
 		/* Decide whether we want to dump it */
 		selectDumpableObject(&(collinfo[i].dobj), fout);
@@ -6431,6 +6443,7 @@ getOpclasses(Archive *fout)
 	OpclassInfo *opcinfo;
 	int			i_tableoid;
 	int			i_oid;
+	int			i_opcmethod;
 	int			i_opcname;
 	int			i_opcnamespace;
 	int			i_opcowner;
@@ -6440,7 +6453,7 @@ getOpclasses(Archive *fout)
 	 * system-defined opclasses at dump-out time.
 	 */
 
-	appendPQExpBufferStr(query, "SELECT tableoid, oid, opcname, "
+	appendPQExpBufferStr(query, "SELECT tableoid, oid, opcmethod, opcname, "
 						 "opcnamespace, "
 						 "opcowner "
 						 "FROM pg_opclass");
@@ -6453,6 +6466,7 @@ getOpclasses(Archive *fout)
 
 	i_tableoid = PQfnumber(res, "tableoid");
 	i_oid = PQfnumber(res, "oid");
+	i_opcmethod = PQfnumber(res, "opcmethod");
 	i_opcname = PQfnumber(res, "opcname");
 	i_opcnamespace = PQfnumber(res, "opcnamespace");
 	i_opcowner = PQfnumber(res, "opcowner");
@@ -6466,6 +6480,7 @@ getOpclasses(Archive *fout)
 		opcinfo[i].dobj.name = pg_strdup(PQgetvalue(res, i, i_opcname));
 		opcinfo[i].dobj.namespace =
 			findNamespace(atooid(PQgetvalue(res, i, i_opcnamespace)));
+		opcinfo[i].opcmethod = atooid(PQgetvalue(res, i, i_opcmethod));
 		opcinfo[i].rolname = getRoleName(PQgetvalue(res, i, i_opcowner));
 
 		/* Decide whether we want to dump it */
@@ -6491,6 +6506,7 @@ getOpfamilies(Archive *fout)
 	OpfamilyInfo *opfinfo;
 	int			i_tableoid;
 	int			i_oid;
+	int			i_opfmethod;
 	int			i_opfname;
 	int			i_opfnamespace;
 	int			i_opfowner;
@@ -6502,7 +6518,7 @@ getOpfamilies(Archive *fout)
 	 * system-defined opfamilies at dump-out time.
 	 */
 
-	appendPQExpBufferStr(query, "SELECT tableoid, oid, opfname, "
+	appendPQExpBufferStr(query, "SELECT tableoid, oid, opfmethod, opfname, "
 						 "opfnamespace, "
 						 "opfowner "
 						 "FROM pg_opfamily");
@@ -6516,6 +6532,7 @@ getOpfamilies(Archive *fout)
 	i_tableoid = PQfnumber(res, "tableoid");
 	i_oid = PQfnumber(res, "oid");
 	i_opfname = PQfnumber(res, "opfname");
+	i_opfmethod = PQfnumber(res, "opfmethod");
 	i_opfnamespace = PQfnumber(res, "opfnamespace");
 	i_opfowner = PQfnumber(res, "opfowner");
 
@@ -6528,6 +6545,7 @@ getOpfamilies(Archive *fout)
 		opfinfo[i].dobj.name = pg_strdup(PQgetvalue(res, i, i_opfname));
 		opfinfo[i].dobj.namespace =
 			findNamespace(atooid(PQgetvalue(res, i, i_opfnamespace)));
+		opfinfo[i].opfmethod = atooid(PQgetvalue(res, i, i_opfmethod));
 		opfinfo[i].rolname = getRoleName(PQgetvalue(res, i, i_opfowner));
 
 		/* Decide whether we want to dump it */
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 39eef1d..a3e848d 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -258,6 +258,8 @@ typedef struct _oprInfo
 	DumpableObject dobj;
 	const char *rolname;
 	char		oprkind;
+	Oid			oprleft;
+	Oid			oprright;
 	Oid			oprcode;
 } OprInfo;
 
@@ -271,12 +273,14 @@ typedef struct _accessMethodInfo
 typedef struct _opclassInfo
 {
 	DumpableObject dobj;
+	Oid			opcmethod;
 	const char *rolname;
 } OpclassInfo;
 
 typedef struct _opfamilyInfo
 {
 	DumpableObject dobj;
+	Oid			opfmethod;
 	const char *rolname;
 } OpfamilyInfo;
 
@@ -284,6 +288,7 @@ typedef struct _collInfo
 {
 	DumpableObject dobj;
 	const char *rolname;
+	int			collencoding;
 } CollInfo;
 
 typedef struct _convInfo
@@ -757,6 +762,7 @@ extern TableInfo *findTableByOid(Oid oid);
 extern TypeInfo *findTypeByOid(Oid oid);
 extern FuncInfo *findFuncByOid(Oid oid);
 extern OprInfo *findOprByOid(Oid oid);
+extern AccessMethodInfo *findAccessMethodByOid(Oid oid);
 extern CollInfo *findCollationByOid(Oid oid);
 extern NamespaceInfo *findNamespaceByOid(Oid oid);
 extern ExtensionInfo *findExtensionByOid(Oid oid);
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index 0b09777..ffae7b3 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -162,6 +162,8 @@ static DumpId postDataBoundId;
 
 
 static int	DOTypeNameCompare(const void *p1, const void *p2);
+static int	pgTypeNameCompare(Oid typid1, Oid typid2);
+static int	accessMethodNameCompare(Oid am1, Oid am2);
 static bool TopoSort(DumpableObject **objs,
 					 int numObjs,
 					 DumpableObject **ordering,
@@ -228,11 +230,24 @@ DOTypeNameCompare(const void *p1, const void *p2)
 	else if (obj2->namespace)
 		return 1;
 
-	/* Sort by name */
+	/*
+	 * Sort by name.  This differs from "Name:" in plain format output, which
+	 * is a _tocEntry.tag.  For example, DumpableObject.name of a constraint
+	 * is pg_constraint.conname, but _tocEntry.tag of a constraint is relname
+	 * and conname joined with a space.
+	 */
 	cmpval = strcmp(obj1->name, obj2->name);
 	if (cmpval != 0)
 		return cmpval;
 
+	/*
+	 * Sort by type.  This helps types that share a type priority without
+	 * sharing a unique name constraint, e.g. opclass and opfamily.
+	 */
+	cmpval = obj1->objType - obj2->objType;
+	if (cmpval != 0)
+		return cmpval;
+
 	/* To have a stable sort order, break ties for some object types */
 	if (obj1->objType == DO_FUNC || obj1->objType == DO_AGG)
 	{
@@ -246,22 +261,10 @@ DOTypeNameCompare(const void *p1, const void *p2)
 			return cmpval;
 		for (i = 0; i < fobj1->nargs; i++)
 		{
-			TypeInfo   *argtype1 = findTypeByOid(fobj1->argtypes[i]);
-			TypeInfo   *argtype2 = findTypeByOid(fobj2->argtypes[i]);
-
-			if (argtype1 && argtype2)
-			{
-				if (argtype1->dobj.namespace && argtype2->dobj.namespace)
-				{
-					cmpval = strcmp(argtype1->dobj.namespace->dobj.name,
-									argtype2->dobj.namespace->dobj.name);
-					if (cmpval != 0)
-						return cmpval;
-				}
-				cmpval = strcmp(argtype1->dobj.name, argtype2->dobj.name);
-				if (cmpval != 0)
-					return cmpval;
-			}
+			cmpval = pgTypeNameCompare(fobj1->argtypes[i],
+									   fobj2->argtypes[i]);
+			if (cmpval != 0)
+				return cmpval;
 		}
 	}
 	else if (obj1->objType == DO_OPERATOR)
@@ -273,6 +276,53 @@ DOTypeNameCompare(const void *p1, const void *p2)
 		cmpval = (oobj2->oprkind - oobj1->oprkind);
 		if (cmpval != 0)
 			return cmpval;
+		/* Within an oprkind, sort by argument type names */
+		cmpval = pgTypeNameCompare(oobj1->oprleft, oobj2->oprleft);
+		if (cmpval != 0)
+			return cmpval;
+		cmpval = pgTypeNameCompare(oobj1->oprright, oobj2->oprright);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	else if (obj1->objType == DO_OPCLASS)
+	{
+		OpclassInfo *opcobj1 = *(OpclassInfo *const *) p1;
+		OpclassInfo *opcobj2 = *(OpclassInfo *const *) p2;
+
+		/* Sort by access method name, per pg_opclass_am_name_nsp_index */
+		cmpval = accessMethodNameCompare(opcobj1->opcmethod,
+										 opcobj2->opcmethod);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	else if (obj1->objType == DO_OPFAMILY)
+	{
+		OpfamilyInfo *opfobj1 = *(OpfamilyInfo *const *) p1;
+		OpfamilyInfo *opfobj2 = *(OpfamilyInfo *const *) p2;
+
+		/* Sort by access method name, per pg_opfamily_am_name_nsp_index */
+		cmpval = accessMethodNameCompare(opfobj1->opfmethod,
+										 opfobj2->opfmethod);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	else if (obj1->objType == DO_COLLATION)
+	{
+		CollInfo   *cobj1 = *(CollInfo *const *) p1;
+		CollInfo   *cobj2 = *(CollInfo *const *) p2;
+
+		/*
+		 * Sort by encoding, per pg_collation_name_enc_nsp_index.  This is
+		 * mostly academic, because CREATE COLLATION has restrictions to make
+		 * (nspname, collname) uniquely identify a collation within a given
+		 * DatabaseEncoding.  pg_import_system_collations() bypasses those
+		 * restrictions, but pg_dump+restore fails after a
+		 * pg_import_system_collations('my_schema') that creates collations
+		 * for a blend of encodings.
+		 */
+		cmpval = cobj1->collencoding - cobj2->collencoding;
+		if (cmpval != 0)
+			return cmpval;
 	}
 	else if (obj1->objType == DO_ATTRDEF)
 	{
@@ -317,11 +367,120 @@ DOTypeNameCompare(const void *p1, const void *p2)
 		if (cmpval != 0)
 			return cmpval;
 	}
+	else if (obj1->objType == DO_CONSTRAINT)
+	{
+		ConstraintInfo *robj1 = *(ConstraintInfo *const *) p1;
+		ConstraintInfo *robj2 = *(ConstraintInfo *const *) p2;
+
+		/* Sort by table name (table namespace was considered already) */
+		cmpval = strcmp(robj1->contable->dobj.name,
+						robj2->contable->dobj.name);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	else if (obj1->objType == DO_PUBLICATION_REL)
+	{
+		PublicationRelInfo *probj1 = *(PublicationRelInfo *const *) p1;
+		PublicationRelInfo *probj2 = *(PublicationRelInfo *const *) p2;
+
+		/* Sort by publication name, since (namespace, name) match the rel */
+		cmpval = strcmp(probj1->publication->dobj.name,
+						probj2->publication->dobj.name);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	else if (obj1->objType == DO_PUBLICATION_TABLE_IN_SCHEMA)
+	{
+		PublicationSchemaInfo *psobj1 = *(PublicationSchemaInfo *const *) p1;
+		PublicationSchemaInfo *psobj2 = *(PublicationSchemaInfo *const *) p2;
 
-	/* Usually shouldn't get here, but if we do, sort by OID */
+		/* Sort by publication name, since ->name is just nspname */
+		cmpval = strcmp(psobj1->publication->dobj.name,
+						psobj2->publication->dobj.name);
+		if (cmpval != 0)
+			return cmpval;
+	}
+
+	/*
+	 * Shouldn't get here except after catalog corruption, but if we do, sort
+	 * by OID.  This may make logically-identical databases differ in the
+	 * order of objects in dump output.  Users will get spurious schema diffs.
+	 * Expect flaky failures of 002_pg_upgrade.pl test 'dump outputs from
+	 * original and restored regression databases match' if the regression
+	 * database contains objects allowing that test to reach here.  That's a
+	 * consequence of the test using "pg_restore -j", which doesn't fully
+	 * constrain OID assignment order.
+	 */
+	Assert(false);
 	return oidcmp(obj1->catId.oid, obj2->catId.oid);
 }
 
+/* Compare two OID-identified pg_type values by nspname, then by typname. */
+static int
+pgTypeNameCompare(Oid typid1, Oid typid2)
+{
+	TypeInfo   *typobj1;
+	TypeInfo   *typobj2;
+	int			cmpval;
+
+	if (typid1 == typid2)
+		return 0;
+
+	typobj1 = findTypeByOid(typid1);
+	typobj2 = findTypeByOid(typid2);
+
+	if (!typobj1 || !typobj2)
+	{
+		/*
+		 * getTypes() didn't find some OID.  Assume catalog corruption, e.g.
+		 * an oprright value without the corresponding OID in a pg_type row.
+		 * Report as "equal", so the caller uses the next available basis for
+		 * comparison, e.g. the next function argument.
+		 *
+		 * Unary operators have InvalidOid in oprleft, but caller's oprkind
+		 * comparison ensures this function compares a unary oprleft only to
+		 * another unary oprleft.  Hence, "typid1 == typid2" took care of
+		 * InvalidOid.
+		 */
+		Assert(false);
+		return 0;
+	}
+
+	if (!typobj1->dobj.namespace || !typobj2->dobj.namespace)
+		Assert(false);			/* catalog corruption */
+	else
+	{
+		cmpval = strcmp(typobj1->dobj.namespace->dobj.name,
+						typobj2->dobj.namespace->dobj.name);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	return strcmp(typobj1->dobj.name, typobj2->dobj.name);
+}
+
+/* Compare two OID-identified pg_am values by amname. */
+static int
+accessMethodNameCompare(Oid am1, Oid am2)
+{
+	AccessMethodInfo *amobj1;
+	AccessMethodInfo *amobj2;
+
+	if (am1 == am2)
+		return 0;
+
+	amobj1 = findAccessMethodByOid(am1);
+	amobj2 = findAccessMethodByOid(am2);
+
+	if (!amobj1 || !amobj2)
+	{
+		/* catalog corruption: handle like pgTypeNameCompare() does */
+		Assert(false);
+		return 0;
+	}
+
+	return strcmp(amobj1->dobj.name, amobj2->dobj.name);
+}
+
 
 /*
  * Sort the given objects into a safe dump order using dependency
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 3a2eacd..1ec3fa3 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -1934,3 +1934,24 @@ RESET client_min_messages;
 RESET SESSION AUTHORIZATION;
 DROP ROLE regress_publication_user, regress_publication_user2;
 DROP ROLE regress_publication_user_dummy;
+-- stage objects for pg_dump tests
+CREATE SCHEMA pubme CREATE TABLE t0 (c int, d int) CREATE TABLE t1 (c int);
+CREATE SCHEMA pubme2 CREATE TABLE t0 (c int, d int);
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION dump_pub_qual_1ct FOR
+  TABLE ONLY pubme.t0 (c, d) WHERE (c > 0);
+CREATE PUBLICATION dump_pub_qual_2ct FOR
+  TABLE ONLY pubme.t0 (c) WHERE (c > 0),
+  TABLE ONLY pubme.t1 (c);
+CREATE PUBLICATION dump_pub_nsp_1ct FOR
+  TABLES IN SCHEMA pubme;
+CREATE PUBLICATION dump_pub_nsp_2ct FOR
+  TABLES IN SCHEMA pubme,
+  TABLES IN SCHEMA pubme2;
+CREATE PUBLICATION dump_pub_all FOR
+  TABLE ONLY pubme.t0,
+  TABLE ONLY pubme.t1 WHERE (c < 0),
+  TABLES IN SCHEMA pubme,
+  TABLES IN SCHEMA pubme2
+  WITH (publish_via_partition_root = true);
+RESET client_min_messages;
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index c9e3091..2585f08 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -1229,3 +1229,25 @@ RESET client_min_messages;
 RESET SESSION AUTHORIZATION;
 DROP ROLE regress_publication_user, regress_publication_user2;
 DROP ROLE regress_publication_user_dummy;
+
+-- stage objects for pg_dump tests
+CREATE SCHEMA pubme CREATE TABLE t0 (c int, d int) CREATE TABLE t1 (c int);
+CREATE SCHEMA pubme2 CREATE TABLE t0 (c int, d int);
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION dump_pub_qual_1ct FOR
+  TABLE ONLY pubme.t0 (c, d) WHERE (c > 0);
+CREATE PUBLICATION dump_pub_qual_2ct FOR
+  TABLE ONLY pubme.t0 (c) WHERE (c > 0),
+  TABLE ONLY pubme.t1 (c);
+CREATE PUBLICATION dump_pub_nsp_1ct FOR
+  TABLES IN SCHEMA pubme;
+CREATE PUBLICATION dump_pub_nsp_2ct FOR
+  TABLES IN SCHEMA pubme,
+  TABLES IN SCHEMA pubme2;
+CREATE PUBLICATION dump_pub_all FOR
+  TABLE ONLY pubme.t0,
+  TABLE ONLY pubme.t1 WHERE (c < 0),
+  TABLES IN SCHEMA pubme,
+  TABLES IN SCHEMA pubme2
+  WITH (publish_via_partition_root = true);
+RESET client_min_messages;
#2Robert Haas
robertmhaas@gmail.com
In reply to: Noah Misch (#1)
Re: Test instability when pg_dump orders by OID

On Mon, Jul 7, 2025 at 3:27 PM Noah Misch <noah@leadboat.com> wrote:

Let's get rid of pg_dump's need to sort by OID, apart from catalog corruption
scenarios.

+1. I had at one point believed that sorting by OID was a good way to
make dumps stable, but this disproves that theory. Sorting by logical
properties of the object is better.

Adding an assert found a total of seven affected object types.
See the second attached patch. The drawback is storing five more fields in
pg_dump memory: oprleft, oprright, opcmethod, opfmethod, and collencoding.
That seems minor relative to existing pg_dump memory efficiency. Since this
is a source of test flakes in v18, I'd like to back-patch to v18. I'm not
sure why the buildfarm hasn't seen the above diff, but I expect the diff could
happen there. This is another nice win for the new test from commit
172259afb563d35001410dc6daad78b250924038. The order instability was always
bad for users, but the test brought it to the forefront. One might argue for
back-patching $SUBJECT further, too.

I agree with back-patching it at least as far as v18. I think it
probably wouldn't hurt anything to back-patch further, and it might
avoid future buildfarm failures. Against that, there's a remote
possibility that someone who is currently saving pg_dump output for
later comparison, say in a case where OIDs are always stable in
practice, could be displeased to see the pg_dump order change in a
minor release. But that seems like a very weak argument against
back-patching. I can't see us ever deciding to put up with buildfarm
instability on such grounds.

Reviewing:

+ * Sort by name.  This differs from "Name:" in plain format output, which
+ * is a _tocEntry.tag.  For example, DumpableObject.name of a constraint
+ * is pg_constraint.conname, but _tocEntry.tag of a constraint is relname
+ * and conname joined with a space.

This comment is useful, but if I were to be critical, it does a better
job saying what this field isn't than what it is.

+ * Sort by encoding, per pg_collation_name_enc_nsp_index.  This is
+ * mostly academic, because CREATE COLLATION has restrictions to make
+ * (nspname, collname) uniquely identify a collation within a given
+ * DatabaseEncoding.  pg_import_system_collations() bypasses those
+ * restrictions, but pg_dump+restore fails after a
+ * pg_import_system_collations('my_schema') that creates collations
+ * for a blend of encodings.

This comment is also useful, but if I were to be critical again, it
does a better job saying why we shouldn't do what the code then does
than why we should.

Neither of those issues seem like must-fix problems to me.

--
Robert Haas
EDB: http://www.enterprisedb.com

#3Noah Misch
noah@leadboat.com
In reply to: Robert Haas (#2)
Re: Test instability when pg_dump orders by OID

On Thu, Jul 17, 2025 at 09:24:02AM -0400, Robert Haas wrote:

On Mon, Jul 7, 2025 at 3:27 PM Noah Misch <noah@leadboat.com> wrote:

Let's get rid of pg_dump's need to sort by OID, apart from catalog corruption
scenarios.

+1. I had at one point believed that sorting by OID was a good way to
make dumps stable, but this disproves that theory. Sorting by logical
properties of the object is better.

Sorting by OID was a reasonable approximation, for its time.

Adding an assert found a total of seven affected object types.
See the second attached patch. The drawback is storing five more fields in
pg_dump memory: oprleft, oprright, opcmethod, opfmethod, and collencoding.
That seems minor relative to existing pg_dump memory efficiency. Since this
is a source of test flakes in v18, I'd like to back-patch to v18. I'm not
sure why the buildfarm hasn't seen the above diff, but I expect the diff could
happen there. This is another nice win for the new test from commit
172259afb563d35001410dc6daad78b250924038. The order instability was always
bad for users, but the test brought it to the forefront. One might argue for
back-patching $SUBJECT further, too.

I agree with back-patching it at least as far as v18. I think it
probably wouldn't hurt anything to back-patch further, and it might
avoid future buildfarm failures. Against that, there's a remote
possibility that someone who is currently saving pg_dump output for
later comparison, say in a case where OIDs are always stable in
practice, could be displeased to see the pg_dump order change in a
minor release. But that seems like a very weak argument against
back-patching. I can't see us ever deciding to put up with buildfarm
instability on such grounds.

Thanks for reviewing. I agree with those merits of back-patching further. A
back-patch to v13 has no pg_dump_sort.c conflicts, while pg_dump.c has
mechanical conflicts around retrieving the extra sort inputs. If there are no
objections in the next 72h, I'll likely back-patch.

Reviewing:

+ * Sort by name.  This differs from "Name:" in plain format output, which
+ * is a _tocEntry.tag.  For example, DumpableObject.name of a constraint
+ * is pg_constraint.conname, but _tocEntry.tag of a constraint is relname
+ * and conname joined with a space.

This comment is useful, but if I were to be critical, it does a better
job saying what this field isn't than what it is.

True. I've changed it to this:

* Sort by name. With a few exceptions, names here are single catalog
* columns. To get a fuller picture, grep pg_dump.c for "dobj.name = ".
* Names here don't match "Name:" in plain format output, which is a
* _tocEntry.tag. For example, DumpableObject.name of a constraint is
* pg_constraint.conname, but _tocEntry.tag of a constraint is relname and
* conname joined with a space.

The patch's original change to the comment was a reaction to my own surprise.
Reading "pg_dump regression|grep Name:|sort|grep CONSTRAINT" I saw unique
"Name:" output for constraints, which felt at odds with the instability in
DOTypeNameCompare() sorting them. But it wasn't the name I was looking for:

- getConstraints() sets DumpableObject.name = conname
- DOTypeNameCompare() sorts by DumpableObject.name
- dumpConstraint() sets _tocEntry.tag = "relname conname"
- _tocEntry.tag becomes the "Name:" in pg_dump output

Long-term, in a post-scarcity world, I'd do one of these or similar:

a. Change what we store in DumpableObject.name so it matches _tocEntry.tag.
b. Rename field DumpableObject.name, so there's no longer a field called
"name" with contents different from the "Name:" values in pg_dump output.

+ * Sort by encoding, per pg_collation_name_enc_nsp_index.  This is
+ * mostly academic, because CREATE COLLATION has restrictions to make
+ * (nspname, collname) uniquely identify a collation within a given
+ * DatabaseEncoding.  pg_import_system_collations() bypasses those
+ * restrictions, but pg_dump+restore fails after a
+ * pg_import_system_collations('my_schema') that creates collations
+ * for a blend of encodings.

This comment is also useful, but if I were to be critical again, it
does a better job saying why we shouldn't do what the code then does
than why we should.

I've tried the following further refinement. If it's worse, I can go back to
the last version.

diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index ffae7b3..f7d6a03 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -248,7 +250,19 @@ DOTypeNameCompare(const void *p1, const void *p2)
 	if (cmpval != 0)
 		return cmpval;
-	/* To have a stable sort order, break ties for some object types */
+	/*
+	 * To have a stable sort order, break ties for some object types.  Most
+	 * catalogs have a natural key, e.g. pg_proc_proname_args_nsp_index.
+	 * Where the above "namespace" and "name" comparisons don't cover all
+	 * natural key columns, compare the rest here.
+	 *
+	 * The natural key usually refers to other catalogs by surrogate keys.
+	 * Hence, this translates each of those references to the natural key of
+	 * the referenced catalog.  That may descend through multiple levels of
+	 * catalog references.  For example, to sort by pg_proc.proargtypes,
+	 * descend to each pg_type and then further to its pg_namespace, for an
+	 * overall sort by (nspname, typname).
+	 */
 	if (obj1->objType == DO_FUNC || obj1->objType == DO_AGG)
 	{
 		FuncInfo   *fobj1 = *(FuncInfo *const *) p1;
@@ -312,13 +326,16 @@ DOTypeNameCompare(const void *p1, const void *p2)
 		CollInfo   *cobj2 = *(CollInfo *const *) p2;
 		/*
-		 * Sort by encoding, per pg_collation_name_enc_nsp_index.  This is
-		 * mostly academic, because CREATE COLLATION has restrictions to make
-		 * (nspname, collname) uniquely identify a collation within a given
-		 * DatabaseEncoding.  pg_import_system_collations() bypasses those
-		 * restrictions, but pg_dump+restore fails after a
-		 * pg_import_system_collations('my_schema') that creates collations
-		 * for a blend of encodings.
+		 * Sort by encoding, per pg_collation_name_enc_nsp_index.  Wherever
+		 * this changes dump order, restoring the dump fails anyway.  CREATE
+		 * COLLATION can't create a tie for this to break, because it imposes
+		 * restrictions to make (nspname, collname) uniquely identify a
+		 * collation within a given DatabaseEncoding.  While
+		 * pg_import_system_collations() can create a tie, pg_dump+restore
+		 * fails after pg_import_system_collations('my_schema') does so.
+		 * There's little to gain by ignoring one natural key column on the
+		 * basis of those limitations elsewhere, so respect the full natural
+		 * key like we do for other object types.
 		 */
 		cmpval = cobj1->collencoding - cobj2->collencoding;
 		if (cmpval != 0)
#4Robert Haas
robertmhaas@gmail.com
In reply to: Noah Misch (#3)
Re: Test instability when pg_dump orders by OID

On Fri, Jul 18, 2025 at 3:17 PM Noah Misch <noah@leadboat.com> wrote:

This comment is useful, but if I were to be critical, it does a better
job saying what this field isn't than what it is.

True. I've changed it to this:

That looks great.

-       /* To have a stable sort order, break ties for some object types */
+       /*
+        * To have a stable sort order, break ties for some object types.  Most
+        * catalogs have a natural key, e.g. pg_proc_proname_args_nsp_index.
+        * Where the above "namespace" and "name" comparisons don't cover all
+        * natural key columns, compare the rest here.
+        *
+        * The natural key usually refers to other catalogs by surrogate keys.
+        * Hence, this translates each of those references to the natural key of
+        * the referenced catalog.  That may descend through multiple levels of
+        * catalog references.  For example, to sort by pg_proc.proargtypes,
+        * descend to each pg_type and then further to its pg_namespace, for an
+        * overall sort by (nspname, typname).
+        */

I really like this.

+                * Sort by encoding, per pg_collation_name_enc_nsp_index.  Wherever
+                * this changes dump order, restoring the dump fails anyway.  CREATE
+                * COLLATION can't create a tie for this to break, because it imposes
+                * restrictions to make (nspname, collname) uniquely identify a
+                * collation within a given DatabaseEncoding.  While
+                * pg_import_system_collations() can create a tie, pg_dump+restore
+                * fails after pg_import_system_collations('my_schema') does so.
+                * There's little to gain by ignoring one natural key column on the
+                * basis of those limitations elsewhere, so respect the full natural
+                * key like we do for other object types.

This is also good. I suggest s/Wherever/Technically, this is not
necessary, because wherever/ and s/There's/However, there's/.

--
Robert Haas
EDB: http://www.enterprisedb.com

#5Noah Misch
noah@leadboat.com
In reply to: Robert Haas (#4)
5 attachment(s)
Re: Test instability when pg_dump orders by OID

On Mon, Jul 21, 2025 at 09:40:02AM -0400, Robert Haas wrote:

On Fri, Jul 18, 2025 at 3:17 PM Noah Misch <noah@leadboat.com> wrote:

+                * Sort by encoding, per pg_collation_name_enc_nsp_index.  Wherever
+                * this changes dump order, restoring the dump fails anyway.  CREATE
+                * COLLATION can't create a tie for this to break, because it imposes
+                * restrictions to make (nspname, collname) uniquely identify a
+                * collation within a given DatabaseEncoding.  While
+                * pg_import_system_collations() can create a tie, pg_dump+restore
+                * fails after pg_import_system_collations('my_schema') does so.
+                * There's little to gain by ignoring one natural key column on the
+                * basis of those limitations elsewhere, so respect the full natural
+                * key like we do for other object types.

This is also good. I suggest s/Wherever/Technically, this is not
necessary, because wherever/ and s/There's/However, there's/.

I used that. I started to prepare the back-branch versions, but that revealed
three problems affecting the master patch:

(1) Sorting constraints segfaulted if either of a pair of equal-name
constraints was a domain constraint. Fortunately, commit da71717 added a test
case for that between when I mailed patch v1 and when I went to commit. One
can reproduce it by dumping a database containing:

CREATE DOMAIN d1 AS int CONSTRAINT dc CHECK (value > 0);
CREATE DOMAIN d2 AS int CONSTRAINT dc CHECK (value > 0);

I made pg_dump sort domain constraints of a given name before table
constraints of that name, for consistency with our decision to sort CREATE
DOMAIN before CREATE TABLE. The main alternative was to sort by parent object
name irrespective of parent object type, i.e. DOMAIN a < TABLE b < DOMAIN c.
That alternative lacked a relevant precedent. I've now audited the natural
keys of catalogs for which I'm changing sort order, and I think that was the
only one I missed.

(2) Sorting opclasses failed a new assertion when dumping a v9.2 source (and
likely 9.[345]), because getAccessMethods() doesn't read pg_am when dumping
from a version predating CREATE ACCESS METHOD. findAccessMethodByOid() found
no access methods, since pg_dump had read none. I've changed
getAccessMethods() to always read pg_am. (For pre-v9.6 sources, I've kept the
function's behavior of never marking an access method for dumping.) pg_am is
small enough for this read to incur negligible cost. The main alternative
was, for pre-v9.6, sorting access methods by pg_am.oid. That would have been
less code, but dump order would have differed between pre-v9.6 and v9.6+.

(3) pgTypeNameCompare() implied postfix operators don't exist, but master
pg_dump will support reading from pre-v14 clusters for several more years.
The code behaved fine, but I've updated the comment.

I regret missing those in v1. I've attached v2, including branch-specific
patches. I'll first need to back-patch 350e6b8, which fixed sorting of CREATE
RULE, to v17 and earlier. Since 350e6b8 is conflict-free all the way back to
v13, I'm not attaching it.

Thanks,
nm

Attachments:

dobjcmp20-disambiguate-v2.patchtext/plain; charset=us-asciiDownload
From: Noah Misch <noah@leadboat.com>

Sort dump objects independent of OIDs, for the 7 holdout object types.

pg_dump sorts objects by their logical names, e.g. (nspname, relname,
tgname), before dependency-driven reordering.  That removes one source
of logically-identical databases differing in their schema-only dumps.
In other words, it helps with schema diffing.  The logical name sort
ignored essential sort keys for constraints, operators, PUBLICATION
... FOR TABLE, PUBLICATION ... FOR TABLES IN SCHEMA, operator classes,
and operator families.  pg_dump's sort then depended on object OID,
yielding spurious schema diffs.  After this change, OIDs affect dump
order only in the event of catalog corruption.  While pg_dump also
wrongly ignored pg_collation.collencoding, CREATE COLLATION restrictions
have been keeping that imperceptible in practical use.

Use techniques like we use for object types already having full sort key
coverage.  Where the pertinent queries weren't fetching the ignored sort
keys, this adds columns to those queries and stores those keys in memory
for the long term.

The ignorance of sort keys became more problematic when commit
172259afb563d35001410dc6daad78b250924038 added a schema diff test
sensitive to it.  However, dump order stability isn't a new goal, and
this might avoid other dump comparison failures.  Hence, back-patch to
v13 (all supported versions).

Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Discussion: https://postgr.es/m/20250707192654.9e.nmisch@google.com
Backpatch-through: 13

diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index aa1589e..a1976fa 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -17,6 +17,7 @@
 
 #include <ctype.h>
 
+#include "catalog/pg_am_d.h"
 #include "catalog/pg_class_d.h"
 #include "catalog/pg_collation_d.h"
 #include "catalog/pg_extension_d.h"
@@ -945,6 +946,24 @@ findOprByOid(Oid oid)
 }
 
 /*
+ * findAccessMethodByOid
+ *	  finds the DumpableObject for the access method with the given oid
+ *	  returns NULL if not found
+ */
+AccessMethodInfo *
+findAccessMethodByOid(Oid oid)
+{
+	CatalogId	catId;
+	DumpableObject *dobj;
+
+	catId.tableoid = AccessMethodRelationId;
+	catId.oid = oid;
+	dobj = findObjectByCatalogId(catId);
+	Assert(dobj == NULL || dobj->objType == DO_ACCESS_METHOD);
+	return (AccessMethodInfo *) dobj;
+}
+
+/*
  * findCollationByOid
  *	  finds the DumpableObject for the collation with the given oid
  *	  returns NULL if not found
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index ede10e5..1f93e7a 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -2207,6 +2207,13 @@ selectDumpableProcLang(ProcLangInfo *plang, Archive *fout)
 static void
 selectDumpableAccessMethod(AccessMethodInfo *method, Archive *fout)
 {
+	/* see getAccessMethods() comment about v9.6. */
+	if (fout->remoteVersion < 90600)
+	{
+		method->dobj.dump = DUMP_COMPONENT_NONE;
+		return;
+	}
+
 	if (checkExtensionMembership(&method->dobj, fout))
 		return;					/* extension membership overrides all else */
 
@@ -6248,6 +6255,8 @@ getOperators(Archive *fout)
 	int			i_oprnamespace;
 	int			i_oprowner;
 	int			i_oprkind;
+	int			i_oprleft;
+	int			i_oprright;
 	int			i_oprcode;
 
 	/*
@@ -6259,6 +6268,8 @@ getOperators(Archive *fout)
 						 "oprnamespace, "
 						 "oprowner, "
 						 "oprkind, "
+						 "oprleft, "
+						 "oprright, "
 						 "oprcode::oid AS oprcode "
 						 "FROM pg_operator");
 
@@ -6274,6 +6285,8 @@ getOperators(Archive *fout)
 	i_oprnamespace = PQfnumber(res, "oprnamespace");
 	i_oprowner = PQfnumber(res, "oprowner");
 	i_oprkind = PQfnumber(res, "oprkind");
+	i_oprleft = PQfnumber(res, "oprleft");
+	i_oprright = PQfnumber(res, "oprright");
 	i_oprcode = PQfnumber(res, "oprcode");
 
 	for (i = 0; i < ntups; i++)
@@ -6287,6 +6300,8 @@ getOperators(Archive *fout)
 			findNamespace(atooid(PQgetvalue(res, i, i_oprnamespace)));
 		oprinfo[i].rolname = getRoleName(PQgetvalue(res, i, i_oprowner));
 		oprinfo[i].oprkind = (PQgetvalue(res, i, i_oprkind))[0];
+		oprinfo[i].oprleft = atooid(PQgetvalue(res, i, i_oprleft));
+		oprinfo[i].oprright = atooid(PQgetvalue(res, i, i_oprright));
 		oprinfo[i].oprcode = atooid(PQgetvalue(res, i, i_oprcode));
 
 		/* Decide whether we want to dump it */
@@ -6315,6 +6330,7 @@ getCollations(Archive *fout)
 	int			i_collname;
 	int			i_collnamespace;
 	int			i_collowner;
+	int			i_collencoding;
 
 	query = createPQExpBuffer();
 
@@ -6325,7 +6341,8 @@ getCollations(Archive *fout)
 
 	appendPQExpBufferStr(query, "SELECT tableoid, oid, collname, "
 						 "collnamespace, "
-						 "collowner "
+						 "collowner, "
+						 "collencoding "
 						 "FROM pg_collation");
 
 	res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
@@ -6339,6 +6356,7 @@ getCollations(Archive *fout)
 	i_collname = PQfnumber(res, "collname");
 	i_collnamespace = PQfnumber(res, "collnamespace");
 	i_collowner = PQfnumber(res, "collowner");
+	i_collencoding = PQfnumber(res, "collencoding");
 
 	for (i = 0; i < ntups; i++)
 	{
@@ -6350,6 +6368,7 @@ getCollations(Archive *fout)
 		collinfo[i].dobj.namespace =
 			findNamespace(atooid(PQgetvalue(res, i, i_collnamespace)));
 		collinfo[i].rolname = getRoleName(PQgetvalue(res, i, i_collowner));
+		collinfo[i].collencoding = atoi(PQgetvalue(res, i, i_collencoding));
 
 		/* Decide whether we want to dump it */
 		selectDumpableObject(&(collinfo[i].dobj), fout);
@@ -6440,16 +6459,28 @@ getAccessMethods(Archive *fout)
 	int			i_amhandler;
 	int			i_amtype;
 
-	/* Before 9.6, there are no user-defined access methods */
-	if (fout->remoteVersion < 90600)
-		return;
-
 	query = createPQExpBuffer();
 
-	/* Select all access methods from pg_am table */
-	appendPQExpBufferStr(query, "SELECT tableoid, oid, amname, amtype, "
-						 "amhandler::pg_catalog.regproc AS amhandler "
-						 "FROM pg_am");
+	/*
+	 * Select all access methods from pg_am table.  v9.6 introduced CREATE
+	 * ACCESS METHOD, so earlier versions usually have only built-in access
+	 * methods.  v9.6 also changed the access method API, replacing dozens of
+	 * pg_am columns with amhandler.  Even if a user created an access method
+	 * by "INSERT INTO pg_am", we have no way to translate pre-v9.6 pg_am
+	 * columns to a v9.6+ CREATE ACCESS METHOD.  Hence, before v9.6, read
+	 * pg_am just to facilitate findAccessMethodByOid() providing the
+	 * OID-to-name mapping.
+	 */
+	appendPQExpBufferStr(query, "SELECT tableoid, oid, amname, ");
+	if (fout->remoteVersion >= 90600)
+		appendPQExpBufferStr(query,
+							 "amtype, "
+							 "amhandler::pg_catalog.regproc AS amhandler ");
+	else
+		appendPQExpBufferStr(query,
+							 "'i'::pg_catalog.\"char\" AS amtype, "
+							 "'-'::pg_catalog.regproc AS amhandler ");
+	appendPQExpBufferStr(query, "FROM pg_am");
 
 	res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
 
@@ -6498,6 +6529,7 @@ getOpclasses(Archive *fout)
 	OpclassInfo *opcinfo;
 	int			i_tableoid;
 	int			i_oid;
+	int			i_opcmethod;
 	int			i_opcname;
 	int			i_opcnamespace;
 	int			i_opcowner;
@@ -6507,7 +6539,7 @@ getOpclasses(Archive *fout)
 	 * system-defined opclasses at dump-out time.
 	 */
 
-	appendPQExpBufferStr(query, "SELECT tableoid, oid, opcname, "
+	appendPQExpBufferStr(query, "SELECT tableoid, oid, opcmethod, opcname, "
 						 "opcnamespace, "
 						 "opcowner "
 						 "FROM pg_opclass");
@@ -6520,6 +6552,7 @@ getOpclasses(Archive *fout)
 
 	i_tableoid = PQfnumber(res, "tableoid");
 	i_oid = PQfnumber(res, "oid");
+	i_opcmethod = PQfnumber(res, "opcmethod");
 	i_opcname = PQfnumber(res, "opcname");
 	i_opcnamespace = PQfnumber(res, "opcnamespace");
 	i_opcowner = PQfnumber(res, "opcowner");
@@ -6533,6 +6566,7 @@ getOpclasses(Archive *fout)
 		opcinfo[i].dobj.name = pg_strdup(PQgetvalue(res, i, i_opcname));
 		opcinfo[i].dobj.namespace =
 			findNamespace(atooid(PQgetvalue(res, i, i_opcnamespace)));
+		opcinfo[i].opcmethod = atooid(PQgetvalue(res, i, i_opcmethod));
 		opcinfo[i].rolname = getRoleName(PQgetvalue(res, i, i_opcowner));
 
 		/* Decide whether we want to dump it */
@@ -6558,6 +6592,7 @@ getOpfamilies(Archive *fout)
 	OpfamilyInfo *opfinfo;
 	int			i_tableoid;
 	int			i_oid;
+	int			i_opfmethod;
 	int			i_opfname;
 	int			i_opfnamespace;
 	int			i_opfowner;
@@ -6569,7 +6604,7 @@ getOpfamilies(Archive *fout)
 	 * system-defined opfamilies at dump-out time.
 	 */
 
-	appendPQExpBufferStr(query, "SELECT tableoid, oid, opfname, "
+	appendPQExpBufferStr(query, "SELECT tableoid, oid, opfmethod, opfname, "
 						 "opfnamespace, "
 						 "opfowner "
 						 "FROM pg_opfamily");
@@ -6583,6 +6618,7 @@ getOpfamilies(Archive *fout)
 	i_tableoid = PQfnumber(res, "tableoid");
 	i_oid = PQfnumber(res, "oid");
 	i_opfname = PQfnumber(res, "opfname");
+	i_opfmethod = PQfnumber(res, "opfmethod");
 	i_opfnamespace = PQfnumber(res, "opfnamespace");
 	i_opfowner = PQfnumber(res, "opfowner");
 
@@ -6595,6 +6631,7 @@ getOpfamilies(Archive *fout)
 		opfinfo[i].dobj.name = pg_strdup(PQgetvalue(res, i, i_opfname));
 		opfinfo[i].dobj.namespace =
 			findNamespace(atooid(PQgetvalue(res, i, i_opfnamespace)));
+		opfinfo[i].opfmethod = atooid(PQgetvalue(res, i, i_opfmethod));
 		opfinfo[i].rolname = getRoleName(PQgetvalue(res, i, i_opfowner));
 
 		/* Decide whether we want to dump it */
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 2370c98..30121af 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -260,6 +260,8 @@ typedef struct _oprInfo
 	DumpableObject dobj;
 	const char *rolname;
 	char		oprkind;
+	Oid			oprleft;
+	Oid			oprright;
 	Oid			oprcode;
 } OprInfo;
 
@@ -273,12 +275,14 @@ typedef struct _accessMethodInfo
 typedef struct _opclassInfo
 {
 	DumpableObject dobj;
+	Oid			opcmethod;
 	const char *rolname;
 } OpclassInfo;
 
 typedef struct _opfamilyInfo
 {
 	DumpableObject dobj;
+	Oid			opfmethod;
 	const char *rolname;
 } OpfamilyInfo;
 
@@ -286,6 +290,7 @@ typedef struct _collInfo
 {
 	DumpableObject dobj;
 	const char *rolname;
+	int			collencoding;
 } CollInfo;
 
 typedef struct _convInfo
@@ -759,6 +764,7 @@ extern TableInfo *findTableByOid(Oid oid);
 extern TypeInfo *findTypeByOid(Oid oid);
 extern FuncInfo *findFuncByOid(Oid oid);
 extern OprInfo *findOprByOid(Oid oid);
+extern AccessMethodInfo *findAccessMethodByOid(Oid oid);
 extern CollInfo *findCollationByOid(Oid oid);
 extern NamespaceInfo *findNamespaceByOid(Oid oid);
 extern ExtensionInfo *findExtensionByOid(Oid oid);
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index f99a079..a02da3e 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -162,6 +162,8 @@ static DumpId postDataBoundId;
 
 
 static int	DOTypeNameCompare(const void *p1, const void *p2);
+static int	pgTypeNameCompare(Oid typid1, Oid typid2);
+static int	accessMethodNameCompare(Oid am1, Oid am2);
 static bool TopoSort(DumpableObject **objs,
 					 int numObjs,
 					 DumpableObject **ordering,
@@ -228,12 +230,39 @@ DOTypeNameCompare(const void *p1, const void *p2)
 	else if (obj2->namespace)
 		return 1;
 
-	/* Sort by name */
+	/*
+	 * Sort by name.  With a few exceptions, names here are single catalog
+	 * columns.  To get a fuller picture, grep pg_dump.c for "dobj.name = ".
+	 * Names here don't match "Name:" in plain format output, which is a
+	 * _tocEntry.tag.  For example, DumpableObject.name of a constraint is
+	 * pg_constraint.conname, but _tocEntry.tag of a constraint is relname and
+	 * conname joined with a space.
+	 */
 	cmpval = strcmp(obj1->name, obj2->name);
 	if (cmpval != 0)
 		return cmpval;
 
-	/* To have a stable sort order, break ties for some object types */
+	/*
+	 * Sort by type.  This helps types that share a type priority without
+	 * sharing a unique name constraint, e.g. opclass and opfamily.
+	 */
+	cmpval = obj1->objType - obj2->objType;
+	if (cmpval != 0)
+		return cmpval;
+
+	/*
+	 * To have a stable sort order, break ties for some object types.  Most
+	 * catalogs have a natural key, e.g. pg_proc_proname_args_nsp_index. Where
+	 * the above "namespace" and "name" comparisons don't cover all natural
+	 * key columns, compare the rest here.
+	 *
+	 * The natural key usually refers to other catalogs by surrogate keys.
+	 * Hence, this translates each of those references to the natural key of
+	 * the referenced catalog.  That may descend through multiple levels of
+	 * catalog references.  For example, to sort by pg_proc.proargtypes,
+	 * descend to each pg_type and then further to its pg_namespace, for an
+	 * overall sort by (nspname, typname).
+	 */
 	if (obj1->objType == DO_FUNC || obj1->objType == DO_AGG)
 	{
 		FuncInfo   *fobj1 = *(FuncInfo *const *) p1;
@@ -246,22 +275,10 @@ DOTypeNameCompare(const void *p1, const void *p2)
 			return cmpval;
 		for (i = 0; i < fobj1->nargs; i++)
 		{
-			TypeInfo   *argtype1 = findTypeByOid(fobj1->argtypes[i]);
-			TypeInfo   *argtype2 = findTypeByOid(fobj2->argtypes[i]);
-
-			if (argtype1 && argtype2)
-			{
-				if (argtype1->dobj.namespace && argtype2->dobj.namespace)
-				{
-					cmpval = strcmp(argtype1->dobj.namespace->dobj.name,
-									argtype2->dobj.namespace->dobj.name);
-					if (cmpval != 0)
-						return cmpval;
-				}
-				cmpval = strcmp(argtype1->dobj.name, argtype2->dobj.name);
-				if (cmpval != 0)
-					return cmpval;
-			}
+			cmpval = pgTypeNameCompare(fobj1->argtypes[i],
+									   fobj2->argtypes[i]);
+			if (cmpval != 0)
+				return cmpval;
 		}
 	}
 	else if (obj1->objType == DO_OPERATOR)
@@ -273,6 +290,57 @@ DOTypeNameCompare(const void *p1, const void *p2)
 		cmpval = (oobj2->oprkind - oobj1->oprkind);
 		if (cmpval != 0)
 			return cmpval;
+		/* Within an oprkind, sort by argument type names */
+		cmpval = pgTypeNameCompare(oobj1->oprleft, oobj2->oprleft);
+		if (cmpval != 0)
+			return cmpval;
+		cmpval = pgTypeNameCompare(oobj1->oprright, oobj2->oprright);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	else if (obj1->objType == DO_OPCLASS)
+	{
+		OpclassInfo *opcobj1 = *(OpclassInfo *const *) p1;
+		OpclassInfo *opcobj2 = *(OpclassInfo *const *) p2;
+
+		/* Sort by access method name, per pg_opclass_am_name_nsp_index */
+		cmpval = accessMethodNameCompare(opcobj1->opcmethod,
+										 opcobj2->opcmethod);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	else if (obj1->objType == DO_OPFAMILY)
+	{
+		OpfamilyInfo *opfobj1 = *(OpfamilyInfo *const *) p1;
+		OpfamilyInfo *opfobj2 = *(OpfamilyInfo *const *) p2;
+
+		/* Sort by access method name, per pg_opfamily_am_name_nsp_index */
+		cmpval = accessMethodNameCompare(opfobj1->opfmethod,
+										 opfobj2->opfmethod);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	else if (obj1->objType == DO_COLLATION)
+	{
+		CollInfo   *cobj1 = *(CollInfo *const *) p1;
+		CollInfo   *cobj2 = *(CollInfo *const *) p2;
+
+		/*
+		 * Sort by encoding, per pg_collation_name_enc_nsp_index. Technically,
+		 * this is not necessary, because wherever this changes dump order,
+		 * restoring the dump fails anyway.  CREATE COLLATION can't create a
+		 * tie for this to break, because it imposes restrictions to make
+		 * (nspname, collname) uniquely identify a collation within a given
+		 * DatabaseEncoding.  While pg_import_system_collations() can create a
+		 * tie, pg_dump+restore fails after
+		 * pg_import_system_collations('my_schema') does so. However, there's
+		 * little to gain by ignoring one natural key column on the basis of
+		 * those limitations elsewhere, so respect the full natural key like
+		 * we do for other object types.
+		 */
+		cmpval = cobj1->collencoding - cobj2->collencoding;
+		if (cmpval != 0)
+			return cmpval;
 	}
 	else if (obj1->objType == DO_ATTRDEF)
 	{
@@ -317,11 +385,143 @@ DOTypeNameCompare(const void *p1, const void *p2)
 		if (cmpval != 0)
 			return cmpval;
 	}
+	else if (obj1->objType == DO_CONSTRAINT)
+	{
+		ConstraintInfo *robj1 = *(ConstraintInfo *const *) p1;
+		ConstraintInfo *robj2 = *(ConstraintInfo *const *) p2;
 
-	/* Usually shouldn't get here, but if we do, sort by OID */
+		/*
+		 * Sort domain constraints before table constraints, for consistency
+		 * with our decision to sort CREATE DOMAIN before CREATE TABLE.
+		 */
+		if (robj1->condomain)
+		{
+			if (robj2->condomain)
+			{
+				/* Sort by domain name (domain namespace was considered) */
+				cmpval = strcmp(robj1->condomain->dobj.name,
+								robj2->condomain->dobj.name);
+				if (cmpval != 0)
+					return cmpval;
+			}
+			else
+				return PRIO_TYPE - PRIO_TABLE;
+		}
+		else if (robj2->condomain)
+			return PRIO_TABLE - PRIO_TYPE;
+		else
+		{
+			/* Sort by table name (table namespace was considered already) */
+			cmpval = strcmp(robj1->contable->dobj.name,
+							robj2->contable->dobj.name);
+			if (cmpval != 0)
+				return cmpval;
+		}
+	}
+	else if (obj1->objType == DO_PUBLICATION_REL)
+	{
+		PublicationRelInfo *probj1 = *(PublicationRelInfo *const *) p1;
+		PublicationRelInfo *probj2 = *(PublicationRelInfo *const *) p2;
+
+		/* Sort by publication name, since (namespace, name) match the rel */
+		cmpval = strcmp(probj1->publication->dobj.name,
+						probj2->publication->dobj.name);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	else if (obj1->objType == DO_PUBLICATION_TABLE_IN_SCHEMA)
+	{
+		PublicationSchemaInfo *psobj1 = *(PublicationSchemaInfo *const *) p1;
+		PublicationSchemaInfo *psobj2 = *(PublicationSchemaInfo *const *) p2;
+
+		/* Sort by publication name, since ->name is just nspname */
+		cmpval = strcmp(psobj1->publication->dobj.name,
+						psobj2->publication->dobj.name);
+		if (cmpval != 0)
+			return cmpval;
+	}
+
+	/*
+	 * Shouldn't get here except after catalog corruption, but if we do, sort
+	 * by OID.  This may make logically-identical databases differ in the
+	 * order of objects in dump output.  Users will get spurious schema diffs.
+	 * Expect flaky failures of 002_pg_upgrade.pl test 'dump outputs from
+	 * original and restored regression databases match' if the regression
+	 * database contains objects allowing that test to reach here.  That's a
+	 * consequence of the test using "pg_restore -j", which doesn't fully
+	 * constrain OID assignment order.
+	 */
+	Assert(false);
 	return oidcmp(obj1->catId.oid, obj2->catId.oid);
 }
 
+/* Compare two OID-identified pg_type values by nspname, then by typname. */
+static int
+pgTypeNameCompare(Oid typid1, Oid typid2)
+{
+	TypeInfo   *typobj1;
+	TypeInfo   *typobj2;
+	int			cmpval;
+
+	if (typid1 == typid2)
+		return 0;
+
+	typobj1 = findTypeByOid(typid1);
+	typobj2 = findTypeByOid(typid2);
+
+	if (!typobj1 || !typobj2)
+	{
+		/*
+		 * getTypes() didn't find some OID.  Assume catalog corruption, e.g.
+		 * an oprright value without the corresponding OID in a pg_type row.
+		 * Report as "equal", so the caller uses the next available basis for
+		 * comparison, e.g. the next function argument.
+		 *
+		 * Unary operators have InvalidOid in oprleft (if oprkind='r') or in
+		 * oprright (if oprkind='l').  Caller already sorted by oprkind,
+		 * calling us only for like-kind operators.  Hence, "typid1 == typid2"
+		 * took care of InvalidOid.  (v14 removed postfix operator support.
+		 * Hence, when dumping from v14+, only oprleft can be InvalidOid.)
+		 */
+		Assert(false);
+		return 0;
+	}
+
+	if (!typobj1->dobj.namespace || !typobj2->dobj.namespace)
+		Assert(false);			/* catalog corruption */
+	else
+	{
+		cmpval = strcmp(typobj1->dobj.namespace->dobj.name,
+						typobj2->dobj.namespace->dobj.name);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	return strcmp(typobj1->dobj.name, typobj2->dobj.name);
+}
+
+/* Compare two OID-identified pg_am values by amname. */
+static int
+accessMethodNameCompare(Oid am1, Oid am2)
+{
+	AccessMethodInfo *amobj1;
+	AccessMethodInfo *amobj2;
+
+	if (am1 == am2)
+		return 0;
+
+	amobj1 = findAccessMethodByOid(am1);
+	amobj2 = findAccessMethodByOid(am2);
+
+	if (!amobj1 || !amobj2)
+	{
+		/* catalog corruption: handle like pgTypeNameCompare() does */
+		Assert(false);
+		return 0;
+	}
+
+	return strcmp(amobj1->dobj.name, amobj2->dobj.name);
+}
+
 
 /*
  * Sort the given objects into a safe dump order using dependency
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 3a2eacd..1ec3fa3 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -1934,3 +1934,24 @@ RESET client_min_messages;
 RESET SESSION AUTHORIZATION;
 DROP ROLE regress_publication_user, regress_publication_user2;
 DROP ROLE regress_publication_user_dummy;
+-- stage objects for pg_dump tests
+CREATE SCHEMA pubme CREATE TABLE t0 (c int, d int) CREATE TABLE t1 (c int);
+CREATE SCHEMA pubme2 CREATE TABLE t0 (c int, d int);
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION dump_pub_qual_1ct FOR
+  TABLE ONLY pubme.t0 (c, d) WHERE (c > 0);
+CREATE PUBLICATION dump_pub_qual_2ct FOR
+  TABLE ONLY pubme.t0 (c) WHERE (c > 0),
+  TABLE ONLY pubme.t1 (c);
+CREATE PUBLICATION dump_pub_nsp_1ct FOR
+  TABLES IN SCHEMA pubme;
+CREATE PUBLICATION dump_pub_nsp_2ct FOR
+  TABLES IN SCHEMA pubme,
+  TABLES IN SCHEMA pubme2;
+CREATE PUBLICATION dump_pub_all FOR
+  TABLE ONLY pubme.t0,
+  TABLE ONLY pubme.t1 WHERE (c < 0),
+  TABLES IN SCHEMA pubme,
+  TABLES IN SCHEMA pubme2
+  WITH (publish_via_partition_root = true);
+RESET client_min_messages;
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index c9e3091..2585f08 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -1229,3 +1229,25 @@ RESET client_min_messages;
 RESET SESSION AUTHORIZATION;
 DROP ROLE regress_publication_user, regress_publication_user2;
 DROP ROLE regress_publication_user_dummy;
+
+-- stage objects for pg_dump tests
+CREATE SCHEMA pubme CREATE TABLE t0 (c int, d int) CREATE TABLE t1 (c int);
+CREATE SCHEMA pubme2 CREATE TABLE t0 (c int, d int);
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION dump_pub_qual_1ct FOR
+  TABLE ONLY pubme.t0 (c, d) WHERE (c > 0);
+CREATE PUBLICATION dump_pub_qual_2ct FOR
+  TABLE ONLY pubme.t0 (c) WHERE (c > 0),
+  TABLE ONLY pubme.t1 (c);
+CREATE PUBLICATION dump_pub_nsp_1ct FOR
+  TABLES IN SCHEMA pubme;
+CREATE PUBLICATION dump_pub_nsp_2ct FOR
+  TABLES IN SCHEMA pubme,
+  TABLES IN SCHEMA pubme2;
+CREATE PUBLICATION dump_pub_all FOR
+  TABLE ONLY pubme.t0,
+  TABLE ONLY pubme.t1 WHERE (c < 0),
+  TABLES IN SCHEMA pubme,
+  TABLES IN SCHEMA pubme2
+  WITH (publish_via_partition_root = true);
+RESET client_min_messages;
dobjcmp20-disambiguate-v2_17.patchtext/plain; charset=us-asciiDownload
commit 4d82220 (HEAD, zzy_test-commit-REL_17_STABLE)
Author:     Noah Misch <noah@leadboat.com>
AuthorDate: Thu Jul 24 17:21:35 2025 -0700
Commit:     Noah Misch <noah@leadboat.com>
CommitDate: Thu Jul 24 17:23:44 2025 -0700

    Sort dump objects independent of OIDs, for the 7 holdout object types.
    
    pg_dump sorts objects by their logical names, e.g. (nspname, relname,
    tgname), before dependency-driven reordering.  That removes one source
    of logically-identical databases differing in their schema-only dumps.
    In other words, it helps with schema diffing.  The logical name sort
    ignored essential sort keys for constraints, operators, PUBLICATION
    ... FOR TABLE, PUBLICATION ... FOR TABLES IN SCHEMA, operator classes,
    and operator families.  pg_dump's sort then depended on object OID,
    yielding spurious schema diffs.  After this change, OIDs affect dump
    order only in the event of catalog corruption.  While pg_dump also
    wrongly ignored pg_collation.collencoding, CREATE COLLATION restrictions
    have been keeping that imperceptible in practical use.
    
    Use techniques like we use for object types already having full sort key
    coverage.  Where the pertinent queries weren't fetching the ignored sort
    keys, this adds columns to those queries and stores those keys in memory
    for the long term.
    
    The ignorance of sort keys became more problematic when commit
    172259afb563d35001410dc6daad78b250924038 added a schema diff test
    sensitive to it.  However, dump order stability isn't a new goal, and
    this might avoid other dump comparison failures.  Hence, back-patch to
    v13 (all supported versions).
    
    Reviewed-by: Robert Haas <robertmhaas@gmail.com>
    Discussion: https://postgr.es/m/20250707192654.9e.nmisch@google.com
    Backpatch-through: 13
    
    Conflicts:
    	src/bin/pg_dump/pg_dump.c
---
 src/bin/pg_dump/common.c                  |  19 +++
 src/bin/pg_dump/pg_dump.c                 |  62 ++++++--
 src/bin/pg_dump/pg_dump.h                 |   6 +
 src/bin/pg_dump/pg_dump_sort.c            | 238 +++++++++++++++++++++++++++---
 src/test/regress/expected/publication.out |  21 +++
 src/test/regress/sql/publication.sql      |  22 +++
 6 files changed, 335 insertions(+), 33 deletions(-)

diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index 64e7dc8..74bbea7 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -17,6 +17,7 @@
 
 #include <ctype.h>
 
+#include "catalog/pg_am_d.h"
 #include "catalog/pg_class_d.h"
 #include "catalog/pg_collation_d.h"
 #include "catalog/pg_extension_d.h"
@@ -934,6 +935,24 @@ findOprByOid(Oid oid)
 }
 
 /*
+ * findAccessMethodByOid
+ *	  finds the DumpableObject for the access method with the given oid
+ *	  returns NULL if not found
+ */
+AccessMethodInfo *
+findAccessMethodByOid(Oid oid)
+{
+	CatalogId	catId;
+	DumpableObject *dobj;
+
+	catId.tableoid = AccessMethodRelationId;
+	catId.oid = oid;
+	dobj = findObjectByCatalogId(catId);
+	Assert(dobj == NULL || dobj->objType == DO_ACCESS_METHOD);
+	return (AccessMethodInfo *) dobj;
+}
+
+/*
  * findCollationByOid
  *	  finds the DumpableObject for the collation with the given oid
  *	  returns NULL if not found
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 2626dd2..0f26b01 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -2012,6 +2012,13 @@ selectDumpableProcLang(ProcLangInfo *plang, Archive *fout)
 static void
 selectDumpableAccessMethod(AccessMethodInfo *method, Archive *fout)
 {
+	/* see getAccessMethods() comment about v9.6. */
+	if (fout->remoteVersion < 90600)
+	{
+		method->dobj.dump = DUMP_COMPONENT_NONE;
+		return;
+	}
+
 	if (checkExtensionMembership(&method->dobj, fout))
 		return;					/* extension membership overrides all else */
 
@@ -5997,6 +6004,8 @@ getOperators(Archive *fout, int *numOprs)
 	int			i_oprnamespace;
 	int			i_oprowner;
 	int			i_oprkind;
+	int			i_oprleft;
+	int			i_oprright;
 	int			i_oprcode;
 
 	/*
@@ -6008,6 +6017,8 @@ getOperators(Archive *fout, int *numOprs)
 						 "oprnamespace, "
 						 "oprowner, "
 						 "oprkind, "
+						 "oprleft, "
+						 "oprright, "
 						 "oprcode::oid AS oprcode "
 						 "FROM pg_operator");
 
@@ -6024,6 +6035,8 @@ getOperators(Archive *fout, int *numOprs)
 	i_oprnamespace = PQfnumber(res, "oprnamespace");
 	i_oprowner = PQfnumber(res, "oprowner");
 	i_oprkind = PQfnumber(res, "oprkind");
+	i_oprleft = PQfnumber(res, "oprleft");
+	i_oprright = PQfnumber(res, "oprright");
 	i_oprcode = PQfnumber(res, "oprcode");
 
 	for (i = 0; i < ntups; i++)
@@ -6037,6 +6050,8 @@ getOperators(Archive *fout, int *numOprs)
 			findNamespace(atooid(PQgetvalue(res, i, i_oprnamespace)));
 		oprinfo[i].rolname = getRoleName(PQgetvalue(res, i, i_oprowner));
 		oprinfo[i].oprkind = (PQgetvalue(res, i, i_oprkind))[0];
+		oprinfo[i].oprleft = atooid(PQgetvalue(res, i, i_oprleft));
+		oprinfo[i].oprright = atooid(PQgetvalue(res, i, i_oprright));
 		oprinfo[i].oprcode = atooid(PQgetvalue(res, i, i_oprcode));
 
 		/* Decide whether we want to dump it */
@@ -6070,6 +6085,7 @@ getCollations(Archive *fout, int *numCollations)
 	int			i_collname;
 	int			i_collnamespace;
 	int			i_collowner;
+	int			i_collencoding;
 
 	query = createPQExpBuffer();
 
@@ -6080,7 +6096,8 @@ getCollations(Archive *fout, int *numCollations)
 
 	appendPQExpBufferStr(query, "SELECT tableoid, oid, collname, "
 						 "collnamespace, "
-						 "collowner "
+						 "collowner, "
+						 "collencoding "
 						 "FROM pg_collation");
 
 	res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
@@ -6095,6 +6112,7 @@ getCollations(Archive *fout, int *numCollations)
 	i_collname = PQfnumber(res, "collname");
 	i_collnamespace = PQfnumber(res, "collnamespace");
 	i_collowner = PQfnumber(res, "collowner");
+	i_collencoding = PQfnumber(res, "collencoding");
 
 	for (i = 0; i < ntups; i++)
 	{
@@ -6106,6 +6124,7 @@ getCollations(Archive *fout, int *numCollations)
 		collinfo[i].dobj.namespace =
 			findNamespace(atooid(PQgetvalue(res, i, i_collnamespace)));
 		collinfo[i].rolname = getRoleName(PQgetvalue(res, i, i_collowner));
+		collinfo[i].collencoding = atoi(PQgetvalue(res, i, i_collencoding));
 
 		/* Decide whether we want to dump it */
 		selectDumpableObject(&(collinfo[i].dobj), fout);
@@ -6207,19 +6226,28 @@ getAccessMethods(Archive *fout, int *numAccessMethods)
 	int			i_amhandler;
 	int			i_amtype;
 
-	/* Before 9.6, there are no user-defined access methods */
-	if (fout->remoteVersion < 90600)
-	{
-		*numAccessMethods = 0;
-		return NULL;
-	}
-
 	query = createPQExpBuffer();
 
-	/* Select all access methods from pg_am table */
-	appendPQExpBufferStr(query, "SELECT tableoid, oid, amname, amtype, "
-						 "amhandler::pg_catalog.regproc AS amhandler "
-						 "FROM pg_am");
+	/*
+	 * Select all access methods from pg_am table.  v9.6 introduced CREATE
+	 * ACCESS METHOD, so earlier versions usually have only built-in access
+	 * methods.  v9.6 also changed the access method API, replacing dozens of
+	 * pg_am columns with amhandler.  Even if a user created an access method
+	 * by "INSERT INTO pg_am", we have no way to translate pre-v9.6 pg_am
+	 * columns to a v9.6+ CREATE ACCESS METHOD.  Hence, before v9.6, read
+	 * pg_am just to facilitate findAccessMethodByOid() providing the
+	 * OID-to-name mapping.
+	 */
+	appendPQExpBufferStr(query, "SELECT tableoid, oid, amname, ");
+	if (fout->remoteVersion >= 90600)
+		appendPQExpBufferStr(query,
+							 "amtype, "
+							 "amhandler::pg_catalog.regproc AS amhandler ");
+	else
+		appendPQExpBufferStr(query,
+							 "'i'::pg_catalog.\"char\" AS amtype, "
+							 "'-'::pg_catalog.regproc AS amhandler ");
+	appendPQExpBufferStr(query, "FROM pg_am");
 
 	res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
 
@@ -6274,6 +6302,7 @@ getOpclasses(Archive *fout, int *numOpclasses)
 	OpclassInfo *opcinfo;
 	int			i_tableoid;
 	int			i_oid;
+	int			i_opcmethod;
 	int			i_opcname;
 	int			i_opcnamespace;
 	int			i_opcowner;
@@ -6283,7 +6312,7 @@ getOpclasses(Archive *fout, int *numOpclasses)
 	 * system-defined opclasses at dump-out time.
 	 */
 
-	appendPQExpBufferStr(query, "SELECT tableoid, oid, opcname, "
+	appendPQExpBufferStr(query, "SELECT tableoid, oid, opcmethod, opcname, "
 						 "opcnamespace, "
 						 "opcowner "
 						 "FROM pg_opclass");
@@ -6297,6 +6326,7 @@ getOpclasses(Archive *fout, int *numOpclasses)
 
 	i_tableoid = PQfnumber(res, "tableoid");
 	i_oid = PQfnumber(res, "oid");
+	i_opcmethod = PQfnumber(res, "opcmethod");
 	i_opcname = PQfnumber(res, "opcname");
 	i_opcnamespace = PQfnumber(res, "opcnamespace");
 	i_opcowner = PQfnumber(res, "opcowner");
@@ -6310,6 +6340,7 @@ getOpclasses(Archive *fout, int *numOpclasses)
 		opcinfo[i].dobj.name = pg_strdup(PQgetvalue(res, i, i_opcname));
 		opcinfo[i].dobj.namespace =
 			findNamespace(atooid(PQgetvalue(res, i, i_opcnamespace)));
+		opcinfo[i].opcmethod = atooid(PQgetvalue(res, i, i_opcmethod));
 		opcinfo[i].rolname = getRoleName(PQgetvalue(res, i, i_opcowner));
 
 		/* Decide whether we want to dump it */
@@ -6340,6 +6371,7 @@ getOpfamilies(Archive *fout, int *numOpfamilies)
 	OpfamilyInfo *opfinfo;
 	int			i_tableoid;
 	int			i_oid;
+	int			i_opfmethod;
 	int			i_opfname;
 	int			i_opfnamespace;
 	int			i_opfowner;
@@ -6351,7 +6383,7 @@ getOpfamilies(Archive *fout, int *numOpfamilies)
 	 * system-defined opfamilies at dump-out time.
 	 */
 
-	appendPQExpBufferStr(query, "SELECT tableoid, oid, opfname, "
+	appendPQExpBufferStr(query, "SELECT tableoid, oid, opfmethod, opfname, "
 						 "opfnamespace, "
 						 "opfowner "
 						 "FROM pg_opfamily");
@@ -6366,6 +6398,7 @@ getOpfamilies(Archive *fout, int *numOpfamilies)
 	i_tableoid = PQfnumber(res, "tableoid");
 	i_oid = PQfnumber(res, "oid");
 	i_opfname = PQfnumber(res, "opfname");
+	i_opfmethod = PQfnumber(res, "opfmethod");
 	i_opfnamespace = PQfnumber(res, "opfnamespace");
 	i_opfowner = PQfnumber(res, "opfowner");
 
@@ -6378,6 +6411,7 @@ getOpfamilies(Archive *fout, int *numOpfamilies)
 		opfinfo[i].dobj.name = pg_strdup(PQgetvalue(res, i, i_opfname));
 		opfinfo[i].dobj.namespace =
 			findNamespace(atooid(PQgetvalue(res, i, i_opfnamespace)));
+		opfinfo[i].opfmethod = atooid(PQgetvalue(res, i, i_opfmethod));
 		opfinfo[i].rolname = getRoleName(PQgetvalue(res, i, i_opfowner));
 
 		/* Decide whether we want to dump it */
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 2439423..2de5afd 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -253,6 +253,8 @@ typedef struct _oprInfo
 	DumpableObject dobj;
 	const char *rolname;
 	char		oprkind;
+	Oid			oprleft;
+	Oid			oprright;
 	Oid			oprcode;
 } OprInfo;
 
@@ -266,12 +268,14 @@ typedef struct _accessMethodInfo
 typedef struct _opclassInfo
 {
 	DumpableObject dobj;
+	Oid			opcmethod;
 	const char *rolname;
 } OpclassInfo;
 
 typedef struct _opfamilyInfo
 {
 	DumpableObject dobj;
+	Oid			opfmethod;
 	const char *rolname;
 } OpfamilyInfo;
 
@@ -279,6 +283,7 @@ typedef struct _collInfo
 {
 	DumpableObject dobj;
 	const char *rolname;
+	int			collencoding;
 } CollInfo;
 
 typedef struct _convInfo
@@ -723,6 +728,7 @@ extern TableInfo *findTableByOid(Oid oid);
 extern TypeInfo *findTypeByOid(Oid oid);
 extern FuncInfo *findFuncByOid(Oid oid);
 extern OprInfo *findOprByOid(Oid oid);
+extern AccessMethodInfo *findAccessMethodByOid(Oid oid);
 extern CollInfo *findCollationByOid(Oid oid);
 extern NamespaceInfo *findNamespaceByOid(Oid oid);
 extern ExtensionInfo *findExtensionByOid(Oid oid);
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index bb31c15..62e27b8 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -161,6 +161,8 @@ static DumpId postDataBoundId;
 
 
 static int	DOTypeNameCompare(const void *p1, const void *p2);
+static int	pgTypeNameCompare(Oid typid1, Oid typid2);
+static int	accessMethodNameCompare(Oid am1, Oid am2);
 static bool TopoSort(DumpableObject **objs,
 					 int numObjs,
 					 DumpableObject **ordering,
@@ -227,12 +229,39 @@ DOTypeNameCompare(const void *p1, const void *p2)
 	else if (obj2->namespace)
 		return 1;
 
-	/* Sort by name */
+	/*
+	 * Sort by name.  With a few exceptions, names here are single catalog
+	 * columns.  To get a fuller picture, grep pg_dump.c for "dobj.name = ".
+	 * Names here don't match "Name:" in plain format output, which is a
+	 * _tocEntry.tag.  For example, DumpableObject.name of a constraint is
+	 * pg_constraint.conname, but _tocEntry.tag of a constraint is relname and
+	 * conname joined with a space.
+	 */
 	cmpval = strcmp(obj1->name, obj2->name);
 	if (cmpval != 0)
 		return cmpval;
 
-	/* To have a stable sort order, break ties for some object types */
+	/*
+	 * Sort by type.  This helps types that share a type priority without
+	 * sharing a unique name constraint, e.g. opclass and opfamily.
+	 */
+	cmpval = obj1->objType - obj2->objType;
+	if (cmpval != 0)
+		return cmpval;
+
+	/*
+	 * To have a stable sort order, break ties for some object types.  Most
+	 * catalogs have a natural key, e.g. pg_proc_proname_args_nsp_index. Where
+	 * the above "namespace" and "name" comparisons don't cover all natural
+	 * key columns, compare the rest here.
+	 *
+	 * The natural key usually refers to other catalogs by surrogate keys.
+	 * Hence, this translates each of those references to the natural key of
+	 * the referenced catalog.  That may descend through multiple levels of
+	 * catalog references.  For example, to sort by pg_proc.proargtypes,
+	 * descend to each pg_type and then further to its pg_namespace, for an
+	 * overall sort by (nspname, typname).
+	 */
 	if (obj1->objType == DO_FUNC || obj1->objType == DO_AGG)
 	{
 		FuncInfo   *fobj1 = *(FuncInfo *const *) p1;
@@ -245,22 +274,10 @@ DOTypeNameCompare(const void *p1, const void *p2)
 			return cmpval;
 		for (i = 0; i < fobj1->nargs; i++)
 		{
-			TypeInfo   *argtype1 = findTypeByOid(fobj1->argtypes[i]);
-			TypeInfo   *argtype2 = findTypeByOid(fobj2->argtypes[i]);
-
-			if (argtype1 && argtype2)
-			{
-				if (argtype1->dobj.namespace && argtype2->dobj.namespace)
-				{
-					cmpval = strcmp(argtype1->dobj.namespace->dobj.name,
-									argtype2->dobj.namespace->dobj.name);
-					if (cmpval != 0)
-						return cmpval;
-				}
-				cmpval = strcmp(argtype1->dobj.name, argtype2->dobj.name);
-				if (cmpval != 0)
-					return cmpval;
-			}
+			cmpval = pgTypeNameCompare(fobj1->argtypes[i],
+									   fobj2->argtypes[i]);
+			if (cmpval != 0)
+				return cmpval;
 		}
 	}
 	else if (obj1->objType == DO_OPERATOR)
@@ -272,6 +289,57 @@ DOTypeNameCompare(const void *p1, const void *p2)
 		cmpval = (oobj2->oprkind - oobj1->oprkind);
 		if (cmpval != 0)
 			return cmpval;
+		/* Within an oprkind, sort by argument type names */
+		cmpval = pgTypeNameCompare(oobj1->oprleft, oobj2->oprleft);
+		if (cmpval != 0)
+			return cmpval;
+		cmpval = pgTypeNameCompare(oobj1->oprright, oobj2->oprright);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	else if (obj1->objType == DO_OPCLASS)
+	{
+		OpclassInfo *opcobj1 = *(OpclassInfo *const *) p1;
+		OpclassInfo *opcobj2 = *(OpclassInfo *const *) p2;
+
+		/* Sort by access method name, per pg_opclass_am_name_nsp_index */
+		cmpval = accessMethodNameCompare(opcobj1->opcmethod,
+										 opcobj2->opcmethod);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	else if (obj1->objType == DO_OPFAMILY)
+	{
+		OpfamilyInfo *opfobj1 = *(OpfamilyInfo *const *) p1;
+		OpfamilyInfo *opfobj2 = *(OpfamilyInfo *const *) p2;
+
+		/* Sort by access method name, per pg_opfamily_am_name_nsp_index */
+		cmpval = accessMethodNameCompare(opfobj1->opfmethod,
+										 opfobj2->opfmethod);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	else if (obj1->objType == DO_COLLATION)
+	{
+		CollInfo   *cobj1 = *(CollInfo *const *) p1;
+		CollInfo   *cobj2 = *(CollInfo *const *) p2;
+
+		/*
+		 * Sort by encoding, per pg_collation_name_enc_nsp_index. Technically,
+		 * this is not necessary, because wherever this changes dump order,
+		 * restoring the dump fails anyway.  CREATE COLLATION can't create a
+		 * tie for this to break, because it imposes restrictions to make
+		 * (nspname, collname) uniquely identify a collation within a given
+		 * DatabaseEncoding.  While pg_import_system_collations() can create a
+		 * tie, pg_dump+restore fails after
+		 * pg_import_system_collations('my_schema') does so. However, there's
+		 * little to gain by ignoring one natural key column on the basis of
+		 * those limitations elsewhere, so respect the full natural key like
+		 * we do for other object types.
+		 */
+		cmpval = cobj1->collencoding - cobj2->collencoding;
+		if (cmpval != 0)
+			return cmpval;
 	}
 	else if (obj1->objType == DO_ATTRDEF)
 	{
@@ -316,11 +384,143 @@ DOTypeNameCompare(const void *p1, const void *p2)
 		if (cmpval != 0)
 			return cmpval;
 	}
+	else if (obj1->objType == DO_CONSTRAINT)
+	{
+		ConstraintInfo *robj1 = *(ConstraintInfo *const *) p1;
+		ConstraintInfo *robj2 = *(ConstraintInfo *const *) p2;
 
-	/* Usually shouldn't get here, but if we do, sort by OID */
+		/*
+		 * Sort domain constraints before table constraints, for consistency
+		 * with our decision to sort CREATE DOMAIN before CREATE TABLE.
+		 */
+		if (robj1->condomain)
+		{
+			if (robj2->condomain)
+			{
+				/* Sort by domain name (domain namespace was considered) */
+				cmpval = strcmp(robj1->condomain->dobj.name,
+								robj2->condomain->dobj.name);
+				if (cmpval != 0)
+					return cmpval;
+			}
+			else
+				return PRIO_TYPE - PRIO_TABLE;
+		}
+		else if (robj2->condomain)
+			return PRIO_TABLE - PRIO_TYPE;
+		else
+		{
+			/* Sort by table name (table namespace was considered already) */
+			cmpval = strcmp(robj1->contable->dobj.name,
+							robj2->contable->dobj.name);
+			if (cmpval != 0)
+				return cmpval;
+		}
+	}
+	else if (obj1->objType == DO_PUBLICATION_REL)
+	{
+		PublicationRelInfo *probj1 = *(PublicationRelInfo *const *) p1;
+		PublicationRelInfo *probj2 = *(PublicationRelInfo *const *) p2;
+
+		/* Sort by publication name, since (namespace, name) match the rel */
+		cmpval = strcmp(probj1->publication->dobj.name,
+						probj2->publication->dobj.name);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	else if (obj1->objType == DO_PUBLICATION_TABLE_IN_SCHEMA)
+	{
+		PublicationSchemaInfo *psobj1 = *(PublicationSchemaInfo *const *) p1;
+		PublicationSchemaInfo *psobj2 = *(PublicationSchemaInfo *const *) p2;
+
+		/* Sort by publication name, since ->name is just nspname */
+		cmpval = strcmp(psobj1->publication->dobj.name,
+						psobj2->publication->dobj.name);
+		if (cmpval != 0)
+			return cmpval;
+	}
+
+	/*
+	 * Shouldn't get here except after catalog corruption, but if we do, sort
+	 * by OID.  This may make logically-identical databases differ in the
+	 * order of objects in dump output.  Users will get spurious schema diffs.
+	 * Expect flaky failures of 002_pg_upgrade.pl test 'dump outputs from
+	 * original and restored regression databases match' if the regression
+	 * database contains objects allowing that test to reach here.  That's a
+	 * consequence of the test using "pg_restore -j", which doesn't fully
+	 * constrain OID assignment order.
+	 */
+	Assert(false);
 	return oidcmp(obj1->catId.oid, obj2->catId.oid);
 }
 
+/* Compare two OID-identified pg_type values by nspname, then by typname. */
+static int
+pgTypeNameCompare(Oid typid1, Oid typid2)
+{
+	TypeInfo   *typobj1;
+	TypeInfo   *typobj2;
+	int			cmpval;
+
+	if (typid1 == typid2)
+		return 0;
+
+	typobj1 = findTypeByOid(typid1);
+	typobj2 = findTypeByOid(typid2);
+
+	if (!typobj1 || !typobj2)
+	{
+		/*
+		 * getTypes() didn't find some OID.  Assume catalog corruption, e.g.
+		 * an oprright value without the corresponding OID in a pg_type row.
+		 * Report as "equal", so the caller uses the next available basis for
+		 * comparison, e.g. the next function argument.
+		 *
+		 * Unary operators have InvalidOid in oprleft (if oprkind='r') or in
+		 * oprright (if oprkind='l').  Caller already sorted by oprkind,
+		 * calling us only for like-kind operators.  Hence, "typid1 == typid2"
+		 * took care of InvalidOid.  (v14 removed postfix operator support.
+		 * Hence, when dumping from v14+, only oprleft can be InvalidOid.)
+		 */
+		Assert(false);
+		return 0;
+	}
+
+	if (!typobj1->dobj.namespace || !typobj2->dobj.namespace)
+		Assert(false);			/* catalog corruption */
+	else
+	{
+		cmpval = strcmp(typobj1->dobj.namespace->dobj.name,
+						typobj2->dobj.namespace->dobj.name);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	return strcmp(typobj1->dobj.name, typobj2->dobj.name);
+}
+
+/* Compare two OID-identified pg_am values by amname. */
+static int
+accessMethodNameCompare(Oid am1, Oid am2)
+{
+	AccessMethodInfo *amobj1;
+	AccessMethodInfo *amobj2;
+
+	if (am1 == am2)
+		return 0;
+
+	amobj1 = findAccessMethodByOid(am1);
+	amobj2 = findAccessMethodByOid(am2);
+
+	if (!amobj1 || !amobj2)
+	{
+		/* catalog corruption: handle like pgTypeNameCompare() does */
+		Assert(false);
+		return 0;
+	}
+
+	return strcmp(amobj1->dobj.name, amobj2->dobj.name);
+}
+
 
 /*
  * Sort the given objects into a safe dump order using dependency
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 30b6371..3edf0be 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -1745,3 +1745,24 @@ DROP SCHEMA sch2 cascade;
 RESET SESSION AUTHORIZATION;
 DROP ROLE regress_publication_user, regress_publication_user2;
 DROP ROLE regress_publication_user_dummy;
+-- stage objects for pg_dump tests
+CREATE SCHEMA pubme CREATE TABLE t0 (c int, d int) CREATE TABLE t1 (c int);
+CREATE SCHEMA pubme2 CREATE TABLE t0 (c int, d int);
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION dump_pub_qual_1ct FOR
+  TABLE ONLY pubme.t0 (c, d) WHERE (c > 0);
+CREATE PUBLICATION dump_pub_qual_2ct FOR
+  TABLE ONLY pubme.t0 (c) WHERE (c > 0),
+  TABLE ONLY pubme.t1 (c);
+CREATE PUBLICATION dump_pub_nsp_1ct FOR
+  TABLES IN SCHEMA pubme;
+CREATE PUBLICATION dump_pub_nsp_2ct FOR
+  TABLES IN SCHEMA pubme,
+  TABLES IN SCHEMA pubme2;
+CREATE PUBLICATION dump_pub_all FOR
+  TABLE ONLY pubme.t0,
+  TABLE ONLY pubme.t1 WHERE (c < 0),
+  TABLES IN SCHEMA pubme,
+  TABLES IN SCHEMA pubme2
+  WITH (publish_via_partition_root = true);
+RESET client_min_messages;
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index 479d4f3..c4f12d4 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -1109,3 +1109,25 @@ DROP SCHEMA sch2 cascade;
 RESET SESSION AUTHORIZATION;
 DROP ROLE regress_publication_user, regress_publication_user2;
 DROP ROLE regress_publication_user_dummy;
+
+-- stage objects for pg_dump tests
+CREATE SCHEMA pubme CREATE TABLE t0 (c int, d int) CREATE TABLE t1 (c int);
+CREATE SCHEMA pubme2 CREATE TABLE t0 (c int, d int);
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION dump_pub_qual_1ct FOR
+  TABLE ONLY pubme.t0 (c, d) WHERE (c > 0);
+CREATE PUBLICATION dump_pub_qual_2ct FOR
+  TABLE ONLY pubme.t0 (c) WHERE (c > 0),
+  TABLE ONLY pubme.t1 (c);
+CREATE PUBLICATION dump_pub_nsp_1ct FOR
+  TABLES IN SCHEMA pubme;
+CREATE PUBLICATION dump_pub_nsp_2ct FOR
+  TABLES IN SCHEMA pubme,
+  TABLES IN SCHEMA pubme2;
+CREATE PUBLICATION dump_pub_all FOR
+  TABLE ONLY pubme.t0,
+  TABLE ONLY pubme.t1 WHERE (c < 0),
+  TABLES IN SCHEMA pubme,
+  TABLES IN SCHEMA pubme2
+  WITH (publish_via_partition_root = true);
+RESET client_min_messages;
dobjcmp20-disambiguate-v2_15.patchtext/plain; charset=us-asciiDownload
commit 4ca0d70 (HEAD, zzy_test-commit-REL_15_STABLE)
Author:     Noah Misch <noah@leadboat.com>
AuthorDate: Thu Jul 24 17:25:43 2025 -0700
Commit:     Noah Misch <noah@leadboat.com>
CommitDate: Thu Jul 24 17:26:18 2025 -0700

    Sort dump objects independent of OIDs, for the 7 holdout object types.
    
    pg_dump sorts objects by their logical names, e.g. (nspname, relname,
    tgname), before dependency-driven reordering.  That removes one source
    of logically-identical databases differing in their schema-only dumps.
    In other words, it helps with schema diffing.  The logical name sort
    ignored essential sort keys for constraints, operators, PUBLICATION
    ... FOR TABLE, PUBLICATION ... FOR TABLES IN SCHEMA, operator classes,
    and operator families.  pg_dump's sort then depended on object OID,
    yielding spurious schema diffs.  After this change, OIDs affect dump
    order only in the event of catalog corruption.  While pg_dump also
    wrongly ignored pg_collation.collencoding, CREATE COLLATION restrictions
    have been keeping that imperceptible in practical use.
    
    Use techniques like we use for object types already having full sort key
    coverage.  Where the pertinent queries weren't fetching the ignored sort
    keys, this adds columns to those queries and stores those keys in memory
    for the long term.
    
    The ignorance of sort keys became more problematic when commit
    172259afb563d35001410dc6daad78b250924038 added a schema diff test
    sensitive to it.  However, dump order stability isn't a new goal, and
    this might avoid other dump comparison failures.  Hence, back-patch to
    v13 (all supported versions).
    
    Reviewed-by: Robert Haas <robertmhaas@gmail.com>
    Discussion: https://postgr.es/m/20250707192654.9e.nmisch@google.com
    Backpatch-through: 13
    
    Conflicts:
    	src/bin/pg_dump/pg_dump.c
---
 src/bin/pg_dump/common.c                  |  19 +++
 src/bin/pg_dump/pg_dump.c                 |  62 ++++++--
 src/bin/pg_dump/pg_dump.h                 |   6 +
 src/bin/pg_dump/pg_dump_sort.c            | 238 +++++++++++++++++++++++++++---
 src/test/regress/expected/publication.out |  21 +++
 src/test/regress/sql/publication.sql      |  22 +++
 6 files changed, 335 insertions(+), 33 deletions(-)

diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index a64d37e..95f8980 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -17,6 +17,7 @@
 
 #include <ctype.h>
 
+#include "catalog/pg_am_d.h"
 #include "catalog/pg_class_d.h"
 #include "catalog/pg_collation_d.h"
 #include "catalog/pg_extension_d.h"
@@ -852,6 +853,24 @@ findOprByOid(Oid oid)
 }
 
 /*
+ * findAccessMethodByOid
+ *	  finds the DumpableObject for the access method with the given oid
+ *	  returns NULL if not found
+ */
+AccessMethodInfo *
+findAccessMethodByOid(Oid oid)
+{
+	CatalogId	catId;
+	DumpableObject *dobj;
+
+	catId.tableoid = AccessMethodRelationId;
+	catId.oid = oid;
+	dobj = findObjectByCatalogId(catId);
+	Assert(dobj == NULL || dobj->objType == DO_ACCESS_METHOD);
+	return (AccessMethodInfo *) dobj;
+}
+
+/*
  * findCollationByOid
  *	  finds the DumpableObject for the collation with the given oid
  *	  returns NULL if not found
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index e85f220..54f07b9 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1874,6 +1874,13 @@ selectDumpableProcLang(ProcLangInfo *plang, Archive *fout)
 static void
 selectDumpableAccessMethod(AccessMethodInfo *method, Archive *fout)
 {
+	/* see getAccessMethods() comment about v9.6. */
+	if (fout->remoteVersion < 90600)
+	{
+		method->dobj.dump = DUMP_COMPONENT_NONE;
+		return;
+	}
+
 	if (checkExtensionMembership(&method->dobj, fout))
 		return;					/* extension membership overrides all else */
 
@@ -5496,6 +5503,8 @@ getOperators(Archive *fout, int *numOprs)
 	int			i_oprnamespace;
 	int			i_oprowner;
 	int			i_oprkind;
+	int			i_oprleft;
+	int			i_oprright;
 	int			i_oprcode;
 
 	/*
@@ -5507,6 +5516,8 @@ getOperators(Archive *fout, int *numOprs)
 					  "oprnamespace, "
 					  "oprowner, "
 					  "oprkind, "
+					  "oprleft, "
+					  "oprright, "
 					  "oprcode::oid AS oprcode "
 					  "FROM pg_operator");
 
@@ -5523,6 +5534,8 @@ getOperators(Archive *fout, int *numOprs)
 	i_oprnamespace = PQfnumber(res, "oprnamespace");
 	i_oprowner = PQfnumber(res, "oprowner");
 	i_oprkind = PQfnumber(res, "oprkind");
+	i_oprleft = PQfnumber(res, "oprleft");
+	i_oprright = PQfnumber(res, "oprright");
 	i_oprcode = PQfnumber(res, "oprcode");
 
 	for (i = 0; i < ntups; i++)
@@ -5536,6 +5549,8 @@ getOperators(Archive *fout, int *numOprs)
 			findNamespace(atooid(PQgetvalue(res, i, i_oprnamespace)));
 		oprinfo[i].rolname = getRoleName(PQgetvalue(res, i, i_oprowner));
 		oprinfo[i].oprkind = (PQgetvalue(res, i, i_oprkind))[0];
+		oprinfo[i].oprleft = atooid(PQgetvalue(res, i, i_oprleft));
+		oprinfo[i].oprright = atooid(PQgetvalue(res, i, i_oprright));
 		oprinfo[i].oprcode = atooid(PQgetvalue(res, i, i_oprcode));
 
 		/* Decide whether we want to dump it */
@@ -5569,6 +5584,7 @@ getCollations(Archive *fout, int *numCollations)
 	int			i_collname;
 	int			i_collnamespace;
 	int			i_collowner;
+	int			i_collencoding;
 
 	query = createPQExpBuffer();
 
@@ -5579,7 +5595,8 @@ getCollations(Archive *fout, int *numCollations)
 
 	appendPQExpBuffer(query, "SELECT tableoid, oid, collname, "
 					  "collnamespace, "
-					  "collowner "
+					  "collowner, "
+					  "collencoding "
 					  "FROM pg_collation");
 
 	res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
@@ -5594,6 +5611,7 @@ getCollations(Archive *fout, int *numCollations)
 	i_collname = PQfnumber(res, "collname");
 	i_collnamespace = PQfnumber(res, "collnamespace");
 	i_collowner = PQfnumber(res, "collowner");
+	i_collencoding = PQfnumber(res, "collencoding");
 
 	for (i = 0; i < ntups; i++)
 	{
@@ -5605,6 +5623,7 @@ getCollations(Archive *fout, int *numCollations)
 		collinfo[i].dobj.namespace =
 			findNamespace(atooid(PQgetvalue(res, i, i_collnamespace)));
 		collinfo[i].rolname = getRoleName(PQgetvalue(res, i, i_collowner));
+		collinfo[i].collencoding = atoi(PQgetvalue(res, i, i_collencoding));
 
 		/* Decide whether we want to dump it */
 		selectDumpableObject(&(collinfo[i].dobj), fout);
@@ -5706,19 +5725,28 @@ getAccessMethods(Archive *fout, int *numAccessMethods)
 	int			i_amhandler;
 	int			i_amtype;
 
-	/* Before 9.6, there are no user-defined access methods */
-	if (fout->remoteVersion < 90600)
-	{
-		*numAccessMethods = 0;
-		return NULL;
-	}
-
 	query = createPQExpBuffer();
 
-	/* Select all access methods from pg_am table */
-	appendPQExpBufferStr(query, "SELECT tableoid, oid, amname, amtype, "
-						 "amhandler::pg_catalog.regproc AS amhandler "
-						 "FROM pg_am");
+	/*
+	 * Select all access methods from pg_am table.  v9.6 introduced CREATE
+	 * ACCESS METHOD, so earlier versions usually have only built-in access
+	 * methods.  v9.6 also changed the access method API, replacing dozens of
+	 * pg_am columns with amhandler.  Even if a user created an access method
+	 * by "INSERT INTO pg_am", we have no way to translate pre-v9.6 pg_am
+	 * columns to a v9.6+ CREATE ACCESS METHOD.  Hence, before v9.6, read
+	 * pg_am just to facilitate findAccessMethodByOid() providing the
+	 * OID-to-name mapping.
+	 */
+	appendPQExpBufferStr(query, "SELECT tableoid, oid, amname, ");
+	if (fout->remoteVersion >= 90600)
+		appendPQExpBufferStr(query,
+							 "amtype, "
+							 "amhandler::pg_catalog.regproc AS amhandler ");
+	else
+		appendPQExpBufferStr(query,
+							 "'i'::pg_catalog.\"char\" AS amtype, "
+							 "'-'::pg_catalog.regproc AS amhandler ");
+	appendPQExpBufferStr(query, "FROM pg_am");
 
 	res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
 
@@ -5773,6 +5801,7 @@ getOpclasses(Archive *fout, int *numOpclasses)
 	OpclassInfo *opcinfo;
 	int			i_tableoid;
 	int			i_oid;
+	int			i_opcmethod;
 	int			i_opcname;
 	int			i_opcnamespace;
 	int			i_opcowner;
@@ -5782,7 +5811,7 @@ getOpclasses(Archive *fout, int *numOpclasses)
 	 * system-defined opclasses at dump-out time.
 	 */
 
-	appendPQExpBuffer(query, "SELECT tableoid, oid, opcname, "
+	appendPQExpBuffer(query, "SELECT tableoid, oid, opcmethod, opcname, "
 					  "opcnamespace, "
 					  "opcowner "
 					  "FROM pg_opclass");
@@ -5796,6 +5825,7 @@ getOpclasses(Archive *fout, int *numOpclasses)
 
 	i_tableoid = PQfnumber(res, "tableoid");
 	i_oid = PQfnumber(res, "oid");
+	i_opcmethod = PQfnumber(res, "opcmethod");
 	i_opcname = PQfnumber(res, "opcname");
 	i_opcnamespace = PQfnumber(res, "opcnamespace");
 	i_opcowner = PQfnumber(res, "opcowner");
@@ -5809,6 +5839,7 @@ getOpclasses(Archive *fout, int *numOpclasses)
 		opcinfo[i].dobj.name = pg_strdup(PQgetvalue(res, i, i_opcname));
 		opcinfo[i].dobj.namespace =
 			findNamespace(atooid(PQgetvalue(res, i, i_opcnamespace)));
+		opcinfo[i].opcmethod = atooid(PQgetvalue(res, i, i_opcmethod));
 		opcinfo[i].rolname = getRoleName(PQgetvalue(res, i, i_opcowner));
 
 		/* Decide whether we want to dump it */
@@ -5839,6 +5870,7 @@ getOpfamilies(Archive *fout, int *numOpfamilies)
 	OpfamilyInfo *opfinfo;
 	int			i_tableoid;
 	int			i_oid;
+	int			i_opfmethod;
 	int			i_opfname;
 	int			i_opfnamespace;
 	int			i_opfowner;
@@ -5850,7 +5882,7 @@ getOpfamilies(Archive *fout, int *numOpfamilies)
 	 * system-defined opfamilies at dump-out time.
 	 */
 
-	appendPQExpBuffer(query, "SELECT tableoid, oid, opfname, "
+	appendPQExpBuffer(query, "SELECT tableoid, oid, opfmethod, opfname, "
 					  "opfnamespace, "
 					  "opfowner "
 					  "FROM pg_opfamily");
@@ -5865,6 +5897,7 @@ getOpfamilies(Archive *fout, int *numOpfamilies)
 	i_tableoid = PQfnumber(res, "tableoid");
 	i_oid = PQfnumber(res, "oid");
 	i_opfname = PQfnumber(res, "opfname");
+	i_opfmethod = PQfnumber(res, "opfmethod");
 	i_opfnamespace = PQfnumber(res, "opfnamespace");
 	i_opfowner = PQfnumber(res, "opfowner");
 
@@ -5877,6 +5910,7 @@ getOpfamilies(Archive *fout, int *numOpfamilies)
 		opfinfo[i].dobj.name = pg_strdup(PQgetvalue(res, i, i_opfname));
 		opfinfo[i].dobj.namespace =
 			findNamespace(atooid(PQgetvalue(res, i, i_opfnamespace)));
+		opfinfo[i].opfmethod = atooid(PQgetvalue(res, i, i_opfmethod));
 		opfinfo[i].rolname = getRoleName(PQgetvalue(res, i, i_opfowner));
 
 		/* Decide whether we want to dump it */
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 774ecb2..a64859a7 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -250,6 +250,8 @@ typedef struct _oprInfo
 	DumpableObject dobj;
 	const char *rolname;
 	char		oprkind;
+	Oid			oprleft;
+	Oid			oprright;
 	Oid			oprcode;
 } OprInfo;
 
@@ -263,12 +265,14 @@ typedef struct _accessMethodInfo
 typedef struct _opclassInfo
 {
 	DumpableObject dobj;
+	Oid			opcmethod;
 	const char *rolname;
 } OpclassInfo;
 
 typedef struct _opfamilyInfo
 {
 	DumpableObject dobj;
+	Oid			opfmethod;
 	const char *rolname;
 } OpfamilyInfo;
 
@@ -276,6 +280,7 @@ typedef struct _collInfo
 {
 	DumpableObject dobj;
 	const char *rolname;
+	int			collencoding;
 } CollInfo;
 
 typedef struct _convInfo
@@ -694,6 +699,7 @@ extern TableInfo *findTableByOid(Oid oid);
 extern TypeInfo *findTypeByOid(Oid oid);
 extern FuncInfo *findFuncByOid(Oid oid);
 extern OprInfo *findOprByOid(Oid oid);
+extern AccessMethodInfo *findAccessMethodByOid(Oid oid);
 extern CollInfo *findCollationByOid(Oid oid);
 extern NamespaceInfo *findNamespaceByOid(Oid oid);
 extern ExtensionInfo *findExtensionByOid(Oid oid);
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index f5152ec..38ad6aa 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -157,6 +157,8 @@ static DumpId postDataBoundId;
 
 
 static int	DOTypeNameCompare(const void *p1, const void *p2);
+static int	pgTypeNameCompare(Oid typid1, Oid typid2);
+static int	accessMethodNameCompare(Oid am1, Oid am2);
 static bool TopoSort(DumpableObject **objs,
 					 int numObjs,
 					 DumpableObject **ordering,
@@ -224,12 +226,39 @@ DOTypeNameCompare(const void *p1, const void *p2)
 	else if (obj2->namespace)
 		return 1;
 
-	/* Sort by name */
+	/*
+	 * Sort by name.  With a few exceptions, names here are single catalog
+	 * columns.  To get a fuller picture, grep pg_dump.c for "dobj.name = ".
+	 * Names here don't match "Name:" in plain format output, which is a
+	 * _tocEntry.tag.  For example, DumpableObject.name of a constraint is
+	 * pg_constraint.conname, but _tocEntry.tag of a constraint is relname and
+	 * conname joined with a space.
+	 */
 	cmpval = strcmp(obj1->name, obj2->name);
 	if (cmpval != 0)
 		return cmpval;
 
-	/* To have a stable sort order, break ties for some object types */
+	/*
+	 * Sort by type.  This helps types that share a type priority without
+	 * sharing a unique name constraint, e.g. opclass and opfamily.
+	 */
+	cmpval = obj1->objType - obj2->objType;
+	if (cmpval != 0)
+		return cmpval;
+
+	/*
+	 * To have a stable sort order, break ties for some object types.  Most
+	 * catalogs have a natural key, e.g. pg_proc_proname_args_nsp_index. Where
+	 * the above "namespace" and "name" comparisons don't cover all natural
+	 * key columns, compare the rest here.
+	 *
+	 * The natural key usually refers to other catalogs by surrogate keys.
+	 * Hence, this translates each of those references to the natural key of
+	 * the referenced catalog.  That may descend through multiple levels of
+	 * catalog references.  For example, to sort by pg_proc.proargtypes,
+	 * descend to each pg_type and then further to its pg_namespace, for an
+	 * overall sort by (nspname, typname).
+	 */
 	if (obj1->objType == DO_FUNC || obj1->objType == DO_AGG)
 	{
 		FuncInfo   *fobj1 = *(FuncInfo *const *) p1;
@@ -242,22 +271,10 @@ DOTypeNameCompare(const void *p1, const void *p2)
 			return cmpval;
 		for (i = 0; i < fobj1->nargs; i++)
 		{
-			TypeInfo   *argtype1 = findTypeByOid(fobj1->argtypes[i]);
-			TypeInfo   *argtype2 = findTypeByOid(fobj2->argtypes[i]);
-
-			if (argtype1 && argtype2)
-			{
-				if (argtype1->dobj.namespace && argtype2->dobj.namespace)
-				{
-					cmpval = strcmp(argtype1->dobj.namespace->dobj.name,
-									argtype2->dobj.namespace->dobj.name);
-					if (cmpval != 0)
-						return cmpval;
-				}
-				cmpval = strcmp(argtype1->dobj.name, argtype2->dobj.name);
-				if (cmpval != 0)
-					return cmpval;
-			}
+			cmpval = pgTypeNameCompare(fobj1->argtypes[i],
+									   fobj2->argtypes[i]);
+			if (cmpval != 0)
+				return cmpval;
 		}
 	}
 	else if (obj1->objType == DO_OPERATOR)
@@ -269,6 +286,57 @@ DOTypeNameCompare(const void *p1, const void *p2)
 		cmpval = (oobj2->oprkind - oobj1->oprkind);
 		if (cmpval != 0)
 			return cmpval;
+		/* Within an oprkind, sort by argument type names */
+		cmpval = pgTypeNameCompare(oobj1->oprleft, oobj2->oprleft);
+		if (cmpval != 0)
+			return cmpval;
+		cmpval = pgTypeNameCompare(oobj1->oprright, oobj2->oprright);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	else if (obj1->objType == DO_OPCLASS)
+	{
+		OpclassInfo *opcobj1 = *(OpclassInfo *const *) p1;
+		OpclassInfo *opcobj2 = *(OpclassInfo *const *) p2;
+
+		/* Sort by access method name, per pg_opclass_am_name_nsp_index */
+		cmpval = accessMethodNameCompare(opcobj1->opcmethod,
+										 opcobj2->opcmethod);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	else if (obj1->objType == DO_OPFAMILY)
+	{
+		OpfamilyInfo *opfobj1 = *(OpfamilyInfo *const *) p1;
+		OpfamilyInfo *opfobj2 = *(OpfamilyInfo *const *) p2;
+
+		/* Sort by access method name, per pg_opfamily_am_name_nsp_index */
+		cmpval = accessMethodNameCompare(opfobj1->opfmethod,
+										 opfobj2->opfmethod);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	else if (obj1->objType == DO_COLLATION)
+	{
+		CollInfo   *cobj1 = *(CollInfo *const *) p1;
+		CollInfo   *cobj2 = *(CollInfo *const *) p2;
+
+		/*
+		 * Sort by encoding, per pg_collation_name_enc_nsp_index. Technically,
+		 * this is not necessary, because wherever this changes dump order,
+		 * restoring the dump fails anyway.  CREATE COLLATION can't create a
+		 * tie for this to break, because it imposes restrictions to make
+		 * (nspname, collname) uniquely identify a collation within a given
+		 * DatabaseEncoding.  While pg_import_system_collations() can create a
+		 * tie, pg_dump+restore fails after
+		 * pg_import_system_collations('my_schema') does so. However, there's
+		 * little to gain by ignoring one natural key column on the basis of
+		 * those limitations elsewhere, so respect the full natural key like
+		 * we do for other object types.
+		 */
+		cmpval = cobj1->collencoding - cobj2->collencoding;
+		if (cmpval != 0)
+			return cmpval;
 	}
 	else if (obj1->objType == DO_ATTRDEF)
 	{
@@ -313,11 +381,143 @@ DOTypeNameCompare(const void *p1, const void *p2)
 		if (cmpval != 0)
 			return cmpval;
 	}
+	else if (obj1->objType == DO_CONSTRAINT)
+	{
+		ConstraintInfo *robj1 = *(ConstraintInfo *const *) p1;
+		ConstraintInfo *robj2 = *(ConstraintInfo *const *) p2;
 
-	/* Usually shouldn't get here, but if we do, sort by OID */
+		/*
+		 * Sort domain constraints before table constraints, for consistency
+		 * with our decision to sort CREATE DOMAIN before CREATE TABLE.
+		 */
+		if (robj1->condomain)
+		{
+			if (robj2->condomain)
+			{
+				/* Sort by domain name (domain namespace was considered) */
+				cmpval = strcmp(robj1->condomain->dobj.name,
+								robj2->condomain->dobj.name);
+				if (cmpval != 0)
+					return cmpval;
+			}
+			else
+				return PRIO_TYPE - PRIO_TABLE;
+		}
+		else if (robj2->condomain)
+			return PRIO_TABLE - PRIO_TYPE;
+		else
+		{
+			/* Sort by table name (table namespace was considered already) */
+			cmpval = strcmp(robj1->contable->dobj.name,
+							robj2->contable->dobj.name);
+			if (cmpval != 0)
+				return cmpval;
+		}
+	}
+	else if (obj1->objType == DO_PUBLICATION_REL)
+	{
+		PublicationRelInfo *probj1 = *(PublicationRelInfo *const *) p1;
+		PublicationRelInfo *probj2 = *(PublicationRelInfo *const *) p2;
+
+		/* Sort by publication name, since (namespace, name) match the rel */
+		cmpval = strcmp(probj1->publication->dobj.name,
+						probj2->publication->dobj.name);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	else if (obj1->objType == DO_PUBLICATION_TABLE_IN_SCHEMA)
+	{
+		PublicationSchemaInfo *psobj1 = *(PublicationSchemaInfo *const *) p1;
+		PublicationSchemaInfo *psobj2 = *(PublicationSchemaInfo *const *) p2;
+
+		/* Sort by publication name, since ->name is just nspname */
+		cmpval = strcmp(psobj1->publication->dobj.name,
+						psobj2->publication->dobj.name);
+		if (cmpval != 0)
+			return cmpval;
+	}
+
+	/*
+	 * Shouldn't get here except after catalog corruption, but if we do, sort
+	 * by OID.  This may make logically-identical databases differ in the
+	 * order of objects in dump output.  Users will get spurious schema diffs.
+	 * Expect flaky failures of 002_pg_upgrade.pl test 'dump outputs from
+	 * original and restored regression databases match' if the regression
+	 * database contains objects allowing that test to reach here.  That's a
+	 * consequence of the test using "pg_restore -j", which doesn't fully
+	 * constrain OID assignment order.
+	 */
+	Assert(false);
 	return oidcmp(obj1->catId.oid, obj2->catId.oid);
 }
 
+/* Compare two OID-identified pg_type values by nspname, then by typname. */
+static int
+pgTypeNameCompare(Oid typid1, Oid typid2)
+{
+	TypeInfo   *typobj1;
+	TypeInfo   *typobj2;
+	int			cmpval;
+
+	if (typid1 == typid2)
+		return 0;
+
+	typobj1 = findTypeByOid(typid1);
+	typobj2 = findTypeByOid(typid2);
+
+	if (!typobj1 || !typobj2)
+	{
+		/*
+		 * getTypes() didn't find some OID.  Assume catalog corruption, e.g.
+		 * an oprright value without the corresponding OID in a pg_type row.
+		 * Report as "equal", so the caller uses the next available basis for
+		 * comparison, e.g. the next function argument.
+		 *
+		 * Unary operators have InvalidOid in oprleft (if oprkind='r') or in
+		 * oprright (if oprkind='l').  Caller already sorted by oprkind,
+		 * calling us only for like-kind operators.  Hence, "typid1 == typid2"
+		 * took care of InvalidOid.  (v14 removed postfix operator support.
+		 * Hence, when dumping from v14+, only oprleft can be InvalidOid.)
+		 */
+		Assert(false);
+		return 0;
+	}
+
+	if (!typobj1->dobj.namespace || !typobj2->dobj.namespace)
+		Assert(false);			/* catalog corruption */
+	else
+	{
+		cmpval = strcmp(typobj1->dobj.namespace->dobj.name,
+						typobj2->dobj.namespace->dobj.name);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	return strcmp(typobj1->dobj.name, typobj2->dobj.name);
+}
+
+/* Compare two OID-identified pg_am values by amname. */
+static int
+accessMethodNameCompare(Oid am1, Oid am2)
+{
+	AccessMethodInfo *amobj1;
+	AccessMethodInfo *amobj2;
+
+	if (am1 == am2)
+		return 0;
+
+	amobj1 = findAccessMethodByOid(am1);
+	amobj2 = findAccessMethodByOid(am2);
+
+	if (!amobj1 || !amobj2)
+	{
+		/* catalog corruption: handle like pgTypeNameCompare() does */
+		Assert(false);
+		return 0;
+	}
+
+	return strcmp(amobj1->dobj.name, amobj2->dobj.name);
+}
+
 
 /*
  * Sort the given objects into a safe dump order using dependency
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 69dc6cf..e8d907c 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -1735,3 +1735,24 @@ DROP SCHEMA sch2 cascade;
 RESET SESSION AUTHORIZATION;
 DROP ROLE regress_publication_user, regress_publication_user2;
 DROP ROLE regress_publication_user_dummy;
+-- stage objects for pg_dump tests
+CREATE SCHEMA pubme CREATE TABLE t0 (c int, d int) CREATE TABLE t1 (c int);
+CREATE SCHEMA pubme2 CREATE TABLE t0 (c int, d int);
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION dump_pub_qual_1ct FOR
+  TABLE ONLY pubme.t0 (c, d) WHERE (c > 0);
+CREATE PUBLICATION dump_pub_qual_2ct FOR
+  TABLE ONLY pubme.t0 (c) WHERE (c > 0),
+  TABLE ONLY pubme.t1 (c);
+CREATE PUBLICATION dump_pub_nsp_1ct FOR
+  TABLES IN SCHEMA pubme;
+CREATE PUBLICATION dump_pub_nsp_2ct FOR
+  TABLES IN SCHEMA pubme,
+  TABLES IN SCHEMA pubme2;
+CREATE PUBLICATION dump_pub_all FOR
+  TABLE ONLY pubme.t0,
+  TABLE ONLY pubme.t1 WHERE (c < 0),
+  TABLES IN SCHEMA pubme,
+  TABLES IN SCHEMA pubme2
+  WITH (publish_via_partition_root = true);
+RESET client_min_messages;
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index d5051a5..46e1489 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -1100,3 +1100,25 @@ DROP SCHEMA sch2 cascade;
 RESET SESSION AUTHORIZATION;
 DROP ROLE regress_publication_user, regress_publication_user2;
 DROP ROLE regress_publication_user_dummy;
+
+-- stage objects for pg_dump tests
+CREATE SCHEMA pubme CREATE TABLE t0 (c int, d int) CREATE TABLE t1 (c int);
+CREATE SCHEMA pubme2 CREATE TABLE t0 (c int, d int);
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION dump_pub_qual_1ct FOR
+  TABLE ONLY pubme.t0 (c, d) WHERE (c > 0);
+CREATE PUBLICATION dump_pub_qual_2ct FOR
+  TABLE ONLY pubme.t0 (c) WHERE (c > 0),
+  TABLE ONLY pubme.t1 (c);
+CREATE PUBLICATION dump_pub_nsp_1ct FOR
+  TABLES IN SCHEMA pubme;
+CREATE PUBLICATION dump_pub_nsp_2ct FOR
+  TABLES IN SCHEMA pubme,
+  TABLES IN SCHEMA pubme2;
+CREATE PUBLICATION dump_pub_all FOR
+  TABLE ONLY pubme.t0,
+  TABLE ONLY pubme.t1 WHERE (c < 0),
+  TABLES IN SCHEMA pubme,
+  TABLES IN SCHEMA pubme2
+  WITH (publish_via_partition_root = true);
+RESET client_min_messages;
dobjcmp20-disambiguate-v2_14.patchtext/plain; charset=us-asciiDownload
commit 57c5fae (HEAD, zzy_test-commit-REL_14_STABLE)
Author:     Noah Misch <noah@leadboat.com>
AuthorDate: Thu Jul 24 17:26:41 2025 -0700
Commit:     Noah Misch <noah@leadboat.com>
CommitDate: Thu Jul 24 17:30:33 2025 -0700

    Sort dump objects independent of OIDs, for the 7 holdout object types.
    
    pg_dump sorts objects by their logical names, e.g. (nspname, relname,
    tgname), before dependency-driven reordering.  That removes one source
    of logically-identical databases differing in their schema-only dumps.
    In other words, it helps with schema diffing.  The logical name sort
    ignored essential sort keys for constraints, operators, PUBLICATION
    ... FOR TABLE, PUBLICATION ... FOR TABLES IN SCHEMA, operator classes,
    and operator families.  pg_dump's sort then depended on object OID,
    yielding spurious schema diffs.  After this change, OIDs affect dump
    order only in the event of catalog corruption.  While pg_dump also
    wrongly ignored pg_collation.collencoding, CREATE COLLATION restrictions
    have been keeping that imperceptible in practical use.
    
    Use techniques like we use for object types already having full sort key
    coverage.  Where the pertinent queries weren't fetching the ignored sort
    keys, this adds columns to those queries and stores those keys in memory
    for the long term.
    
    The ignorance of sort keys became more problematic when commit
    172259afb563d35001410dc6daad78b250924038 added a schema diff test
    sensitive to it.  However, dump order stability isn't a new goal, and
    this might avoid other dump comparison failures.  Hence, back-patch to
    v13 (all supported versions).
    
    Reviewed-by: Robert Haas <robertmhaas@gmail.com>
    Discussion: https://postgr.es/m/20250707192654.9e.nmisch@google.com
    Backpatch-through: 13
    
    Conflicts:
    	src/bin/pg_dump/pg_dump.c
    	src/bin/pg_dump/pg_dump.h
---
 src/bin/pg_dump/common.c                  |  19 +++
 src/bin/pg_dump/pg_dump.c                 |  62 ++++++--
 src/bin/pg_dump/pg_dump.h                 |   6 +
 src/bin/pg_dump/pg_dump_sort.c            | 227 +++++++++++++++++++++++++++---
 src/test/regress/expected/publication.out |   8 ++
 src/test/regress/sql/publication.sql      |   9 ++
 6 files changed, 298 insertions(+), 33 deletions(-)

diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index 3cabd82..7c59c91 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -17,6 +17,7 @@
 
 #include <ctype.h>
 
+#include "catalog/pg_am_d.h"
 #include "catalog/pg_class_d.h"
 #include "fe_utils/string_utils.h"
 #include "pg_backup_archiver.h"
@@ -931,6 +932,24 @@ findOprByOid(Oid oid)
 }
 
 /*
+ * findAccessMethodByOid
+ *	  finds the DumpableObject for the access method with the given oid
+ *	  returns NULL if not found
+ */
+AccessMethodInfo *
+findAccessMethodByOid(Oid oid)
+{
+	CatalogId	catId;
+	DumpableObject *dobj;
+
+	catId.tableoid = AccessMethodRelationId;
+	catId.oid = oid;
+	dobj = findObjectByCatalogId(catId);
+	Assert(dobj == NULL || dobj->objType == DO_ACCESS_METHOD);
+	return (AccessMethodInfo *) dobj;
+}
+
+/*
  * findCollationByOid
  *	  finds the entry (in collinfo) of the collation with the given oid
  *	  returns NULL if not found
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index f5a6578..a255cc6 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1899,6 +1899,13 @@ selectDumpableProcLang(ProcLangInfo *plang, Archive *fout)
 static void
 selectDumpableAccessMethod(AccessMethodInfo *method, Archive *fout)
 {
+	/* see getAccessMethods() comment about v9.6. */
+	if (fout->remoteVersion < 90600)
+	{
+		method->dobj.dump = DUMP_COMPONENT_NONE;
+		return;
+	}
+
 	if (checkExtensionMembership(&method->dobj, fout))
 		return;					/* extension membership overrides all else */
 
@@ -5525,6 +5532,8 @@ getOperators(Archive *fout, int *numOprs)
 	int			i_oprnamespace;
 	int			i_rolname;
 	int			i_oprkind;
+	int			i_oprleft;
+	int			i_oprright;
 	int			i_oprcode;
 
 	/*
@@ -5536,6 +5545,8 @@ getOperators(Archive *fout, int *numOprs)
 					  "oprnamespace, "
 					  "(%s oprowner) AS rolname, "
 					  "oprkind, "
+					  "oprleft, "
+					  "oprright, "
 					  "oprcode::oid AS oprcode "
 					  "FROM pg_operator",
 					  username_subquery);
@@ -5553,6 +5564,8 @@ getOperators(Archive *fout, int *numOprs)
 	i_oprnamespace = PQfnumber(res, "oprnamespace");
 	i_rolname = PQfnumber(res, "rolname");
 	i_oprkind = PQfnumber(res, "oprkind");
+	i_oprleft = PQfnumber(res, "oprleft");
+	i_oprright = PQfnumber(res, "oprright");
 	i_oprcode = PQfnumber(res, "oprcode");
 
 	for (i = 0; i < ntups; i++)
@@ -5566,6 +5579,8 @@ getOperators(Archive *fout, int *numOprs)
 			findNamespace(atooid(PQgetvalue(res, i, i_oprnamespace)));
 		oprinfo[i].rolname = pg_strdup(PQgetvalue(res, i, i_rolname));
 		oprinfo[i].oprkind = (PQgetvalue(res, i, i_oprkind))[0];
+		oprinfo[i].oprleft = atooid(PQgetvalue(res, i, i_oprleft));
+		oprinfo[i].oprright = atooid(PQgetvalue(res, i, i_oprright));
 		oprinfo[i].oprcode = atooid(PQgetvalue(res, i, i_oprcode));
 
 		/* Decide whether we want to dump it */
@@ -5606,6 +5621,7 @@ getCollations(Archive *fout, int *numCollations)
 	int			i_collname;
 	int			i_collnamespace;
 	int			i_rolname;
+	int			i_collencoding;
 
 	/* Collations didn't exist pre-9.1 */
 	if (fout->remoteVersion < 90100)
@@ -5623,7 +5639,8 @@ getCollations(Archive *fout, int *numCollations)
 
 	appendPQExpBuffer(query, "SELECT tableoid, oid, collname, "
 					  "collnamespace, "
-					  "(%s collowner) AS rolname "
+					  "(%s collowner) AS rolname, "
+					  "collencoding "
 					  "FROM pg_collation",
 					  username_subquery);
 
@@ -5639,6 +5656,7 @@ getCollations(Archive *fout, int *numCollations)
 	i_collname = PQfnumber(res, "collname");
 	i_collnamespace = PQfnumber(res, "collnamespace");
 	i_rolname = PQfnumber(res, "rolname");
+	i_collencoding = PQfnumber(res, "collencoding");
 
 	for (i = 0; i < ntups; i++)
 	{
@@ -5650,6 +5668,7 @@ getCollations(Archive *fout, int *numCollations)
 		collinfo[i].dobj.namespace =
 			findNamespace(atooid(PQgetvalue(res, i, i_collnamespace)));
 		collinfo[i].rolname = pg_strdup(PQgetvalue(res, i, i_rolname));
+		collinfo[i].collencoding = atoi(PQgetvalue(res, i, i_collencoding));
 
 		/* Decide whether we want to dump it */
 		selectDumpableObject(&(collinfo[i].dobj), fout);
@@ -5758,19 +5777,28 @@ getAccessMethods(Archive *fout, int *numAccessMethods)
 	int			i_amhandler;
 	int			i_amtype;
 
-	/* Before 9.6, there are no user-defined access methods */
-	if (fout->remoteVersion < 90600)
-	{
-		*numAccessMethods = 0;
-		return NULL;
-	}
-
 	query = createPQExpBuffer();
 
-	/* Select all access methods from pg_am table */
-	appendPQExpBufferStr(query, "SELECT tableoid, oid, amname, amtype, "
-						 "amhandler::pg_catalog.regproc AS amhandler "
-						 "FROM pg_am");
+	/*
+	 * Select all access methods from pg_am table.  v9.6 introduced CREATE
+	 * ACCESS METHOD, so earlier versions usually have only built-in access
+	 * methods.  v9.6 also changed the access method API, replacing dozens of
+	 * pg_am columns with amhandler.  Even if a user created an access method
+	 * by "INSERT INTO pg_am", we have no way to translate pre-v9.6 pg_am
+	 * columns to a v9.6+ CREATE ACCESS METHOD.  Hence, before v9.6, read
+	 * pg_am just to facilitate findAccessMethodByOid() providing the
+	 * OID-to-name mapping.
+	 */
+	appendPQExpBufferStr(query, "SELECT tableoid, oid, amname, ");
+	if (fout->remoteVersion >= 90600)
+		appendPQExpBufferStr(query,
+							 "amtype, "
+							 "amhandler::pg_catalog.regproc AS amhandler ");
+	else
+		appendPQExpBufferStr(query,
+							 "'i'::pg_catalog.\"char\" AS amtype, "
+							 "'-'::pg_catalog.regproc AS amhandler ");
+	appendPQExpBufferStr(query, "FROM pg_am");
 
 	res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
 
@@ -5828,6 +5856,7 @@ getOpclasses(Archive *fout, int *numOpclasses)
 	OpclassInfo *opcinfo;
 	int			i_tableoid;
 	int			i_oid;
+	int			i_opcmethod;
 	int			i_opcname;
 	int			i_opcnamespace;
 	int			i_rolname;
@@ -5837,7 +5866,7 @@ getOpclasses(Archive *fout, int *numOpclasses)
 	 * system-defined opclasses at dump-out time.
 	 */
 
-	appendPQExpBuffer(query, "SELECT tableoid, oid, opcname, "
+	appendPQExpBuffer(query, "SELECT tableoid, oid, opcmethod, opcname, "
 					  "opcnamespace, "
 					  "(%s opcowner) AS rolname "
 					  "FROM pg_opclass",
@@ -5852,6 +5881,7 @@ getOpclasses(Archive *fout, int *numOpclasses)
 
 	i_tableoid = PQfnumber(res, "tableoid");
 	i_oid = PQfnumber(res, "oid");
+	i_opcmethod = PQfnumber(res, "opcmethod");
 	i_opcname = PQfnumber(res, "opcname");
 	i_opcnamespace = PQfnumber(res, "opcnamespace");
 	i_rolname = PQfnumber(res, "rolname");
@@ -5865,6 +5895,7 @@ getOpclasses(Archive *fout, int *numOpclasses)
 		opcinfo[i].dobj.name = pg_strdup(PQgetvalue(res, i, i_opcname));
 		opcinfo[i].dobj.namespace =
 			findNamespace(atooid(PQgetvalue(res, i, i_opcnamespace)));
+		opcinfo[i].opcmethod = atooid(PQgetvalue(res, i, i_opcmethod));
 		opcinfo[i].rolname = pg_strdup(PQgetvalue(res, i, i_rolname));
 
 		/* Decide whether we want to dump it */
@@ -5902,6 +5933,7 @@ getOpfamilies(Archive *fout, int *numOpfamilies)
 	OpfamilyInfo *opfinfo;
 	int			i_tableoid;
 	int			i_oid;
+	int			i_opfmethod;
 	int			i_opfname;
 	int			i_opfnamespace;
 	int			i_rolname;
@@ -5920,7 +5952,7 @@ getOpfamilies(Archive *fout, int *numOpfamilies)
 	 * system-defined opfamilies at dump-out time.
 	 */
 
-	appendPQExpBuffer(query, "SELECT tableoid, oid, opfname, "
+	appendPQExpBuffer(query, "SELECT tableoid, oid, opfmethod, opfname, "
 					  "opfnamespace, "
 					  "(%s opfowner) AS rolname "
 					  "FROM pg_opfamily",
@@ -5936,6 +5968,7 @@ getOpfamilies(Archive *fout, int *numOpfamilies)
 	i_tableoid = PQfnumber(res, "tableoid");
 	i_oid = PQfnumber(res, "oid");
 	i_opfname = PQfnumber(res, "opfname");
+	i_opfmethod = PQfnumber(res, "opfmethod");
 	i_opfnamespace = PQfnumber(res, "opfnamespace");
 	i_rolname = PQfnumber(res, "rolname");
 
@@ -5948,6 +5981,7 @@ getOpfamilies(Archive *fout, int *numOpfamilies)
 		opfinfo[i].dobj.name = pg_strdup(PQgetvalue(res, i, i_opfname));
 		opfinfo[i].dobj.namespace =
 			findNamespace(atooid(PQgetvalue(res, i, i_opfnamespace)));
+		opfinfo[i].opfmethod = atooid(PQgetvalue(res, i, i_opfmethod));
 		opfinfo[i].rolname = pg_strdup(PQgetvalue(res, i, i_rolname));
 
 		/* Decide whether we want to dump it */
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 832aa86..bcd7841 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -222,6 +222,8 @@ typedef struct _oprInfo
 	DumpableObject dobj;
 	char	   *rolname;
 	char		oprkind;
+	Oid			oprleft;
+	Oid			oprright;
 	Oid			oprcode;
 } OprInfo;
 
@@ -235,12 +237,14 @@ typedef struct _accessMethodInfo
 typedef struct _opclassInfo
 {
 	DumpableObject dobj;
+	Oid			opcmethod;
 	char	   *rolname;
 } OpclassInfo;
 
 typedef struct _opfamilyInfo
 {
 	DumpableObject dobj;
+	Oid			opfmethod;
 	char	   *rolname;
 } OpfamilyInfo;
 
@@ -248,6 +252,7 @@ typedef struct _collInfo
 {
 	DumpableObject dobj;
 	char	   *rolname;
+	int			collencoding;
 } CollInfo;
 
 typedef struct _convInfo
@@ -675,6 +680,7 @@ extern TableInfo *findTableByOid(Oid oid);
 extern TypeInfo *findTypeByOid(Oid oid);
 extern FuncInfo *findFuncByOid(Oid oid);
 extern OprInfo *findOprByOid(Oid oid);
+extern AccessMethodInfo *findAccessMethodByOid(Oid oid);
 extern CollInfo *findCollationByOid(Oid oid);
 extern NamespaceInfo *findNamespaceByOid(Oid oid);
 extern ExtensionInfo *findExtensionByOid(Oid oid);
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index 039b3be..b6d7337 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -155,6 +155,8 @@ static DumpId postDataBoundId;
 
 
 static int	DOTypeNameCompare(const void *p1, const void *p2);
+static int	pgTypeNameCompare(Oid typid1, Oid typid2);
+static int	accessMethodNameCompare(Oid am1, Oid am2);
 static bool TopoSort(DumpableObject **objs,
 					 int numObjs,
 					 DumpableObject **ordering,
@@ -222,12 +224,39 @@ DOTypeNameCompare(const void *p1, const void *p2)
 	else if (obj2->namespace)
 		return 1;
 
-	/* Sort by name */
+	/*
+	 * Sort by name.  With a few exceptions, names here are single catalog
+	 * columns.  To get a fuller picture, grep pg_dump.c for "dobj.name = ".
+	 * Names here don't match "Name:" in plain format output, which is a
+	 * _tocEntry.tag.  For example, DumpableObject.name of a constraint is
+	 * pg_constraint.conname, but _tocEntry.tag of a constraint is relname and
+	 * conname joined with a space.
+	 */
 	cmpval = strcmp(obj1->name, obj2->name);
 	if (cmpval != 0)
 		return cmpval;
 
-	/* To have a stable sort order, break ties for some object types */
+	/*
+	 * Sort by type.  This helps types that share a type priority without
+	 * sharing a unique name constraint, e.g. opclass and opfamily.
+	 */
+	cmpval = obj1->objType - obj2->objType;
+	if (cmpval != 0)
+		return cmpval;
+
+	/*
+	 * To have a stable sort order, break ties for some object types.  Most
+	 * catalogs have a natural key, e.g. pg_proc_proname_args_nsp_index. Where
+	 * the above "namespace" and "name" comparisons don't cover all natural
+	 * key columns, compare the rest here.
+	 *
+	 * The natural key usually refers to other catalogs by surrogate keys.
+	 * Hence, this translates each of those references to the natural key of
+	 * the referenced catalog.  That may descend through multiple levels of
+	 * catalog references.  For example, to sort by pg_proc.proargtypes,
+	 * descend to each pg_type and then further to its pg_namespace, for an
+	 * overall sort by (nspname, typname).
+	 */
 	if (obj1->objType == DO_FUNC || obj1->objType == DO_AGG)
 	{
 		FuncInfo   *fobj1 = *(FuncInfo *const *) p1;
@@ -240,22 +269,10 @@ DOTypeNameCompare(const void *p1, const void *p2)
 			return cmpval;
 		for (i = 0; i < fobj1->nargs; i++)
 		{
-			TypeInfo   *argtype1 = findTypeByOid(fobj1->argtypes[i]);
-			TypeInfo   *argtype2 = findTypeByOid(fobj2->argtypes[i]);
-
-			if (argtype1 && argtype2)
-			{
-				if (argtype1->dobj.namespace && argtype2->dobj.namespace)
-				{
-					cmpval = strcmp(argtype1->dobj.namespace->dobj.name,
-									argtype2->dobj.namespace->dobj.name);
-					if (cmpval != 0)
-						return cmpval;
-				}
-				cmpval = strcmp(argtype1->dobj.name, argtype2->dobj.name);
-				if (cmpval != 0)
-					return cmpval;
-			}
+			cmpval = pgTypeNameCompare(fobj1->argtypes[i],
+									   fobj2->argtypes[i]);
+			if (cmpval != 0)
+				return cmpval;
 		}
 	}
 	else if (obj1->objType == DO_OPERATOR)
@@ -267,6 +284,57 @@ DOTypeNameCompare(const void *p1, const void *p2)
 		cmpval = (oobj2->oprkind - oobj1->oprkind);
 		if (cmpval != 0)
 			return cmpval;
+		/* Within an oprkind, sort by argument type names */
+		cmpval = pgTypeNameCompare(oobj1->oprleft, oobj2->oprleft);
+		if (cmpval != 0)
+			return cmpval;
+		cmpval = pgTypeNameCompare(oobj1->oprright, oobj2->oprright);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	else if (obj1->objType == DO_OPCLASS)
+	{
+		OpclassInfo *opcobj1 = *(OpclassInfo *const *) p1;
+		OpclassInfo *opcobj2 = *(OpclassInfo *const *) p2;
+
+		/* Sort by access method name, per pg_opclass_am_name_nsp_index */
+		cmpval = accessMethodNameCompare(opcobj1->opcmethod,
+										 opcobj2->opcmethod);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	else if (obj1->objType == DO_OPFAMILY)
+	{
+		OpfamilyInfo *opfobj1 = *(OpfamilyInfo *const *) p1;
+		OpfamilyInfo *opfobj2 = *(OpfamilyInfo *const *) p2;
+
+		/* Sort by access method name, per pg_opfamily_am_name_nsp_index */
+		cmpval = accessMethodNameCompare(opfobj1->opfmethod,
+										 opfobj2->opfmethod);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	else if (obj1->objType == DO_COLLATION)
+	{
+		CollInfo   *cobj1 = *(CollInfo *const *) p1;
+		CollInfo   *cobj2 = *(CollInfo *const *) p2;
+
+		/*
+		 * Sort by encoding, per pg_collation_name_enc_nsp_index. Technically,
+		 * this is not necessary, because wherever this changes dump order,
+		 * restoring the dump fails anyway.  CREATE COLLATION can't create a
+		 * tie for this to break, because it imposes restrictions to make
+		 * (nspname, collname) uniquely identify a collation within a given
+		 * DatabaseEncoding.  While pg_import_system_collations() can create a
+		 * tie, pg_dump+restore fails after
+		 * pg_import_system_collations('my_schema') does so. However, there's
+		 * little to gain by ignoring one natural key column on the basis of
+		 * those limitations elsewhere, so respect the full natural key like
+		 * we do for other object types.
+		 */
+		cmpval = cobj1->collencoding - cobj2->collencoding;
+		if (cmpval != 0)
+			return cmpval;
 	}
 	else if (obj1->objType == DO_ATTRDEF)
 	{
@@ -311,11 +379,132 @@ DOTypeNameCompare(const void *p1, const void *p2)
 		if (cmpval != 0)
 			return cmpval;
 	}
+	else if (obj1->objType == DO_CONSTRAINT)
+	{
+		ConstraintInfo *robj1 = *(ConstraintInfo *const *) p1;
+		ConstraintInfo *robj2 = *(ConstraintInfo *const *) p2;
+
+		/*
+		 * Sort domain constraints before table constraints, for consistency
+		 * with our decision to sort CREATE DOMAIN before CREATE TABLE.
+		 */
+		if (robj1->condomain)
+		{
+			if (robj2->condomain)
+			{
+				/* Sort by domain name (domain namespace was considered) */
+				cmpval = strcmp(robj1->condomain->dobj.name,
+								robj2->condomain->dobj.name);
+				if (cmpval != 0)
+					return cmpval;
+			}
+			else
+				return PRIO_TYPE - PRIO_TABLE;
+		}
+		else if (robj2->condomain)
+			return PRIO_TABLE - PRIO_TYPE;
+		else
+		{
+			/* Sort by table name (table namespace was considered already) */
+			cmpval = strcmp(robj1->contable->dobj.name,
+							robj2->contable->dobj.name);
+			if (cmpval != 0)
+				return cmpval;
+		}
+	}
+	else if (obj1->objType == DO_PUBLICATION_REL)
+	{
+		PublicationRelInfo *probj1 = *(PublicationRelInfo *const *) p1;
+		PublicationRelInfo *probj2 = *(PublicationRelInfo *const *) p2;
+
+		/* Sort by publication name, since (namespace, name) match the rel */
+		cmpval = strcmp(probj1->publication->dobj.name,
+						probj2->publication->dobj.name);
+		if (cmpval != 0)
+			return cmpval;
+	}
 
-	/* Usually shouldn't get here, but if we do, sort by OID */
+	/*
+	 * Shouldn't get here except after catalog corruption, but if we do, sort
+	 * by OID.  This may make logically-identical databases differ in the
+	 * order of objects in dump output.  Users will get spurious schema diffs.
+	 * Expect flaky failures of 002_pg_upgrade.pl test 'dump outputs from
+	 * original and restored regression databases match' if the regression
+	 * database contains objects allowing that test to reach here.  That's a
+	 * consequence of the test using "pg_restore -j", which doesn't fully
+	 * constrain OID assignment order.
+	 */
+	Assert(false);
 	return oidcmp(obj1->catId.oid, obj2->catId.oid);
 }
 
+/* Compare two OID-identified pg_type values by nspname, then by typname. */
+static int
+pgTypeNameCompare(Oid typid1, Oid typid2)
+{
+	TypeInfo   *typobj1;
+	TypeInfo   *typobj2;
+	int			cmpval;
+
+	if (typid1 == typid2)
+		return 0;
+
+	typobj1 = findTypeByOid(typid1);
+	typobj2 = findTypeByOid(typid2);
+
+	if (!typobj1 || !typobj2)
+	{
+		/*
+		 * getTypes() didn't find some OID.  Assume catalog corruption, e.g.
+		 * an oprright value without the corresponding OID in a pg_type row.
+		 * Report as "equal", so the caller uses the next available basis for
+		 * comparison, e.g. the next function argument.
+		 *
+		 * Unary operators have InvalidOid in oprleft (if oprkind='r') or in
+		 * oprright (if oprkind='l').  Caller already sorted by oprkind,
+		 * calling us only for like-kind operators.  Hence, "typid1 == typid2"
+		 * took care of InvalidOid.  (v14 removed postfix operator support.
+		 * Hence, when dumping from v14+, only oprleft can be InvalidOid.)
+		 */
+		Assert(false);
+		return 0;
+	}
+
+	if (!typobj1->dobj.namespace || !typobj2->dobj.namespace)
+		Assert(false);			/* catalog corruption */
+	else
+	{
+		cmpval = strcmp(typobj1->dobj.namespace->dobj.name,
+						typobj2->dobj.namespace->dobj.name);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	return strcmp(typobj1->dobj.name, typobj2->dobj.name);
+}
+
+/* Compare two OID-identified pg_am values by amname. */
+static int
+accessMethodNameCompare(Oid am1, Oid am2)
+{
+	AccessMethodInfo *amobj1;
+	AccessMethodInfo *amobj2;
+
+	if (am1 == am2)
+		return 0;
+
+	amobj1 = findAccessMethodByOid(am1);
+	amobj2 = findAccessMethodByOid(am2);
+
+	if (!amobj1 || !amobj2)
+	{
+		/* catalog corruption: handle like pgTypeNameCompare() does */
+		Assert(false);
+		return 0;
+	}
+
+	return strcmp(amobj1->dobj.name, amobj2->dobj.name);
+}
+
 
 /*
  * Sort the given objects into a safe dump order using dependency
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index b7ce080..35595fb 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -344,3 +344,11 @@ NOTICE:  drop cascades to table pub_test.testpub_nopk
 RESET SESSION AUTHORIZATION;
 DROP ROLE regress_publication_user, regress_publication_user2;
 DROP ROLE regress_publication_user_dummy;
+-- stage objects for pg_dump tests
+CREATE SCHEMA pubme CREATE TABLE t0 (c int, d int) CREATE TABLE t1 (c int);
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION dump_pub_1ct FOR TABLE ONLY pubme.t0;
+CREATE PUBLICATION dump_pub_2ct FOR TABLE ONLY pubme.t0, pubme.t1;
+CREATE PUBLICATION dump_pub_all FOR TABLE ONLY pubme.t0, pubme.t1
+  WITH (publish_via_partition_root = true);
+RESET client_min_messages;
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index 7d5c937..021d4e2 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -218,3 +218,12 @@ DROP SCHEMA pub_test CASCADE;
 RESET SESSION AUTHORIZATION;
 DROP ROLE regress_publication_user, regress_publication_user2;
 DROP ROLE regress_publication_user_dummy;
+
+-- stage objects for pg_dump tests
+CREATE SCHEMA pubme CREATE TABLE t0 (c int, d int) CREATE TABLE t1 (c int);
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION dump_pub_1ct FOR TABLE ONLY pubme.t0;
+CREATE PUBLICATION dump_pub_2ct FOR TABLE ONLY pubme.t0, pubme.t1;
+CREATE PUBLICATION dump_pub_all FOR TABLE ONLY pubme.t0, pubme.t1
+  WITH (publish_via_partition_root = true);
+RESET client_min_messages;
dobjcmp20-disambiguate-v2_13.patchtext/plain; charset=us-asciiDownload
commit 5aadf5d (HEAD, zzy_test-commit-REL_13_STABLE)
Author:     Noah Misch <noah@leadboat.com>
AuthorDate: Thu Jul 24 17:32:07 2025 -0700
Commit:     Noah Misch <noah@leadboat.com>
CommitDate: Thu Jul 24 17:41:00 2025 -0700

    Sort dump objects independent of OIDs, for the 7 holdout object types.
    
    pg_dump sorts objects by their logical names, e.g. (nspname, relname,
    tgname), before dependency-driven reordering.  That removes one source
    of logically-identical databases differing in their schema-only dumps.
    In other words, it helps with schema diffing.  The logical name sort
    ignored essential sort keys for constraints, operators, PUBLICATION
    ... FOR TABLE, PUBLICATION ... FOR TABLES IN SCHEMA, operator classes,
    and operator families.  pg_dump's sort then depended on object OID,
    yielding spurious schema diffs.  After this change, OIDs affect dump
    order only in the event of catalog corruption.  While pg_dump also
    wrongly ignored pg_collation.collencoding, CREATE COLLATION restrictions
    have been keeping that imperceptible in practical use.
    
    Use techniques like we use for object types already having full sort key
    coverage.  Where the pertinent queries weren't fetching the ignored sort
    keys, this adds columns to those queries and stores those keys in memory
    for the long term.
    
    The ignorance of sort keys became more problematic when commit
    172259afb563d35001410dc6daad78b250924038 added a schema diff test
    sensitive to it.  However, dump order stability isn't a new goal, and
    this might avoid other dump comparison failures.  Hence, back-patch to
    v13 (all supported versions).
    
    Reviewed-by: Robert Haas <robertmhaas@gmail.com>
    Discussion: https://postgr.es/m/20250707192654.9e.nmisch@google.com
    Backpatch-through: 13
    
    Conflicts:
    	src/bin/pg_dump/pg_dump.c
---
 src/bin/pg_dump/common.c                  |  19 +++
 src/bin/pg_dump/pg_dump.c                 |  62 ++++++--
 src/bin/pg_dump/pg_dump.h                 |   6 +
 src/bin/pg_dump/pg_dump_sort.c            | 227 +++++++++++++++++++++++++++---
 src/test/regress/expected/publication.out |   8 ++
 src/test/regress/sql/publication.sql      |   9 ++
 6 files changed, 298 insertions(+), 33 deletions(-)

diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index c68b86b..e4a5812 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -17,6 +17,7 @@
 
 #include <ctype.h>
 
+#include "catalog/pg_am_d.h"
 #include "catalog/pg_class_d.h"
 #include "fe_utils/string_utils.h"
 #include "pg_backup_archiver.h"
@@ -891,6 +892,24 @@ findOprByOid(Oid oid)
 }
 
 /*
+ * findAccessMethodByOid
+ *	  finds the DumpableObject for the access method with the given oid
+ *	  returns NULL if not found
+ */
+AccessMethodInfo *
+findAccessMethodByOid(Oid oid)
+{
+	CatalogId	catId;
+	DumpableObject *dobj;
+
+	catId.tableoid = AccessMethodRelationId;
+	catId.oid = oid;
+	dobj = findObjectByCatalogId(catId);
+	Assert(dobj == NULL || dobj->objType == DO_ACCESS_METHOD);
+	return (AccessMethodInfo *) dobj;
+}
+
+/*
  * findCollationByOid
  *	  finds the entry (in collinfo) of the collation with the given oid
  *	  returns NULL if not found
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 4b13669..6745520 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1767,6 +1767,13 @@ selectDumpableProcLang(ProcLangInfo *plang, Archive *fout)
 static void
 selectDumpableAccessMethod(AccessMethodInfo *method, Archive *fout)
 {
+	/* see getAccessMethods() comment about v9.6. */
+	if (fout->remoteVersion < 90600)
+	{
+		method->dobj.dump = DUMP_COMPONENT_NONE;
+		return;
+	}
+
 	if (checkExtensionMembership(&method->dobj, fout))
 		return;					/* extension membership overrides all else */
 
@@ -5325,6 +5332,8 @@ getOperators(Archive *fout, int *numOprs)
 	int			i_oprnamespace;
 	int			i_rolname;
 	int			i_oprkind;
+	int			i_oprleft;
+	int			i_oprright;
 	int			i_oprcode;
 
 	/*
@@ -5336,6 +5345,8 @@ getOperators(Archive *fout, int *numOprs)
 					  "oprnamespace, "
 					  "(%s oprowner) AS rolname, "
 					  "oprkind, "
+					  "oprleft, "
+					  "oprright, "
 					  "oprcode::oid AS oprcode "
 					  "FROM pg_operator",
 					  username_subquery);
@@ -5353,6 +5364,8 @@ getOperators(Archive *fout, int *numOprs)
 	i_oprnamespace = PQfnumber(res, "oprnamespace");
 	i_rolname = PQfnumber(res, "rolname");
 	i_oprkind = PQfnumber(res, "oprkind");
+	i_oprleft = PQfnumber(res, "oprleft");
+	i_oprright = PQfnumber(res, "oprright");
 	i_oprcode = PQfnumber(res, "oprcode");
 
 	for (i = 0; i < ntups; i++)
@@ -5367,6 +5380,8 @@ getOperators(Archive *fout, int *numOprs)
 						  atooid(PQgetvalue(res, i, i_oprnamespace)));
 		oprinfo[i].rolname = pg_strdup(PQgetvalue(res, i, i_rolname));
 		oprinfo[i].oprkind = (PQgetvalue(res, i, i_oprkind))[0];
+		oprinfo[i].oprleft = atooid(PQgetvalue(res, i, i_oprleft));
+		oprinfo[i].oprright = atooid(PQgetvalue(res, i, i_oprright));
 		oprinfo[i].oprcode = atooid(PQgetvalue(res, i, i_oprcode));
 
 		/* Decide whether we want to dump it */
@@ -5407,6 +5422,7 @@ getCollations(Archive *fout, int *numCollations)
 	int			i_collname;
 	int			i_collnamespace;
 	int			i_rolname;
+	int			i_collencoding;
 
 	/* Collations didn't exist pre-9.1 */
 	if (fout->remoteVersion < 90100)
@@ -5424,7 +5440,8 @@ getCollations(Archive *fout, int *numCollations)
 
 	appendPQExpBuffer(query, "SELECT tableoid, oid, collname, "
 					  "collnamespace, "
-					  "(%s collowner) AS rolname "
+					  "(%s collowner) AS rolname, "
+					  "collencoding "
 					  "FROM pg_collation",
 					  username_subquery);
 
@@ -5440,6 +5457,7 @@ getCollations(Archive *fout, int *numCollations)
 	i_collname = PQfnumber(res, "collname");
 	i_collnamespace = PQfnumber(res, "collnamespace");
 	i_rolname = PQfnumber(res, "rolname");
+	i_collencoding = PQfnumber(res, "collencoding");
 
 	for (i = 0; i < ntups; i++)
 	{
@@ -5452,6 +5470,7 @@ getCollations(Archive *fout, int *numCollations)
 			findNamespace(fout,
 						  atooid(PQgetvalue(res, i, i_collnamespace)));
 		collinfo[i].rolname = pg_strdup(PQgetvalue(res, i, i_rolname));
+		collinfo[i].collencoding = atoi(PQgetvalue(res, i, i_collencoding));
 
 		/* Decide whether we want to dump it */
 		selectDumpableObject(&(collinfo[i].dobj), fout);
@@ -5561,19 +5580,28 @@ getAccessMethods(Archive *fout, int *numAccessMethods)
 	int			i_amhandler;
 	int			i_amtype;
 
-	/* Before 9.6, there are no user-defined access methods */
-	if (fout->remoteVersion < 90600)
-	{
-		*numAccessMethods = 0;
-		return NULL;
-	}
-
 	query = createPQExpBuffer();
 
-	/* Select all access methods from pg_am table */
-	appendPQExpBufferStr(query, "SELECT tableoid, oid, amname, amtype, "
-						 "amhandler::pg_catalog.regproc AS amhandler "
-						 "FROM pg_am");
+	/*
+	 * Select all access methods from pg_am table.  v9.6 introduced CREATE
+	 * ACCESS METHOD, so earlier versions usually have only built-in access
+	 * methods.  v9.6 also changed the access method API, replacing dozens of
+	 * pg_am columns with amhandler.  Even if a user created an access method
+	 * by "INSERT INTO pg_am", we have no way to translate pre-v9.6 pg_am
+	 * columns to a v9.6+ CREATE ACCESS METHOD.  Hence, before v9.6, read
+	 * pg_am just to facilitate findAccessMethodByOid() providing the
+	 * OID-to-name mapping.
+	 */
+	appendPQExpBufferStr(query, "SELECT tableoid, oid, amname, ");
+	if (fout->remoteVersion >= 90600)
+		appendPQExpBufferStr(query,
+							 "amtype, "
+							 "amhandler::pg_catalog.regproc AS amhandler ");
+	else
+		appendPQExpBufferStr(query,
+							 "'i'::pg_catalog.\"char\" AS amtype, "
+							 "'-'::pg_catalog.regproc AS amhandler ");
+	appendPQExpBufferStr(query, "FROM pg_am");
 
 	res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
 
@@ -5631,6 +5659,7 @@ getOpclasses(Archive *fout, int *numOpclasses)
 	OpclassInfo *opcinfo;
 	int			i_tableoid;
 	int			i_oid;
+	int			i_opcmethod;
 	int			i_opcname;
 	int			i_opcnamespace;
 	int			i_rolname;
@@ -5640,7 +5669,7 @@ getOpclasses(Archive *fout, int *numOpclasses)
 	 * system-defined opclasses at dump-out time.
 	 */
 
-	appendPQExpBuffer(query, "SELECT tableoid, oid, opcname, "
+	appendPQExpBuffer(query, "SELECT tableoid, oid, opcmethod, opcname, "
 					  "opcnamespace, "
 					  "(%s opcowner) AS rolname "
 					  "FROM pg_opclass",
@@ -5655,6 +5684,7 @@ getOpclasses(Archive *fout, int *numOpclasses)
 
 	i_tableoid = PQfnumber(res, "tableoid");
 	i_oid = PQfnumber(res, "oid");
+	i_opcmethod = PQfnumber(res, "opcmethod");
 	i_opcname = PQfnumber(res, "opcname");
 	i_opcnamespace = PQfnumber(res, "opcnamespace");
 	i_rolname = PQfnumber(res, "rolname");
@@ -5669,6 +5699,7 @@ getOpclasses(Archive *fout, int *numOpclasses)
 		opcinfo[i].dobj.namespace =
 			findNamespace(fout,
 						  atooid(PQgetvalue(res, i, i_opcnamespace)));
+		opcinfo[i].opcmethod = atooid(PQgetvalue(res, i, i_opcmethod));
 		opcinfo[i].rolname = pg_strdup(PQgetvalue(res, i, i_rolname));
 
 		/* Decide whether we want to dump it */
@@ -5706,6 +5737,7 @@ getOpfamilies(Archive *fout, int *numOpfamilies)
 	OpfamilyInfo *opfinfo;
 	int			i_tableoid;
 	int			i_oid;
+	int			i_opfmethod;
 	int			i_opfname;
 	int			i_opfnamespace;
 	int			i_rolname;
@@ -5724,7 +5756,7 @@ getOpfamilies(Archive *fout, int *numOpfamilies)
 	 * system-defined opfamilies at dump-out time.
 	 */
 
-	appendPQExpBuffer(query, "SELECT tableoid, oid, opfname, "
+	appendPQExpBuffer(query, "SELECT tableoid, oid, opfmethod, opfname, "
 					  "opfnamespace, "
 					  "(%s opfowner) AS rolname "
 					  "FROM pg_opfamily",
@@ -5740,6 +5772,7 @@ getOpfamilies(Archive *fout, int *numOpfamilies)
 	i_tableoid = PQfnumber(res, "tableoid");
 	i_oid = PQfnumber(res, "oid");
 	i_opfname = PQfnumber(res, "opfname");
+	i_opfmethod = PQfnumber(res, "opfmethod");
 	i_opfnamespace = PQfnumber(res, "opfnamespace");
 	i_rolname = PQfnumber(res, "rolname");
 
@@ -5753,6 +5786,7 @@ getOpfamilies(Archive *fout, int *numOpfamilies)
 		opfinfo[i].dobj.namespace =
 			findNamespace(fout,
 						  atooid(PQgetvalue(res, i, i_opfnamespace)));
+		opfinfo[i].opfmethod = atooid(PQgetvalue(res, i, i_opfmethod));
 		opfinfo[i].rolname = pg_strdup(PQgetvalue(res, i, i_rolname));
 
 		/* Decide whether we want to dump it */
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 552e9e1..7fcc544 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -219,6 +219,8 @@ typedef struct _oprInfo
 	DumpableObject dobj;
 	char	   *rolname;
 	char		oprkind;
+	Oid			oprleft;
+	Oid			oprright;
 	Oid			oprcode;
 } OprInfo;
 
@@ -232,12 +234,14 @@ typedef struct _accessMethodInfo
 typedef struct _opclassInfo
 {
 	DumpableObject dobj;
+	Oid			opcmethod;
 	char	   *rolname;
 } OpclassInfo;
 
 typedef struct _opfamilyInfo
 {
 	DumpableObject dobj;
+	Oid			opfmethod;
 	char	   *rolname;
 } OpfamilyInfo;
 
@@ -245,6 +249,7 @@ typedef struct _collInfo
 {
 	DumpableObject dobj;
 	char	   *rolname;
+	int			collencoding;
 } CollInfo;
 
 typedef struct _convInfo
@@ -662,6 +667,7 @@ extern TableInfo *findTableByOid(Oid oid);
 extern TypeInfo *findTypeByOid(Oid oid);
 extern FuncInfo *findFuncByOid(Oid oid);
 extern OprInfo *findOprByOid(Oid oid);
+extern AccessMethodInfo *findAccessMethodByOid(Oid oid);
 extern CollInfo *findCollationByOid(Oid oid);
 extern NamespaceInfo *findNamespaceByOid(Oid oid);
 extern ExtensionInfo *findExtensionByOid(Oid oid);
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index 51e6be9..892eda7 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -104,6 +104,8 @@ static DumpId postDataBoundId;
 
 
 static int	DOTypeNameCompare(const void *p1, const void *p2);
+static int	pgTypeNameCompare(Oid typid1, Oid typid2);
+static int	accessMethodNameCompare(Oid am1, Oid am2);
 static bool TopoSort(DumpableObject **objs,
 					 int numObjs,
 					 DumpableObject **ordering,
@@ -171,12 +173,39 @@ DOTypeNameCompare(const void *p1, const void *p2)
 	else if (obj2->namespace)
 		return 1;
 
-	/* Sort by name */
+	/*
+	 * Sort by name.  With a few exceptions, names here are single catalog
+	 * columns.  To get a fuller picture, grep pg_dump.c for "dobj.name = ".
+	 * Names here don't match "Name:" in plain format output, which is a
+	 * _tocEntry.tag.  For example, DumpableObject.name of a constraint is
+	 * pg_constraint.conname, but _tocEntry.tag of a constraint is relname and
+	 * conname joined with a space.
+	 */
 	cmpval = strcmp(obj1->name, obj2->name);
 	if (cmpval != 0)
 		return cmpval;
 
-	/* To have a stable sort order, break ties for some object types */
+	/*
+	 * Sort by type.  This helps types that share a type priority without
+	 * sharing a unique name constraint, e.g. opclass and opfamily.
+	 */
+	cmpval = obj1->objType - obj2->objType;
+	if (cmpval != 0)
+		return cmpval;
+
+	/*
+	 * To have a stable sort order, break ties for some object types.  Most
+	 * catalogs have a natural key, e.g. pg_proc_proname_args_nsp_index. Where
+	 * the above "namespace" and "name" comparisons don't cover all natural
+	 * key columns, compare the rest here.
+	 *
+	 * The natural key usually refers to other catalogs by surrogate keys.
+	 * Hence, this translates each of those references to the natural key of
+	 * the referenced catalog.  That may descend through multiple levels of
+	 * catalog references.  For example, to sort by pg_proc.proargtypes,
+	 * descend to each pg_type and then further to its pg_namespace, for an
+	 * overall sort by (nspname, typname).
+	 */
 	if (obj1->objType == DO_FUNC || obj1->objType == DO_AGG)
 	{
 		FuncInfo   *fobj1 = *(FuncInfo *const *) p1;
@@ -189,22 +218,10 @@ DOTypeNameCompare(const void *p1, const void *p2)
 			return cmpval;
 		for (i = 0; i < fobj1->nargs; i++)
 		{
-			TypeInfo   *argtype1 = findTypeByOid(fobj1->argtypes[i]);
-			TypeInfo   *argtype2 = findTypeByOid(fobj2->argtypes[i]);
-
-			if (argtype1 && argtype2)
-			{
-				if (argtype1->dobj.namespace && argtype2->dobj.namespace)
-				{
-					cmpval = strcmp(argtype1->dobj.namespace->dobj.name,
-									argtype2->dobj.namespace->dobj.name);
-					if (cmpval != 0)
-						return cmpval;
-				}
-				cmpval = strcmp(argtype1->dobj.name, argtype2->dobj.name);
-				if (cmpval != 0)
-					return cmpval;
-			}
+			cmpval = pgTypeNameCompare(fobj1->argtypes[i],
+									   fobj2->argtypes[i]);
+			if (cmpval != 0)
+				return cmpval;
 		}
 	}
 	else if (obj1->objType == DO_OPERATOR)
@@ -216,6 +233,57 @@ DOTypeNameCompare(const void *p1, const void *p2)
 		cmpval = (oobj2->oprkind - oobj1->oprkind);
 		if (cmpval != 0)
 			return cmpval;
+		/* Within an oprkind, sort by argument type names */
+		cmpval = pgTypeNameCompare(oobj1->oprleft, oobj2->oprleft);
+		if (cmpval != 0)
+			return cmpval;
+		cmpval = pgTypeNameCompare(oobj1->oprright, oobj2->oprright);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	else if (obj1->objType == DO_OPCLASS)
+	{
+		OpclassInfo *opcobj1 = *(OpclassInfo *const *) p1;
+		OpclassInfo *opcobj2 = *(OpclassInfo *const *) p2;
+
+		/* Sort by access method name, per pg_opclass_am_name_nsp_index */
+		cmpval = accessMethodNameCompare(opcobj1->opcmethod,
+										 opcobj2->opcmethod);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	else if (obj1->objType == DO_OPFAMILY)
+	{
+		OpfamilyInfo *opfobj1 = *(OpfamilyInfo *const *) p1;
+		OpfamilyInfo *opfobj2 = *(OpfamilyInfo *const *) p2;
+
+		/* Sort by access method name, per pg_opfamily_am_name_nsp_index */
+		cmpval = accessMethodNameCompare(opfobj1->opfmethod,
+										 opfobj2->opfmethod);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	else if (obj1->objType == DO_COLLATION)
+	{
+		CollInfo   *cobj1 = *(CollInfo *const *) p1;
+		CollInfo   *cobj2 = *(CollInfo *const *) p2;
+
+		/*
+		 * Sort by encoding, per pg_collation_name_enc_nsp_index. Technically,
+		 * this is not necessary, because wherever this changes dump order,
+		 * restoring the dump fails anyway.  CREATE COLLATION can't create a
+		 * tie for this to break, because it imposes restrictions to make
+		 * (nspname, collname) uniquely identify a collation within a given
+		 * DatabaseEncoding.  While pg_import_system_collations() can create a
+		 * tie, pg_dump+restore fails after
+		 * pg_import_system_collations('my_schema') does so. However, there's
+		 * little to gain by ignoring one natural key column on the basis of
+		 * those limitations elsewhere, so respect the full natural key like
+		 * we do for other object types.
+		 */
+		cmpval = cobj1->collencoding - cobj2->collencoding;
+		if (cmpval != 0)
+			return cmpval;
 	}
 	else if (obj1->objType == DO_ATTRDEF)
 	{
@@ -260,11 +328,132 @@ DOTypeNameCompare(const void *p1, const void *p2)
 		if (cmpval != 0)
 			return cmpval;
 	}
+	else if (obj1->objType == DO_CONSTRAINT)
+	{
+		ConstraintInfo *robj1 = *(ConstraintInfo *const *) p1;
+		ConstraintInfo *robj2 = *(ConstraintInfo *const *) p2;
+
+		/*
+		 * Sort domain constraints before table constraints, for consistency
+		 * with our decision to sort CREATE DOMAIN before CREATE TABLE.
+		 */
+		if (robj1->condomain)
+		{
+			if (robj2->condomain)
+			{
+				/* Sort by domain name (domain namespace was considered) */
+				cmpval = strcmp(robj1->condomain->dobj.name,
+								robj2->condomain->dobj.name);
+				if (cmpval != 0)
+					return cmpval;
+			}
+			else
+				return dbObjectTypePriority[DO_TYPE] - dbObjectTypePriority[DO_TABLE];
+		}
+		else if (robj2->condomain)
+			return dbObjectTypePriority[DO_TABLE] - dbObjectTypePriority[DO_TYPE];
+		else
+		{
+			/* Sort by table name (table namespace was considered already) */
+			cmpval = strcmp(robj1->contable->dobj.name,
+							robj2->contable->dobj.name);
+			if (cmpval != 0)
+				return cmpval;
+		}
+	}
+	else if (obj1->objType == DO_PUBLICATION_REL)
+	{
+		PublicationRelInfo *probj1 = *(PublicationRelInfo *const *) p1;
+		PublicationRelInfo *probj2 = *(PublicationRelInfo *const *) p2;
+
+		/* Sort by publication name, since (namespace, name) match the rel */
+		cmpval = strcmp(probj1->publication->dobj.name,
+						probj2->publication->dobj.name);
+		if (cmpval != 0)
+			return cmpval;
+	}
 
-	/* Usually shouldn't get here, but if we do, sort by OID */
+	/*
+	 * Shouldn't get here except after catalog corruption, but if we do, sort
+	 * by OID.  This may make logically-identical databases differ in the
+	 * order of objects in dump output.  Users will get spurious schema diffs.
+	 * Expect flaky failures of 002_pg_upgrade.pl test 'dump outputs from
+	 * original and restored regression databases match' if the regression
+	 * database contains objects allowing that test to reach here.  That's a
+	 * consequence of the test using "pg_restore -j", which doesn't fully
+	 * constrain OID assignment order.
+	 */
+	Assert(false);
 	return oidcmp(obj1->catId.oid, obj2->catId.oid);
 }
 
+/* Compare two OID-identified pg_type values by nspname, then by typname. */
+static int
+pgTypeNameCompare(Oid typid1, Oid typid2)
+{
+	TypeInfo   *typobj1;
+	TypeInfo   *typobj2;
+	int			cmpval;
+
+	if (typid1 == typid2)
+		return 0;
+
+	typobj1 = findTypeByOid(typid1);
+	typobj2 = findTypeByOid(typid2);
+
+	if (!typobj1 || !typobj2)
+	{
+		/*
+		 * getTypes() didn't find some OID.  Assume catalog corruption, e.g.
+		 * an oprright value without the corresponding OID in a pg_type row.
+		 * Report as "equal", so the caller uses the next available basis for
+		 * comparison, e.g. the next function argument.
+		 *
+		 * Unary operators have InvalidOid in oprleft (if oprkind='r') or in
+		 * oprright (if oprkind='l').  Caller already sorted by oprkind,
+		 * calling us only for like-kind operators.  Hence, "typid1 == typid2"
+		 * took care of InvalidOid.  (v14 removed postfix operator support.
+		 * Hence, when dumping from v14+, only oprleft can be InvalidOid.)
+		 */
+		Assert(false);
+		return 0;
+	}
+
+	if (!typobj1->dobj.namespace || !typobj2->dobj.namespace)
+		Assert(false);			/* catalog corruption */
+	else
+	{
+		cmpval = strcmp(typobj1->dobj.namespace->dobj.name,
+						typobj2->dobj.namespace->dobj.name);
+		if (cmpval != 0)
+			return cmpval;
+	}
+	return strcmp(typobj1->dobj.name, typobj2->dobj.name);
+}
+
+/* Compare two OID-identified pg_am values by amname. */
+static int
+accessMethodNameCompare(Oid am1, Oid am2)
+{
+	AccessMethodInfo *amobj1;
+	AccessMethodInfo *amobj2;
+
+	if (am1 == am2)
+		return 0;
+
+	amobj1 = findAccessMethodByOid(am1);
+	amobj2 = findAccessMethodByOid(am2);
+
+	if (!amobj1 || !amobj2)
+	{
+		/* catalog corruption: handle like pgTypeNameCompare() does */
+		Assert(false);
+		return 0;
+	}
+
+	return strcmp(amobj1->dobj.name, amobj2->dobj.name);
+}
+
 
 /*
  * Sort the given objects into a safe dump order using dependency
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index b7ce080..35595fb 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -344,3 +344,11 @@ NOTICE:  drop cascades to table pub_test.testpub_nopk
 RESET SESSION AUTHORIZATION;
 DROP ROLE regress_publication_user, regress_publication_user2;
 DROP ROLE regress_publication_user_dummy;
+-- stage objects for pg_dump tests
+CREATE SCHEMA pubme CREATE TABLE t0 (c int, d int) CREATE TABLE t1 (c int);
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION dump_pub_1ct FOR TABLE ONLY pubme.t0;
+CREATE PUBLICATION dump_pub_2ct FOR TABLE ONLY pubme.t0, pubme.t1;
+CREATE PUBLICATION dump_pub_all FOR TABLE ONLY pubme.t0, pubme.t1
+  WITH (publish_via_partition_root = true);
+RESET client_min_messages;
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index 7d5c937..021d4e2 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -218,3 +218,12 @@ DROP SCHEMA pub_test CASCADE;
 RESET SESSION AUTHORIZATION;
 DROP ROLE regress_publication_user, regress_publication_user2;
 DROP ROLE regress_publication_user_dummy;
+
+-- stage objects for pg_dump tests
+CREATE SCHEMA pubme CREATE TABLE t0 (c int, d int) CREATE TABLE t1 (c int);
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION dump_pub_1ct FOR TABLE ONLY pubme.t0;
+CREATE PUBLICATION dump_pub_2ct FOR TABLE ONLY pubme.t0, pubme.t1;
+CREATE PUBLICATION dump_pub_all FOR TABLE ONLY pubme.t0, pubme.t1
+  WITH (publish_via_partition_root = true);
+RESET client_min_messages;
#6Robert Haas
robertmhaas@gmail.com
In reply to: Noah Misch (#5)
Re: Test instability when pg_dump orders by OID

On Thu, Jul 24, 2025 at 10:27 PM Noah Misch <noah@leadboat.com> wrote:

I regret missing those in v1. I've attached v2, including branch-specific
patches. I'll first need to back-patch 350e6b8, which fixed sorting of CREATE
RULE, to v17 and earlier. Since 350e6b8 is conflict-free all the way back to
v13, I'm not attaching it.

Back-patching 350e6b8 to facilitate back-patching this seems OK. I did
a read-through of dobjcomp20-disambiguate-v2.patch and have no further
review comments. I did not read through the back-patched versions on
the assumption that back-porting was straightforward enough that a
separate review was not required.

--
Robert Haas
EDB: http://www.enterprisedb.com

#7Alexander Lakhin
exclusion@gmail.com
In reply to: Noah Misch (#1)
Re: Test instability when pg_dump orders by OID

Hello Noah,

07.07.2025 22:26, Noah Misch wrote:

A 002_pg_upgrade.pl run got swapped order of tags "notnull_tbl1_upg nn" and
"notnull_parent_upg nn" for the schema diff test that commit
172259afb563d35001410dc6daad78b250924038 added in v18:

@@ -436873,14 +436873,14 @@
ALTER TABLE public.insert_tbl
ADD CONSTRAINT ne_insert_tbl_con CHECK (((x + z) = 1)) NOT ENFORCED;
--
--- Name: notnull_tbl1_upg nn; Type: CONSTRAINT; Schema: public; Owner: nm
+-- Name: notnull_parent_upg nn; Type: CONSTRAINT; Schema: public; Owner: nm
--
-ALTER TABLE public.notnull_tbl1_upg
+ALTER TABLE public.notnull_parent_upg
ADD CONSTRAINT nn NOT NULL a NOT VALID;
--
--- Name: notnull_parent_upg nn; Type: CONSTRAINT; Schema: public; Owner: nm
+-- Name: notnull_tbl1_upg nn; Type: CONSTRAINT; Schema: public; Owner: nm
--
-ALTER TABLE public.notnull_parent_upg
+ALTER TABLE public.notnull_tbl1_upg
It's rather funny that a few days before the fix is going to be pushed,
hippopotamus proved the need for it [1] (I saw no such failures on the
buildfarm before):
...
[17:09:56.372](2.577s) not ok 8 - dump outputs from original and restored regression databases match
[17:09:56.372](0.000s)
[17:09:56.372](0.000s) #   Failed test 'dump outputs from original and restored regression databases match'
[17:09:56.372](0.000s) #   at 
/home/buildfarm/hippopotamus/buildroot/REL_18_STABLE/pgsql.build/src/bin/pg_upgrade/../../../src/test/perl/PostgreSQL/Test/Utils.pm 
line 800.
[17:09:56.373](0.000s) #          got: '1'
#     expected: '0'
=== diff of 
/home/buildfarm/hippopotamus/buildroot/REL_18_STABLE/pgsql.build/src/bin/pg_upgrade/tmp_check/tmp_test_v61D/src_dump.sql_adjusted 
and 
/home/buildfarm/hippopotamus/buildroot/REL_18_STABLE/pgsql.build/src/bin/pg_upgrade/tmp_check/tmp_test_v61D/dest_dump.sql_adjusted
=== stdout ===
--- 
/home/buildfarm/hippopotamus/buildroot/REL_18_STABLE/pgsql.build/src/bin/pg_upgrade/tmp_check/tmp_test_v61D/src_dump.sql_adjusted 
2025-07-28 17:09:55.040029896 +0200
+++ 
/home/buildfarm/hippopotamus/buildroot/REL_18_STABLE/pgsql.build/src/bin/pg_upgrade/tmp_check/tmp_test_v61D/dest_dump.sql_adjusted 
2025-07-28 17:09:56.208057237 +0200
@@ -436960,14 +436960,14 @@
  ALTER TABLE public.insert_tbl
      ADD CONSTRAINT ne_insert_tbl_con CHECK (((x + z) = 1)) NOT ENFORCED;
  --
--- Name: notnull_tbl1_upg nn; Type: CONSTRAINT; Schema: public; Owner: buildfarm
+-- Name: notnull_parent_upg nn; Type: CONSTRAINT; Schema: public; Owner: buildfarm
  --
-ALTER TABLE public.notnull_tbl1_upg
+ALTER TABLE public.notnull_parent_upg
      ADD CONSTRAINT nn NOT NULL a NOT VALID;
  --
--- Name: notnull_parent_upg nn; Type: CONSTRAINT; Schema: public; Owner: buildfarm
+-- Name: notnull_tbl1_upg nn; Type: CONSTRAINT; Schema: public; Owner: buildfarm
  --
-ALTER TABLE public.notnull_parent_upg
+ALTER TABLE public.notnull_tbl1_upg
      ADD CONSTRAINT nn NOT NULL a NOT VALID;
  --
  -- Name: notnul

Thank you for working on the fix!

[1]: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hippopotamus&amp;dt=2025-07-28%2015%3A05%3A11

Best regards,
Alexander

#8Noah Misch
noah@leadboat.com
In reply to: Robert Haas (#6)
Re: Test instability when pg_dump orders by OID

On Fri, Jul 25, 2025 at 02:01:01PM -0400, Robert Haas wrote:

On Thu, Jul 24, 2025 at 10:27 PM Noah Misch <noah@leadboat.com> wrote:

I regret missing those in v1. I've attached v2, including branch-specific
patches. I'll first need to back-patch 350e6b8, which fixed sorting of CREATE
RULE, to v17 and earlier. Since 350e6b8 is conflict-free all the way back to
v13, I'm not attaching it.

Back-patching 350e6b8 to facilitate back-patching this seems OK. I did
a read-through of dobjcomp20-disambiguate-v2.patch and have no further
review comments. I did not read through the back-patched versions on
the assumption that back-porting was straightforward enough that a
separate review was not required.

Pushed as 0decd5e. v14 supports binary-upgrade from v8.3 and regular dump
from v8.0. That required two other changes. First, pg_opclass.opcmethod had
name opcamid until v8.3 (commit a78fcfb). Accounting for both names was
trivial. Second, pg_am first had fixed OID AccessMethodRelationId in v8.1
(commit 7c13781). The find*ByOid functions have been assuming fixed catalog
OIDs since v15's commit 92316a4. The $SUBJECT commit added
findAccessMethodByOid() to all branches, so I changed the v14/v13
findAccessMethodByOid() to be more like v14/v13 findOprByOid(), which doesn't
assume AccessMethodRelationId. If folks want more details, let me know.

#9Alexander Lakhin
exclusion@gmail.com
In reply to: Noah Misch (#8)
Re: Test instability when pg_dump orders by OID

Hello Noah,

04.08.2025 03:03, Noah Misch wrote:

Pushed as 0decd5e. ...

Please look at a new anomaly introduced with that commit. The following
script:
createdb regression

echo "
CREATE USER u1;
ALTER DEFAULT PRIVILEGES FOR ROLE u1 REVOKE INSERT ON TABLES FROM u1;

CREATE USER u2;
ALTER DEFAULT PRIVILEGES FOR ROLE u2 REVOKE INSERT ON TABLES FROM u2;
" | psql regression

pg_dump regression

triggers:
pg_dump: pg_dump_sort.c:454: DOTypeNameCompare: Assertion `0' failed.

Reproduced on master and REL_13_STABLE.

Best  regards,
Alexander

#10Kirill Reshke
reshkekirill@gmail.com
In reply to: Alexander Lakhin (#9)
1 attachment(s)
Re: Test instability when pg_dump orders by OID

Hi all!

On Sun, 10 Aug 2025 at 12:00, Alexander Lakhin <exclusion@gmail.com> wrote:

Hello Noah,

04.08.2025 03:03, Noah Misch wrote:

Pushed as 0decd5e. ...

Please look at a new anomaly introduced with that commit. The following
script:
createdb regression

echo "
CREATE USER u1;
ALTER DEFAULT PRIVILEGES FOR ROLE u1 REVOKE INSERT ON TABLES FROM u1;

CREATE USER u2;
ALTER DEFAULT PRIVILEGES FOR ROLE u2 REVOKE INSERT ON TABLES FROM u2;
" | psql regression

pg_dump regression

triggers:
pg_dump: pg_dump_sort.c:454: DOTypeNameCompare: Assertion `0' failed.

I reproduced this. Indeed, in case of default acl we happen to use OID sort.

PFA resolves this issue. I simply added DEFAULT ACL case-specific
tiebreaker that resolves object order.

--
Best regards,
Kirill Reshke

Attachments:

0001-Handle-DEFAULT-ACL-case-in-DOTypeNameCompare-functio.patchapplication/octet-stream; name=0001-Handle-DEFAULT-ACL-case-in-DOTypeNameCompare-functio.patchDownload
From f271fcb56f32fb6f9ebf96710145305791932061 Mon Sep 17 00:00:00 2001
From: reshke <reshke@double.cloud>
Date: Sun, 10 Aug 2025 11:34:37 +0000
Subject: [PATCH] Handle DEFAULT ACL case in DOTypeNameCompare function

Previously, default acl objects used to be sorted by OID in
pg_dump output. Since 0decd5e this is considered as bad pratice,
so compare them by defaclrole.
---
 src/bin/pg_dump/pg_dump_sort.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index a02da3e9652..08667e95a40 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -440,6 +440,16 @@ DOTypeNameCompare(const void *p1, const void *p2)
 		if (cmpval != 0)
 			return cmpval;
 	}
+	else if (obj1->objType == DO_DEFAULT_ACL)
+	{
+		DefaultACLInfo *daclobj1 = *(DefaultACLInfo *const *) p1;
+		DefaultACLInfo *daclobj2 = *(DefaultACLInfo *const *) p2;
+
+		/* Sort by defaclrole */
+		cmpval = strcmp(daclobj1->defaclrole, daclobj2->defaclrole);
+		if (cmpval != 0)
+			return cmpval;
+	}
 
 	/*
 	 * Shouldn't get here except after catalog corruption, but if we do, sort
-- 
2.43.0

#11Noah Misch
noah@leadboat.com
In reply to: Kirill Reshke (#10)
Re: Test instability when pg_dump orders by OID

On Sun, Aug 10, 2025 at 04:41:20PM +0500, Kirill Reshke wrote:

On Sun, 10 Aug 2025 at 12:00, Alexander Lakhin <exclusion@gmail.com> wrote:

04.08.2025 03:03, Noah Misch wrote:

Pushed as 0decd5e. ...

Please look at a new anomaly introduced with that commit. The following
script:
createdb regression

echo "
CREATE USER u1;
ALTER DEFAULT PRIVILEGES FOR ROLE u1 REVOKE INSERT ON TABLES FROM u1;

CREATE USER u2;
ALTER DEFAULT PRIVILEGES FOR ROLE u2 REVOKE INSERT ON TABLES FROM u2;
" | psql regression

pg_dump regression

triggers:
pg_dump: pg_dump_sort.c:454: DOTypeNameCompare: Assertion `0' failed.

I reproduced this. Indeed, in case of default acl we happen to use OID sort.

Thanks. Given the current state of freeze for tomorrow's release wrap, the
decision is less obvious than usual. I'm seeing these options:

1. Remove the new assertion in v13-v18.
2. Push your proposed fix.
3. Change nothing. (This would be the choice if one is maximally concerned
about deviating from the freeze and unconcerned about --enable-cassert
builds of releases.)

I am inclined to make today's change be (1). A fresh audit of catalog PRIMARY
KEY and UNIQUE constraints didn't find any more missed cases, but (1) still
feels like the right level of cautiousness. If there are no objections in the
next 3hr, I'll proceed with (1).

PFA resolves this issue. I simply added DEFAULT ACL case-specific
tiebreaker that resolves object order.

Thanks. Could you make src/test/regress create regression database objects so
the code addition has coverage? Using pg_signal_backend and
pg_read_all_settings as the default ACL role names should avoid that suite's
limitations. (The suite must run under any role name and must drop any roles
it creates, so it can't assume any particular non-system role name survives
the suite.)

#12Robert Haas
robertmhaas@gmail.com
In reply to: Noah Misch (#11)
Re: Test instability when pg_dump orders by OID

On Sun, Aug 10, 2025 at 12:37 PM Noah Misch <noah@leadboat.com> wrote:

Thanks. Given the current state of freeze for tomorrow's release wrap, the
decision is less obvious than usual. I'm seeing these options:

1. Remove the new assertion in v13-v18.
2. Push your proposed fix.
3. Change nothing. (This would be the choice if one is maximally concerned
about deviating from the freeze and unconcerned about --enable-cassert
builds of releases.)

I am inclined to make today's change be (1).

Sounds right to me.

--
Robert Haas
EDB: http://www.enterprisedb.com

#13Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#12)
Re: Test instability when pg_dump orders by OID

Robert Haas <robertmhaas@gmail.com> writes:

On Sun, Aug 10, 2025 at 12:37 PM Noah Misch <noah@leadboat.com> wrote:

Thanks. Given the current state of freeze for tomorrow's release wrap, the
decision is less obvious than usual. I'm seeing these options:

1. Remove the new assertion in v13-v18.
2. Push your proposed fix.
3. Change nothing. (This would be the choice if one is maximally concerned
about deviating from the freeze and unconcerned about --enable-cassert
builds of releases.)

I am inclined to make today's change be (1).

Sounds right to me.

I agree. The fact that this case escaped notice suggests that there
might be more. We don't want to ship a version of pg_dump that will
assert if that happens. Keep the assert in HEAD, for sure, but it's
uncomfortable having it in back branches.

As for the actual fix, push it after the freeze lifts. The fact that
we didn't quite get there on making dump order stable isn't a
freeze-break-worthy bug.

regards, tom lane

#14Kirill Reshke
reshkekirill@gmail.com
In reply to: Noah Misch (#11)
1 attachment(s)
Re: Test instability when pg_dump orders by OID

On Sun, 10 Aug 2025 at 21:37, Noah Misch <noah@leadboat.com> wrote:

Thanks. Could you make src/test/regress create regression database objects so
the code addition has coverage? Using pg_signal_backend and
pg_read_all_settings as the default ACL role names should avoid that suite's
limitations. (The suite must run under any role name and must drop any roles
it creates, so it can't assume any particular non-system role name survives
the suite.)

Here is my attempt at implementing necessary legwork. It's v3 because
I accidentally cleared the CC list in my previous attempt. Noah kindly
explained to me how additions to the regress test will cause pg_dump
logic to be tested as well.
TIL 002_pg_upgarde.pl runs a regression suite, so if we create any
database objects in it, it will end up being dumped and restored in
that test.
So, I checked that without changes in pg_dump_sort.c, 002_pg_upgarde
fails and with changes it does not.

PFA. I am not horribly sure about my additions to the
`src/test/regress/sql/privileges.sql` file, maybe appending SQL to the
end of the file is not the best option and there is a better place.

--
Best regards,
Kirill Reshke

Attachments:

v3-0001-Handle-DEFAULT-ACL-case-in-DOTypeNameCompare-func.patchapplication/octet-stream; name=v3-0001-Handle-DEFAULT-ACL-case-in-DOTypeNameCompare-func.patchDownload
From 34829da58cdde613899fe01d1e9ceecd2815eac7 Mon Sep 17 00:00:00 2001
From: reshke <reshke@double.cloud>
Date: Sun, 10 Aug 2025 11:34:37 +0000
Subject: [PATCH v3] Handle DEFAULT ACL case in DOTypeNameCompare function

Previously, default acl objects used to be sorted by OID in
pg_dump output. Since 0decd5e this is considered as bad pratice,
so compare them by defaclrole.
---
 src/bin/pg_dump/pg_dump_sort.c           | 10 ++++++++++
 src/test/regress/expected/privileges.out |  3 +++
 src/test/regress/sql/privileges.sql      |  4 ++++
 3 files changed, 17 insertions(+)

diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index a02da3e9652..08667e95a40 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -440,6 +440,16 @@ DOTypeNameCompare(const void *p1, const void *p2)
 		if (cmpval != 0)
 			return cmpval;
 	}
+	else if (obj1->objType == DO_DEFAULT_ACL)
+	{
+		DefaultACLInfo *daclobj1 = *(DefaultACLInfo *const *) p1;
+		DefaultACLInfo *daclobj2 = *(DefaultACLInfo *const *) p2;
+
+		/* Sort by defaclrole */
+		cmpval = strcmp(daclobj1->defaclrole, daclobj2->defaclrole);
+		if (cmpval != 0)
+			return cmpval;
+	}
 
 	/*
 	 * Shouldn't get here except after catalog corruption, but if we do, sort
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 602a6b255bc..ad9a0c22188 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -3448,6 +3448,9 @@ DROP SCHEMA reindex_test;
 DROP ROLE regress_no_maintain;
 DROP ROLE regress_maintain;
 DROP ROLE regress_maintain_all;
+-- leave some default ACLs for pg_upgrade's dump-restore test input.
+ALTER DEFAULT PRIVILEGES FOR ROLE pg_signal_backend REVOKE INSERT ON TABLES FROM pg_signal_backend;
+ALTER DEFAULT PRIVILEGES FOR ROLE pg_read_all_settings REVOKE INSERT ON TABLES FROM pg_read_all_settings;
 -- grantor selection
 CREATE ROLE regress_grantor1;
 CREATE ROLE regress_grantor2 ROLE regress_grantor1;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 3eacc1340aa..f4a4701c6c8 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -2095,6 +2095,10 @@ DROP ROLE regress_no_maintain;
 DROP ROLE regress_maintain;
 DROP ROLE regress_maintain_all;
 
+-- leave some default ACLs for pg_upgrade's dump-restore test input.
+ALTER DEFAULT PRIVILEGES FOR ROLE pg_signal_backend REVOKE INSERT ON TABLES FROM pg_signal_backend;
+ALTER DEFAULT PRIVILEGES FOR ROLE pg_read_all_settings REVOKE INSERT ON TABLES FROM pg_read_all_settings;
+
 -- grantor selection
 CREATE ROLE regress_grantor1;
 CREATE ROLE regress_grantor2 ROLE regress_grantor1;
-- 
2.43.0

#15Kirill Reshke
reshkekirill@gmail.com
In reply to: Noah Misch (#11)
Re: Test instability when pg_dump orders by OID

On Sun, 10 Aug 2025 at 21:37, Noah Misch <noah@leadboat.com> wrote:

Thanks. Given the current state of freeze for tomorrow's release wrap, the
decision is less obvious than usual. I'm seeing these options:

1. Remove the new assertion in v13-v18.
2. Push your proposed fix.
3. Change nothing. (This would be the choice if one is maximally concerned
about deviating from the freeze and unconcerned about --enable-cassert
builds of releases.)

I am inclined to make today's change be (1). A fresh audit of catalog PRIMARY
KEY and UNIQUE constraints didn't find any more missed cases, but (1) still
feels like the right level of cautiousness. If there are no objections in the
next 3hr, I'll proceed with (1).

Hi! I can see we have option (1) (28e7252 etc). Can we now move
forward with option (2) for HEAD?

--
Best regards,
Kirill Reshke

#16Noah Misch
noah@leadboat.com
In reply to: Kirill Reshke (#15)
Re: Test instability when pg_dump orders by OID

On Wed, Aug 20, 2025 at 10:11:15AM +0500, Kirill Reshke wrote:

On Sun, 10 Aug 2025 at 21:37, Noah Misch <noah@leadboat.com> wrote:

Thanks. Given the current state of freeze for tomorrow's release wrap, the
decision is less obvious than usual. I'm seeing these options:

1. Remove the new assertion in v13-v18.
2. Push your proposed fix.
3. Change nothing. (This would be the choice if one is maximally concerned
about deviating from the freeze and unconcerned about --enable-cassert
builds of releases.)

I am inclined to make today's change be (1). A fresh audit of catalog PRIMARY
KEY and UNIQUE constraints didn't find any more missed cases, but (1) still
feels like the right level of cautiousness. If there are no objections in the
next 3hr, I'll proceed with (1).

Hi! I can see we have option (1) (28e7252 etc). Can we now move
forward with option (2) for HEAD?

Yep. It's in my queue.

#17Noah Misch
noah@leadboat.com
In reply to: Kirill Reshke (#14)
Re: Test instability when pg_dump orders by OID

On Mon, Aug 11, 2025 at 12:20:09AM +0500, Kirill Reshke wrote:

On Sun, 10 Aug 2025 at 21:37, Noah Misch <noah@leadboat.com> wrote:

Thanks. Could you make src/test/regress create regression database objects so
the code addition has coverage? Using pg_signal_backend and
pg_read_all_settings as the default ACL role names should avoid that suite's
limitations. (The suite must run under any role name and must drop any roles
it creates, so it can't assume any particular non-system role name survives
the suite.)

Here is my attempt at implementing necessary legwork. It's v3 because
I accidentally cleared the CC list in my previous attempt. Noah kindly
explained to me how additions to the regress test will cause pg_dump
logic to be tested as well.
TIL 002_pg_upgarde.pl runs a regression suite, so if we create any
database objects in it, it will end up being dumped and restored in
that test.
So, I checked that without changes in pg_dump_sort.c, 002_pg_upgarde
fails and with changes it does not.

Great.

PFA. I am not horribly sure about my additions to the
`src/test/regress/sql/privileges.sql` file, maybe appending SQL to the
end of the file is not the best option and there is a better place.

I like how src/test/regress/sql/collate.icu.utf8.sql puts that kind of thing
just after cleanup, so I put it there. Pushed as b61a5c4 with a few other
cosmetic changes. Thanks.

#18Noah Misch
noah@leadboat.com
In reply to: Noah Misch (#17)
Re: Test instability when pg_dump orders by OID

On Fri, Aug 22, 2025 at 10:20:19PM -0700, Noah Misch wrote:

On Mon, Aug 11, 2025 at 12:20:09AM +0500, Kirill Reshke wrote:

On Sun, 10 Aug 2025 at 21:37, Noah Misch <noah@leadboat.com> wrote:

Thanks. Could you make src/test/regress create regression database objects so
the code addition has coverage? Using pg_signal_backend and
pg_read_all_settings as the default ACL role names should avoid that suite's
limitations. (The suite must run under any role name and must drop any roles
it creates, so it can't assume any particular non-system role name survives
the suite.)

Here is my attempt at implementing necessary legwork. It's v3 because
I accidentally cleared the CC list in my previous attempt. Noah kindly
explained to me how additions to the regress test will cause pg_dump
logic to be tested as well.
TIL 002_pg_upgarde.pl runs a regression suite, so if we create any
database objects in it, it will end up being dumped and restored in
that test.
So, I checked that without changes in pg_dump_sort.c, 002_pg_upgarde
fails and with changes it does not.

Great.

PFA. I am not horribly sure about my additions to the
`src/test/regress/sql/privileges.sql` file, maybe appending SQL to the
end of the file is not the best option and there is a better place.

I like how src/test/regress/sql/collate.icu.utf8.sql puts that kind of thing
just after cleanup, so I put it there. Pushed as b61a5c4 with a few other
cosmetic changes. Thanks.

TestUpgradeXversion fails:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=copperhead&amp;dt=2025-08-23%2007%3A34%3A38

--- /home/pgbf/buildroot/upgrade.copperhead/REL_18_STABLE/origin-REL_16_STABLE.sql.fixed	2025-08-23 10:28:16.464887433 +0200
+++ /home/pgbf/buildroot/upgrade.copperhead/REL_18_STABLE/converted-REL_16_STABLE-to-REL_18_STABLE.sql.fixed	2025-08-23 10:28:16.508887289 +0200
@@ -606490,13 +606490,13 @@
 --
 -- Name: DEFAULT PRIVILEGES FOR TABLES; Type: DEFAULT ACL; Schema: -; Owner: pg_read_all_settings
 --
-ALTER DEFAULT PRIVILEGES FOR ROLE pg_read_all_settings REVOKE SELECT,INSERT,REFERENCES,DELETE,TRIGGER,TRUNCATE,UPDATE ON TABLES FROM pg_read_all_settings;
-ALTER DEFAULT PRIVILEGES FOR ROLE pg_read_all_settings GRANT SELECT,REFERENCES,DELETE,TRIGGER,TRUNCATE,UPDATE ON TABLES TO pg_read_all_settings;
+ALTER DEFAULT PRIVILEGES FOR ROLE pg_read_all_settings REVOKE ALL ON TABLES FROM pg_read_all_settings;
+ALTER DEFAULT PRIVILEGES FOR ROLE pg_read_all_settings GRANT SELECT,REFERENCES,DELETE,TRIGGER,TRUNCATE,MAINTAIN,UPDATE ON TABLES TO pg_read_all_settings;

Crossing the boundary of the MAINTAIN privilege existing seems relevant. Will
fix. (My checklist did tell me to do a local run of TestUpgradeXversion. I
skipped it, betting this patch wouldn't break that test. I lost that bet.)

#19Noah Misch
noah@leadboat.com
In reply to: Noah Misch (#18)
Re: Test instability when pg_dump orders by OID

On Sat, Aug 23, 2025 at 07:45:05AM -0700, Noah Misch wrote:

On Fri, Aug 22, 2025 at 10:20:19PM -0700, Noah Misch wrote:

Pushed as b61a5c4 with a few other
cosmetic changes. Thanks.

TestUpgradeXversion fails:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=copperhead&amp;dt=2025-08-23%2007%3A34%3A38

--- /home/pgbf/buildroot/upgrade.copperhead/REL_18_STABLE/origin-REL_16_STABLE.sql.fixed	2025-08-23 10:28:16.464887433 +0200
+++ /home/pgbf/buildroot/upgrade.copperhead/REL_18_STABLE/converted-REL_16_STABLE-to-REL_18_STABLE.sql.fixed	2025-08-23 10:28:16.508887289 +0200
@@ -606490,13 +606490,13 @@
--
-- Name: DEFAULT PRIVILEGES FOR TABLES; Type: DEFAULT ACL; Schema: -; Owner: pg_read_all_settings
--
-ALTER DEFAULT PRIVILEGES FOR ROLE pg_read_all_settings REVOKE SELECT,INSERT,REFERENCES,DELETE,TRIGGER,TRUNCATE,UPDATE ON TABLES FROM pg_read_all_settings;
-ALTER DEFAULT PRIVILEGES FOR ROLE pg_read_all_settings GRANT SELECT,REFERENCES,DELETE,TRIGGER,TRUNCATE,UPDATE ON TABLES TO pg_read_all_settings;
+ALTER DEFAULT PRIVILEGES FOR ROLE pg_read_all_settings REVOKE ALL ON TABLES FROM pg_read_all_settings;
+ALTER DEFAULT PRIVILEGES FOR ROLE pg_read_all_settings GRANT SELECT,REFERENCES,DELETE,TRIGGER,TRUNCATE,MAINTAIN,UPDATE ON TABLES TO pg_read_all_settings;

Crossing the boundary of the MAINTAIN privilege existing seems relevant. Will
fix. (My checklist did tell me to do a local run of TestUpgradeXversion. I
skipped it, betting this patch wouldn't break that test. I lost that bet.)

I've pushed commit ad44124 to fix this by changing "REVOKE INSERT ON TABLES"
to "REVOKE USAGE ON TYPES". Types have the same one privilege in all
supported versions, so they avoid the problem. An alternative was to GRANT to
a different role, not REVOKE from the owner role.

I noticed that the buildfarm diff revealed suboptimal pg_dump behaviors
involving v17+ pg_dump dumping a v16- origin server. I failed to resist
studying that. While I plan not to work on these myself, see below for what I
found. If someone works on these, that probably deserves its own thread:

==== Dump+reload+dump sees a schema diff

Consider the sequence:

1. start a v16 server
2. run the commands commit b61a5c4 added to privileges.sql
3. dump the v16 server w/ v17 pg_dump
includes: REVOKE (all but MAINTAIN); GRANT (all but INSERT and MAINTAIN)
4. start a v17 server
5. restore the dump to the v17 server
6. dump the v17 server
includes: REVOKE ALL; GRANT (all but INSERT)

The dump from (3) differs from the dump in (6), hence the buildfarm failure.
Both dumps correctly reproduce catalog state, but it's not great to vary
output like this.

buildACLCommands() operates at the aclitem level. If it gets these args:

acls = defaclacl = {pg_signal_backend=rwdDxt/pg_signal_backend}
baseacls = acldefault = {pg_signal_backend=arwdDxt/pg_signal_backend}

then it revokes the 7 "baseacls" privileges and grants the 6 "acls"
privileges. By diffing individual ai_privs bits instead of whole aclitem
values, it could deduce that REVOKE INSERT suffices. In other words, do the
equivalent of exploding each aclitem into individual privilege records before
diffing to decide what to GRANT and what to REVOKE:

# defaclacl
(pg_signal_backend,r,$grantor)
...

# acldefault
(pg_signal_backend,a,$grantor)
(pg_signal_backend,r,$grantor)
...

I think that would also make the dump output do the right thing in e.g. v17
pg_dump migrating a v17 origin to a v16 destination. (That's not guaranteed
to work, but we don't offer a better-supported downgrade path.) We could then
re-add the test from commit b61a5c4. There could easily be corner cases I'm
not considering; the strategy may be a dead end.

==== Never emits {REVOKE|GRANT} ALL ON {TABLE|TABLES}

Tactically, that's because v17 parseACLItem() never finds the MAINTAIN bit in
a v16- aclitem. A v16 GRANT ALL ON TABLE becomes a GRANT (all but MAINTAIN)
ON TABLE when migrated to v17. The prior debut of new relation ai_privs bits,
REFERENCES etc., came in v7.2. Unlike MAINTAIN, v7.2 commit 1b68bcfad checked
the v7.2+ bits only for v7.2+ origin servers. A v7.1 aclitem that had every
v7.1-era privilege (arwR = SELECT,UPDATE/DELETE,INSERT,RULE) would also get
v7.2 REFERENCES. Lack of any v7.1-era privilege would mean no REFERENCES.

The v7.2 approach would be suboptimal if a new bit is more powerful than all
existing bits for the object type. For example, FOREIGN SERVER, LANGUAGE,
TYPE, etc. have just USAGE. If v19 were to introduce any other privilege for
those, v18->v19 restore shouldn't emit GRANT ALL for those. (v7.2 was okay,
because v7.1 RULE was roughly as powerful as v7.2 REFERENCES.)

Given the 11 months without complaints, changing the MAINTAIN treatment now is
likely riskier than leaving it as-is. If I had a realized this outcome before
the v17 release, though, I would have recommended the v7.2 approach instead.
(MAINTAIN is strictly less powerful than REFERENCES, so the last paragraph's
caveat does not apply.)

==== Outdated comment in buildACLCommands

(This is not specific to v17+.)

/*
* At this point we have issued REVOKE statements for all initial and
* default privileges that are no longer present on the object, so we are
* almost ready to GRANT the privileges listed in grantitems[].
*
* We still need some hacking though to cover the case where new default
* public privileges are added in new versions: the REVOKE ALL will revoke
* them, leading to behavior different from what the old version had,
* which is generally not what's wanted. So add back default privs if the
* source database is too old to have had that particular priv. (As of
* right now, no such cases exist in supported versions.)
*/

This originated in commit 2ee56b6a3 (2007-01), to handle v8.2 adding privilege
CONNECT ON DATABASE. It's no longer just "new default public privileges" that
could need code here. Any version-dependent "baseacls" (world_default,
owner_default, or pg_init_privs) could entail code here. For example, if
MAINTAIN privilege had used the v7.2 pg_dump approach (see heading "Never
emits {REVOKE|GRANT} ALL ON {TABLE|TABLES}"), then a GRANT MAINTAIN for table
owners would belong here, to compensate for the v17 owner_default change.

The "diffing individual privileges" approach potentially removes the need for
code here.

If anyone seeks more details on the above, let me know.

#20Andres Freund
andres@anarazel.de
In reply to: Noah Misch (#18)
Re: Test instability when pg_dump orders by OID

Hi,

On 2025-08-23 07:45:05 -0700, Noah Misch wrote:

Crossing the boundary of the MAINTAIN privilege existing seems relevant. Will
fix. (My checklist did tell me to do a local run of TestUpgradeXversion. I
skipped it, betting this patch wouldn't break that test. I lost that bet.)

I wonder if it's worth adding support to CI to perform the cross-version
upgrade test. It'd be pretty easy to install all pgdg apt postgres packages to
the debian image, which then could be used as the source version...

Greetings,

Andres Freund

#21Tom Lane
tgl@sss.pgh.pa.us
In reply to: Andres Freund (#20)
Re: Test instability when pg_dump orders by OID

Andres Freund <andres@anarazel.de> writes:

I wonder if it's worth adding support to CI to perform the cross-version
upgrade test. It'd be pretty easy to install all pgdg apt postgres packages to
the debian image, which then could be used as the source version...

I feel that that's the wrong tradeoff. CI should be expected to be
fairly cheap, not to catch everything the buildfarm could catch.

regards, tom lane

#22Noah Misch
noah@leadboat.com
In reply to: Tom Lane (#21)
Re: Test instability when pg_dump orders by OID

On Sun, Aug 24, 2025 at 11:50:01AM -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

I wonder if it's worth adding support to CI to perform the cross-version
upgrade test. It'd be pretty easy to install all pgdg apt postgres packages to
the debian image, which then could be used as the source version...

I think catching this particular case would take more than that. It entails
running the latest v16 src/test/regress suite, capturing the dump of that into
$animal_root/upgrade.$animal/REL_16_STABLE/*.sql, and seeing the upgrade
failure of that dump having the latest v16 regression objects. I don't know
how to get there without a v16 source tree.

I feel that that's the wrong tradeoff. CI should be expected to be
fairly cheap, not to catch everything the buildfarm could catch.

It can always be non-default, like the mingw test.

#23Andres Freund
andres@anarazel.de
In reply to: Noah Misch (#22)
Re: Test instability when pg_dump orders by OID

Hi,

On 2025-08-24 09:08:16 -0700, Noah Misch wrote:

On Sun, Aug 24, 2025 at 11:50:01AM -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

I wonder if it's worth adding support to CI to perform the cross-version
upgrade test. It'd be pretty easy to install all pgdg apt postgres packages to
the debian image, which then could be used as the source version...

I think catching this particular case would take more than that. It entails
running the latest v16 src/test/regress suite, capturing the dump of that into
$animal_root/upgrade.$animal/REL_16_STABLE/*.sql, and seeing the upgrade
failure of that dump having the latest v16 regression objects. I don't know
how to get there without a v16 source tree.

Ah, ok, that does make it less worthwhile to go after.

I feel that that's the wrong tradeoff. CI should be expected to be
fairly cheap, not to catch everything the buildfarm could catch.

I think it's also about removing painful manual testing - and imo manually
running cross-version pg_upgrade tests is really rather painful.

It can always be non-default, like the mingw test.

Indeed. We now have the infrastructure to enable such tests for cfbot while
not running by default in user's repositories (which will commonly be more
compute constrained).

Greetings,

Andres Freund

#24Noah Misch
noah@leadboat.com
In reply to: Andres Freund (#23)
Re: Test instability when pg_dump orders by OID

On Mon, Aug 25, 2025 at 05:15:55PM -0400, Andres Freund wrote:

On 2025-08-24 09:08:16 -0700, Noah Misch wrote:

On Sun, Aug 24, 2025 at 11:50:01AM -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

I wonder if it's worth adding support to CI to perform the cross-version
upgrade test. It'd be pretty easy to install all pgdg apt postgres packages to
the debian image, which then could be used as the source version...

I think catching this particular case would take more than that. It entails
running the latest v16 src/test/regress suite, capturing the dump of that into
$animal_root/upgrade.$animal/REL_16_STABLE/*.sql, and seeing the upgrade
failure of that dump having the latest v16 regression objects. I don't know
how to get there without a v16 source tree.

Ah, ok, that does make it less worthwhile to go after.

I feel that that's the wrong tradeoff. CI should be expected to be
fairly cheap, not to catch everything the buildfarm could catch.

I think it's also about removing painful manual testing - and imo manually
running cross-version pg_upgrade tests is really rather painful.

I make the buildfarm client drive it. That was painful to set up the first
time[1]For example, there's no one OpenSSL version compatible with all of v9.2 - v19. Disabling SSL doesn't solve that: some versions then disable pgcrypto, and the upgrade test fails for pgcrypto being absent on one side of the upgrade. I settled on SSL only for the versions where pgcrypto requires it. Version-dependent LD_LIBRARY_PATH etc. likely would have been an alternative., but the per-run manual pain isn't bad. A run of all supported
branches takes hours of wall time, though. There are some optimization
opportunities, but it hasn't come up often enough to make those compelling for
me to implement.

[1]: For example, there's no one OpenSSL version compatible with all of v9.2 - v19. Disabling SSL doesn't solve that: some versions then disable pgcrypto, and the upgrade test fails for pgcrypto being absent on one side of the upgrade. I settled on SSL only for the versions where pgcrypto requires it. Version-dependent LD_LIBRARY_PATH etc. likely would have been an alternative.
v19. Disabling SSL doesn't solve that: some versions then disable pgcrypto,
and the upgrade test fails for pgcrypto being absent on one side of the
upgrade. I settled on SSL only for the versions where pgcrypto requires it.
Version-dependent LD_LIBRARY_PATH etc. likely would have been an alternative.

#25Tom Lane
tgl@sss.pgh.pa.us
In reply to: Noah Misch (#24)
Re: Test instability when pg_dump orders by OID

Noah Misch <noah@leadboat.com> writes:

On Mon, Aug 25, 2025 at 05:15:55PM -0400, Andres Freund wrote:

I think it's also about removing painful manual testing - and imo manually
running cross-version pg_upgrade tests is really rather painful.

I make the buildfarm client drive it.

Yeah, same here. I have a BF instance lying around that's configured
to do the cross-version pg_upgrade tests. It's not registered with
the BF server, I just run it manually when I need to test that.

regards, tom lane