Re: Test instability when pg_dump orders by OID - Mailing list pgsql-hackers

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)