From 2d0d0f3158afdc47e49afa68d87dd4823ad3d32c Mon Sep 17 00:00:00 2001 From: Noah Misch Date: Thu, 3 Sep 2026 18:07:02 +0000 Subject: [PATCH 2/2] SAMPLE fixes for the eleven testable dump-order defects Not proposed patches. These exist so that the branch is coherent -- the tests in the preceding commit need something to pass against -- and so that "this test fails without the fix" is checkable. Drop this commit to see them fail. Four involve a judgement rather than a mechanical key completion: * D5 preserves the order the user wrote in CREATE POLICY, via unnest ... WITH ORDINALITY. Plain ORDER BY rolname would be simpler but rewrites the clause. Both remove the OID dependence. * D4 joins pg_type and pg_namespace and orders by (nspname, typname) rather than by regtype output, whose rendering depends on search_path. * D1 sorts the RLS-enable pseudo-object before its table's policies. Either order is stable. * D12 sorts an ACL array, which buildACLCommands() warns can be unsafe. It is safe here only because a default ACL's items all share one grantor, so there is no grant chain to replay in order; that argument is the basis of the fix and is the thing to check before accepting it. Verified: with these applied, meson test over the pg_dump, test_pg_dump and dummy_seclabel suites is 13/13 (002_pg_dump alone is 13697 subtests). With all five product files reverted, pg_dump aborts on D1's assertion. With only D1's fix applied, each remaining test fails by its own name. This work is model-generated and unreviewed by a human; see PROVENANCE.md. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Newm1jZHVy54kfX1eRPDwB --- src/bin/pg_dump/common.c | 8 +- src/bin/pg_dump/dumputils.c | 8 +- src/bin/pg_dump/pg_dump.c | 155 +++++++++++++++++++++++++++------ src/bin/pg_dump/pg_dump_sort.c | 12 +++ src/bin/pg_dump/pg_dumpall.c | 2 +- 5 files changed, 153 insertions(+), 32 deletions(-) diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c index 047e1c6..38eb274 100644 --- a/src/bin/pg_dump/common.c +++ b/src/bin/pg_dump/common.c @@ -286,10 +286,10 @@ flagInhTables(Archive *fout, TableInfo *tblinfo, int numTables, for (i = 0; i < numInherits; i++) { /* - * Skip a hashtable lookup if it's same table as last time. This is - * unlikely for the child, but less so for the parent. (Maybe we - * should ask the backend for a sorted array to make it more likely? - * Not clear the sorting effort would be repaid, though.) + * Skip a hashtable lookup if it's same table as last time. + * getInherits() sorts by inhrelid, so consecutive rows for the same + * child do come together; repeats of the same parent are less + * predictable. */ if (child == NULL || child->dobj.catId.oid != inhinfo[i].inhrelid) diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c index a3835cc..d7b000f 100644 --- a/src/bin/pg_dump/dumputils.c +++ b/src/bin/pg_dump/dumputils.c @@ -681,10 +681,16 @@ void buildShSecLabelQuery(const char *catalog_name, Oid objectId, PQExpBuffer sql) { + /* + * Sort by provider, the remaining column of pg_shseclabel's unique key + * (classoid and objoid are already fixed by the WHERE clause), so that + * the emitted commands do not depend on physical row order. + */ appendPQExpBuffer(sql, "SELECT provider, label FROM pg_catalog.pg_shseclabel " "WHERE classoid = 'pg_catalog.%s'::pg_catalog.regclass " - "AND objoid = '%u'", catalog_name, objectId); + "AND objoid = '%u' " + "ORDER BY provider", catalog_name, objectId); } /* diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index db14834..436c17d 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -3760,10 +3760,16 @@ dumpDatabaseConfig(Archive *AH, PQExpBuffer outbuf, PQclear(res); - /* Now look for role-and-database-specific options */ + /* + * Now look for role-and-database-specific options. Order by role name, + * so that the emitted commands don't depend on the roles' OIDs; rolname + * is a complete sort key, since pg_db_role_setting has at most one row + * per (setdatabase, setrole). + */ printfPQExpBuffer(buf, "SELECT rolname, unnest(setconfig) " "FROM pg_db_role_setting s, pg_roles r " - "WHERE setrole = r.oid AND setdatabase = '%u'::oid", + "WHERE setrole = r.oid AND setdatabase = '%u'::oid " + "ORDER BY 1", dboid); res = ExecuteSqlQuery(AH, buf->data, PGRES_TUPLES_OK); @@ -4274,9 +4280,20 @@ getPolicies(Archive *fout, TableInfo tblinfo[], int numTables) printfPQExpBuffer(query, "SELECT pol.oid, pol.tableoid, pol.polrelid, pol.polname, pol.polcmd, "); appendPQExpBufferStr(query, "pol.polpermissive, "); + /* + * The role names in the policy's TO clause must come out in the order + * they appear in polroles, which is the order they were written in + * CREATE POLICY. An unordered ARRAY() subquery would instead return them + * in pg_authid scan order, so two databases holding identical policies + * would dump differently whenever their roles occupy different physical + * positions or the planner picks a different scan for pg_authid. + */ appendPQExpBuffer(query, "CASE WHEN pol.polroles = '{0}' THEN NULL ELSE " - " pg_catalog.array_to_string(ARRAY(SELECT pg_catalog.quote_ident(rolname) from pg_catalog.pg_roles WHERE oid = ANY(pol.polroles)), ', ') END AS polroles, " + " pg_catalog.array_to_string(ARRAY(SELECT pg_catalog.quote_ident(r.rolname) " + "FROM pg_catalog.unnest(pol.polroles) WITH ORDINALITY AS u(roleoid, ord) " + "JOIN pg_catalog.pg_roles r ON (r.oid = u.roleoid) " + "ORDER BY u.ord), ', ') END AS polroles, " "pg_catalog.pg_get_expr(pol.polqual, pol.polrelid) AS polqual, " "pg_catalog.pg_get_expr(pol.polwithcheck, pol.polrelid) AS polwithcheck " "FROM unnest('%s'::pg_catalog.oid[]) AS src(tbloid)\n" @@ -4595,10 +4612,22 @@ getPublications(Archive *fout) PGresult *res_tbls; resetPQExpBuffer(query); + /* + * Sort the EXCEPT list by the excluded relations' names. The list + * is a set, and pg_publication_rel has no ordering column, so an + * unordered query would emit it in heap order and make two + * logically-identical databases dump differently. Sorting by + * prrelid would just trade heap order for OID order; use the + * referenced relation's natural key (nspname, relname), matching + * DOTypeNameCompare(). + */ appendPQExpBuffer(query, - "SELECT prrelid\n" - "FROM pg_catalog.pg_publication_rel\n" - "WHERE prpubid = %u AND prexcept", + "SELECT pr.prrelid\n" + "FROM pg_catalog.pg_publication_rel pr\n" + " JOIN pg_catalog.pg_class c ON c.oid = pr.prrelid\n" + " JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace\n" + "WHERE pr.prpubid = %u AND pr.prexcept\n" + "ORDER BY n.nspname, c.relname", pubinfo[i].dobj.catId.oid); res_tbls = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK); @@ -5677,6 +5706,10 @@ dumpSubscription(Archive *fout, const SubscriptionInfo *subinfo) /* * Given a "create query", append as many ALTER ... DEPENDS ON EXTENSION as * the object needs. + * + * The statements are emitted in extension name order, so that the text of the + * object's archive entry is a function of the object's dependencies and not of + * the order in which those dependencies happen to appear in pg_depend. */ static void append_depends_on_extension(Archive *fout, @@ -5704,7 +5737,8 @@ append_depends_on_extension(Archive *fout, "FROM pg_catalog.pg_depend d, pg_catalog.pg_extension e " "WHERE d.refobjid = e.oid AND classid = '%s'::pg_catalog.regclass " "AND objid = '%u'::pg_catalog.oid AND deptype = 'x' " - "AND refclassid = 'pg_catalog.pg_extension'::pg_catalog.regclass", + "AND refclassid = 'pg_catalog.pg_extension'::pg_catalog.regclass " + "ORDER BY e.extname", catalog, dobj->catId.oid); res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK); @@ -7692,8 +7726,18 @@ getInherits(Archive *fout, int *numInherits) int i_inhrelid; int i_inhparent; - /* find all the inheritance information */ - appendPQExpBufferStr(query, "SELECT inhrelid, inhparent FROM pg_inherits"); + /* + * Find all the inheritance information. ORDER BY inhseqno is essential: + * the order of a table's parents is a logical property of the database + * (inhseqno fixes the order of the child's inherited columns), while the + * physical order of pg_inherits rows is not, since a line pointer freed + * by NO INHERIT or DROP TABLE and then reclaimed by VACUUM gets reused by + * a later entry with a higher inhseqno. Sorting by inhrelid as well + * makes the "same table as last time" caching in flagInhTables() work. + */ + appendPQExpBufferStr(query, + "SELECT inhrelid, inhparent FROM pg_inherits " + "ORDER BY inhrelid, inhseqno"); res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK); @@ -10570,12 +10614,26 @@ getDefaultACLs(Archive *fout) * for the case of 'S' (DEFACLOBJ_SEQUENCE) which must be converted to * 's'. */ + /* + * The stored element order of defaclacl carries no information: the + * backend canonicalizes these arrays with aclitemsort(), which orders them + * by grantee OID. Dumping in that order would make our output depend on + * OID assignment, so re-sort by the aclitem's textual form, i.e. by + * grantee name. Unlike an object's own ACL, a default ACL cannot contain + * a chain of grants by different grantors -- every item's grantor is + * defaclrole -- so reordering is safe here. + */ appendPQExpBufferStr(query, "SELECT oid, tableoid, " "defaclrole, " "defaclnamespace, " "defaclobjtype, " - "defaclacl, " + "CASE WHEN pg_catalog.array_length(defaclacl, 1) IS NULL " + "THEN defaclacl ELSE " + "(SELECT pg_catalog.array_agg(a ORDER BY " + "a::pg_catalog.text COLLATE pg_catalog.\"C\") " + "FROM pg_catalog.unnest(defaclacl) AS a) " + "END AS defaclacl, " "CASE WHEN defaclnamespace = 0 THEN " "acldefault(CASE WHEN defaclobjtype = 'S' " "THEN 's'::\"char\" ELSE defaclobjtype END, " @@ -11954,6 +12012,7 @@ dumpExtension(Archive *fout, const ExtensionInfo *extinfo) */ int i; int n; + char **reqexts; appendPQExpBufferStr(q, "-- For binary upgrade, create an empty extension and insert objects into it\n"); @@ -11989,7 +12048,14 @@ dumpExtension(Archive *fout, const ExtensionInfo *extinfo) else appendPQExpBufferStr(q, "NULL"); appendPQExpBufferStr(q, ", "); - appendPQExpBufferStr(q, "ARRAY["); + /* + * Collect the names of the extensions this one requires. The + * dependency array is in the order getDependencies() read the + * pg_depend rows, which is a function of the required extensions' + * OIDs; sort the names so that the output depends only on the + * database's logical content. + */ + reqexts = (char **) pg_malloc(extinfo->dobj.nDeps * sizeof(char *)); n = 0; for (i = 0; i < extinfo->dobj.nDeps; i++) { @@ -11997,14 +12063,20 @@ dumpExtension(Archive *fout, const ExtensionInfo *extinfo) extobj = findObjectByDumpId(extinfo->dobj.dependencies[i]); if (extobj && extobj->objType == DO_EXTENSION) - { - if (n++ > 0) - appendPQExpBufferChar(q, ','); - appendStringLiteralAH(q, extobj->name, fout); - } + reqexts[n++] = extobj->name; + } + qsort(reqexts, n, sizeof(char *), pg_qsort_strcmp); + + appendPQExpBufferStr(q, "ARRAY["); + for (i = 0; i < n; i++) + { + if (i > 0) + appendPQExpBufferChar(q, ','); + appendStringLiteralAH(q, reqexts[i], fout); } appendPQExpBufferStr(q, "]::pg_catalog.text[]"); appendPQExpBufferStr(q, ");\n"); + pg_free(reqexts); } if (extinfo->dobj.dump & DUMP_COMPONENT_DEFINITION) @@ -14577,15 +14649,20 @@ dumpOpclass(Archive *fout, const OpclassInfo *opcinfo) appendPQExpBuffer(query, "SELECT amopstrategy, " "amopopr::pg_catalog.regoperator, " "opfname AS sortfamily, " - "nspname AS sortfamilynsp " + "n.nspname AS sortfamilynsp " "FROM pg_catalog.pg_amop ao JOIN pg_catalog.pg_depend ON " "(classid = 'pg_catalog.pg_amop'::pg_catalog.regclass AND objid = ao.oid) " "LEFT JOIN pg_catalog.pg_opfamily f ON f.oid = amopsortfamily " "LEFT JOIN pg_catalog.pg_namespace n ON n.oid = opfnamespace " + "JOIN pg_catalog.pg_type lt ON lt.oid = ao.amoplefttype " + "JOIN pg_catalog.pg_namespace ln ON ln.oid = lt.typnamespace " + "JOIN pg_catalog.pg_type rt ON rt.oid = ao.amoprighttype " + "JOIN pg_catalog.pg_namespace rn ON rn.oid = rt.typnamespace " "WHERE refclassid = 'pg_catalog.pg_opclass'::pg_catalog.regclass " "AND refobjid = '%u'::pg_catalog.oid " "AND amopfamily = '%s'::pg_catalog.oid " - "ORDER BY amopstrategy", + "ORDER BY amopstrategy, ln.nspname, lt.typname, " + "rn.nspname, rt.typname", opcinfo->dobj.catId.oid, opcfamily); @@ -14639,12 +14716,19 @@ dumpOpclass(Archive *fout, const OpclassInfo *opcinfo) "amproc::pg_catalog.regprocedure, " "amproclefttype::pg_catalog.regtype, " "amprocrighttype::pg_catalog.regtype " - "FROM pg_catalog.pg_amproc ap, pg_catalog.pg_depend " + "FROM pg_catalog.pg_amproc ap, pg_catalog.pg_depend, " + "pg_catalog.pg_type lt, pg_catalog.pg_namespace ln, " + "pg_catalog.pg_type rt, pg_catalog.pg_namespace rn " "WHERE refclassid = 'pg_catalog.pg_opclass'::pg_catalog.regclass " "AND refobjid = '%u'::pg_catalog.oid " "AND classid = 'pg_catalog.pg_amproc'::pg_catalog.regclass " "AND objid = ap.oid " - "ORDER BY amprocnum", + "AND lt.oid = ap.amproclefttype " + "AND ln.oid = lt.typnamespace " + "AND rt.oid = ap.amprocrighttype " + "AND rn.oid = rt.typnamespace " + "ORDER BY amprocnum, ln.nspname, lt.typname, " + "rn.nspname, rt.typname", opcinfo->dobj.catId.oid); res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK); @@ -14779,15 +14863,20 @@ dumpOpfamily(Archive *fout, const OpfamilyInfo *opfinfo) appendPQExpBuffer(query, "SELECT amopstrategy, " "amopopr::pg_catalog.regoperator, " "opfname AS sortfamily, " - "nspname AS sortfamilynsp " + "n.nspname AS sortfamilynsp " "FROM pg_catalog.pg_amop ao JOIN pg_catalog.pg_depend ON " "(classid = 'pg_catalog.pg_amop'::pg_catalog.regclass AND objid = ao.oid) " "LEFT JOIN pg_catalog.pg_opfamily f ON f.oid = amopsortfamily " "LEFT JOIN pg_catalog.pg_namespace n ON n.oid = opfnamespace " + "JOIN pg_catalog.pg_type lt ON lt.oid = ao.amoplefttype " + "JOIN pg_catalog.pg_namespace ln ON ln.oid = lt.typnamespace " + "JOIN pg_catalog.pg_type rt ON rt.oid = ao.amoprighttype " + "JOIN pg_catalog.pg_namespace rn ON rn.oid = rt.typnamespace " "WHERE refclassid = 'pg_catalog.pg_opfamily'::pg_catalog.regclass " "AND refobjid = '%u'::pg_catalog.oid " "AND amopfamily = '%u'::pg_catalog.oid " - "ORDER BY amopstrategy", + "ORDER BY amopstrategy, ln.nspname, lt.typname, " + "rn.nspname, rt.typname", opfinfo->dobj.catId.oid, opfinfo->dobj.catId.oid); @@ -14799,12 +14888,19 @@ dumpOpfamily(Archive *fout, const OpfamilyInfo *opfinfo) "amproc::pg_catalog.regprocedure, " "amproclefttype::pg_catalog.regtype, " "amprocrighttype::pg_catalog.regtype " - "FROM pg_catalog.pg_amproc ap, pg_catalog.pg_depend " + "FROM pg_catalog.pg_amproc ap, pg_catalog.pg_depend, " + "pg_catalog.pg_type lt, pg_catalog.pg_namespace ln, " + "pg_catalog.pg_type rt, pg_catalog.pg_namespace rn " "WHERE refclassid = 'pg_catalog.pg_opfamily'::pg_catalog.regclass " "AND refobjid = '%u'::pg_catalog.oid " "AND classid = 'pg_catalog.pg_amproc'::pg_catalog.regclass " "AND objid = ap.oid " - "ORDER BY amprocnum", + "AND lt.oid = ap.amproclefttype " + "AND ln.oid = lt.typnamespace " + "AND rt.oid = ap.amprocrighttype " + "AND rn.oid = rt.typnamespace " + "ORDER BY amprocnum, ln.nspname, lt.typname, " + "rn.nspname, rt.typname", opfinfo->dobj.catId.oid); res_procs = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK); @@ -16731,7 +16827,8 @@ findSecLabels(Oid classoid, Oid objoid, SecLabelItem **items) * Construct a table of all security labels available for database objects; * also set the has-seclabel component flag for each relevant object. * - * The table is sorted by classoid/objid/objsubid for speed in lookup. + * The table is sorted by classoid/objid/objsubid/provider for speed in + * lookup. */ static void collectSecLabels(Archive *fout) @@ -16749,10 +16846,16 @@ collectSecLabels(Archive *fout) query = createPQExpBuffer(); + /* + * Sort by provider as well. It is the remaining column of pg_seclabel's + * unique key, so adding it makes the ordering total; without it, the + * order of the labels an object has from different providers would come + * from physical row order, making the dump unstable. + */ appendPQExpBufferStr(query, "SELECT label, provider, classoid, objoid, objsubid " "FROM pg_catalog.pg_seclabels " - "ORDER BY classoid, objoid, objsubid"); + "ORDER BY classoid, objoid, objsubid, provider"); res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK); diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c index 8ca0332..38c3a1c 100644 --- a/src/bin/pg_dump/pg_dump_sort.c +++ b/src/bin/pg_dump/pg_dump_sort.c @@ -394,6 +394,18 @@ DOTypeNameCompare(const void *p1, const void *p2) pobj2->poltable->dobj.name); if (cmpval != 0) return cmpval; + + /* + * getPolicies() represents "RLS is enabled on this table" as a + * PolicyInfo with null polname whose dobj.name is the table's relname. + * Policy names live in a per-table namespace disjoint from relation + * names, so such a marker ties with a real policy of that same name on + * that same table; whether polname is null is then the only remaining + * natural-key field. Sort the marker first. + */ + cmpval = (pobj1->polname != NULL) - (pobj2->polname != NULL); + if (cmpval != 0) + return cmpval; } else if (obj1->objType == DO_RULE) { diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c index c53e77c..d867e9e 100644 --- a/src/bin/pg_dump/pg_dumpall.c +++ b/src/bin/pg_dump/pg_dumpall.c @@ -1373,7 +1373,7 @@ dumpTablespaces(PGconn *conn) "pg_catalog.shobj_description(oid, 'pg_tablespace') " "FROM pg_catalog.pg_tablespace " "WHERE spcname !~ '^pg_' " - "ORDER BY 1"); + "ORDER BY 2"); if (PQntuples(res) > 0) fprintf(OPF, "--\n-- Tablespaces\n--\n\n"); -- 2.49.0