Date: Thu, 10 Jul 2025 12:31:56 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Nitin Motiani <nitinmotiani@google.com>
Cc: Hannu Krosing <hannuk@google.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: pg_dump sort priority mismatch for large objects
Message-ID: <aG_5DBCjdDX6KAoD@nathan>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Archived-At: <https://www.postgresql.org/message-id/aG_5DBCjdDX6KAoD%40nathan>
Precedence: bulk

(moving to a new thread)

On Thu, Jul 10, 2025 at 06:05:26PM +0530, Nitin Motiani wrote:
>> There might be an existing issue here, because dbObjectTypePriorities
>> has the following comment:
>>
>> * NOTE: object-type priorities must match the section assignments made in
>> * pg_dump.c; that is, PRE_DATA objects must sort before DO_PRE_DATA_BOUNDARY,
>> * POST_DATA objects must sort after DO_POST_DATA_BOUNDARY, and DATA objects
>> * must sort between them.
>>
>> But dumpLO() puts large objects in SECTION_DATA, and PRIO_LARGE_OBJECT
>> is before PRIO_PRE_DATA_BOUNDARY. I admittedly haven't spent too much
>> time investigating this, though.
> 
> I looked through the history of this to see how this happened and if it
> could be an existing issue. Prior to a45c78e3284b, dumpLO used to put large
> objects in SECTION_PRE_DATA. That commit changed dumpLO and also changed
> addBoundaryDependencies to move DO_LARGE_OBJECT from pre-data to data
> section. Seems like since then this has been inconsistent with
> pg_dump_sort.c. I think the change in pg_dump_sort.c should be backported
> to PG17 & 18 independent of the state of the larger patch.

Thanks for doing the archaeology!

> But even with the inconsistency, it doesn't look like there is an existing
> issue. As the dependencies were changed in addBoundaryDependencies, that
> should take precedence over the order in pg_dump_sort.c. The
> dbObjectTypePriorities are used by sortDumpableObjectsByTypeName. But right
> after that sortDumpableObjects sorts the objects based on dependencies
> therefore the change in boundary dependencies should ensure that this is
> working as intended. Still I think dbObjectTypePriorities should be made
> consistent with the rest.

+1, if for no other reason than we'll need it to be below PRIO_TABLE_DATA
for the speed-up-pg_upgrade-with-many-LOs patch [0].  Does anyone see any
problems with applying something like the following down to v17?

diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index 0b0977788f1..538e7dcb493 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -76,10 +76,10 @@ enum dbObjectTypePriorities
     PRIO_TABLE_ATTACH,
     PRIO_DUMMY_TYPE,
     PRIO_ATTRDEF,
-    PRIO_LARGE_OBJECT,
     PRIO_PRE_DATA_BOUNDARY,     /* boundary! */
     PRIO_TABLE_DATA,
     PRIO_SEQUENCE_SET,
+    PRIO_LARGE_OBJECT,
     PRIO_LARGE_OBJECT_DATA,
     PRIO_STATISTICS_DATA_DATA,
     PRIO_POST_DATA_BOUNDARY,    /* boundary! */

[0] https://postgr.es/m/aBkQLSkx1zUJ-LwJ%40nathan

-- 
nathan