Subject: Re: pg_upgrade: transfer pg_largeobject_metadata's files when possible

From: Nathan Bossart <[email-masked]>
Date: Wed, 4 Feb 2026 10:06:29 -0600

On Tue, Feb 03, 2026 at 06:46:25PM -0500, Andres Freund wrote:
>> The reason, I think, is that the COPY is happening into a relfilenode that
>> will be overwritten later, it doesn't yet contain the contents of the old
>> cluster.
>> 
>> Presumably we do this because we need the temporary pg_largeobject_metadata to
>> make COMMENT ON and security label commands not fail.
>> 
>> If this is the reasoning / how it works, shouldn't there be a comment in the
>> code or the commit message explaining that? Because it sure seems non-obvious
>> to me.

Right, the COPY for LOs with comments and security labels is solely meant
to avoid failure when restoring the comments and security labels, since we
won't have transferred the relation files yet.  This was the case before
commit 12a53c732c, where we had this comment in getBlobs():

        * We *do* dump out the definition of the blob because we need that to
        * make the restoration of the comments, and anything else, work since
        * pg_upgrade copies the files behind pg_largeobject and
        * pg_largeobject_metadata after the dump is restored.

Commit 3bcfcd815e (mine) added this one to pg_dump.c:

        * If upgrading from v16 or newer, only dump large objects with
        * comments/seclabels.  For these upgrades, pg_upgrade can copy/link
        * pg_largeobject_metadata's files (which is usually faster) but we
        * still need to dump LOs with comments/seclabels here so that the
        * subsequent COMMENT and SECURITY LABEL commands work.  pg_upgrade
        * can't copy/link the files from older versions because aclitem
        * (needed by pg_largeobject_metadata.lomacl) changed its storage
        * format in v16.

IIUC your critique is that this doesn't explain the overwriting behavior
like the older comment does.  I'll work on adding that.

>> It's also not entirely obvious to me that this is safe - after all
>> (bbe08b8869bd, revised in 0e758ae89) appeared to have taken some pains to
>> ensure that the file gets unlinked immediately during the "binary upgrade
>> mode" TRUNCATE. But now we are actually filling that file again, after the
>> relation had been truncated?
> 
> An example of what could go wrong:
>
> [... examples of what could go wrong ...] 

I'm considering a couple of options here, but it seems like the easiest
thing to do is to move the TRUNCATE commands to the end of the dump file.
At least, that seems to be sufficient for our existing tests.  If that
seems okay to you, I can work on putting together a patch.

-- 
nathan

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 4 Feb 2026 14:08:47 -0600

On Wed, Feb 04, 2026 at 10:06:29AM -0600, Nathan Bossart wrote:
> [...]

Here is a rough first draft of a patch that does this.

-- 
nathan
>From c911f5b16132514a461f72b550ee8cd86dfadc1d Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Wed, 4 Feb 2026 14:04:18 -0600
Subject: [PATCH v1 1/1] fix pg_largeobject_metadata file transfer

---
 src/bin/pg_dump/pg_dump.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 2bebefd0ba2..3ba57492fa6 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -379,6 +379,7 @@ static const char *getFormattedTypeName(Archive *fout, Oid oid, OidOptions opts)
 static void getLOs(Archive *fout);
 static void dumpLO(Archive *fout, const LoInfo *loinfo);
 static int	dumpLOs(Archive *fout, const void *arg);
+static void dumpLOTruncation(Archive *fout);
 static void dumpPolicy(Archive *fout, const PolicyInfo *polinfo);
 static void dumpPublication(Archive *fout, const PublicationInfo *pubinfo);
 static void dumpPublicationTable(Archive *fout, const PublicationRelInfo *pubrinfo);
@@ -1157,7 +1158,10 @@ main(int argc, char **argv)
 		 * subsequent COMMENT and SECURITY LABEL commands work.  pg_upgrade
 		 * can't copy/link the files from older versions because aclitem
 		 * (needed by pg_largeobject_metadata.lomacl) changed its storage
-		 * format in v16.
+		 * format in v16.  At the end of the dump, we'll generate a TRUNCATE
+		 * command for pg_largeobject_metadata so that it's contents are
+		 * cleared in preparation for the subsequent file transfer by
+		 * pg_upgrade.
 		 */
 		if (fout->remoteVersion >= 160000)
 			lo_metadata->dataObj->filtercond = "WHERE oid IN "
@@ -1243,6 +1247,13 @@ main(int argc, char **argv)
 	for (i = 0; i < numObjs; i++)
 		dumpDumpableObject(fout, dobjs[i]);
 
+	/*
+	 * For binary upgrades, set relfrozenxids, relminmxids, and relfilenodes
+	 * of pg_largeobject and maybe pg_largeobject_metadata, and remove all
+	 * their files.  We will transfer them from the old cluster as needed.
+	 */
+	dumpLOTruncation(fout);
+
 	/*
 	 * Set up options info to ensure we dump what we want.
 	 */
@@ -3671,6 +3682,20 @@ dumpDatabase(Archive *fout)
 								  .dropStmt = delQry->data,
 								  .deps = &dbDumpId));
 
+	PQclear(res);
+
+	free(qdatname);
+	destroyPQExpBuffer(dbQry);
+	destroyPQExpBuffer(delQry);
+	destroyPQExpBuffer(creaQry);
+	destroyPQExpBuffer(labelq);
+}
+
+static void
+dumpLOTruncation(Archive *fout)
+{
+	DumpOptions *dopt = fout->dopt;
+
 	/*
 	 * pg_largeobject comes from the old system intact, so set its
 	 * relfrozenxids, relminmxids and relfilenode.
@@ -3769,14 +3794,14 @@ dumpDatabase(Archive *fout)
 		ArchiveEntry(fout, nilCatalogId, createDumpId(),
 					 ARCHIVE_OPTS(.tag = "pg_largeobject",
 								  .description = "pg_largeobject",
-								  .section = SECTION_PRE_DATA,
+								  .section = SECTION_POST_DATA,
 								  .createStmt = loOutQry->data));
 
 		if (fout->remoteVersion >= 160000)
 			ArchiveEntry(fout, nilCatalogId, createDumpId(),
 						 ARCHIVE_OPTS(.tag = "pg_largeobject_metadata",
 									  .description = "pg_largeobject_metadata",
-									  .section = SECTION_PRE_DATA,
+									  .section = SECTION_POST_DATA,
 									  .createStmt = lomOutQry->data));
 
 		PQclear(lo_res);
@@ -3787,14 +3812,6 @@ dumpDatabase(Archive *fout)
 		destroyPQExpBuffer(loOutQry);
 		destroyPQExpBuffer(lomOutQry);
 	}
-
-	PQclear(res);
-
-	free(qdatname);
-	destroyPQExpBuffer(dbQry);
-	destroyPQExpBuffer(delQry);
-	destroyPQExpBuffer(creaQry);
-	destroyPQExpBuffer(labelq);
 }
 
 /*
-- 
2.50.1 (Apple Git-155)

---

From: Andres Freund <[email-masked]>
Date: Thu, 5 Feb 2026 11:19:46 -0500

Hi,

> like the older comment does.  I'll work on adding that.

I think even the old comment was woefully under-documenting that the commands
are all just make work that's going to be thrown out almost immediately after.

Thanks for addressing that!

> Here is a rough first draft of a patch that does this.

It certainly seems better than what we do now.  Still feels pretty grotty and
error prone to me that we fill the catalog table and then throw the contents
out.

> +		 * cleared in preparation for the subsequent file transfer by

I'd move that comment to earlier in the paragraph, it sounds a bit like it
applies to the v16 specific bits.

>  	 * Set up options info to ensure we dump what we want.

Seems good to move this to a dedicated function, regardless of anything else.

Do I see correctly that we just rely on the ordering in the file, rather than
dependencies? That's not a complaint, I just don't know that code very well.

Greetings,

Andres Freund

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 5 Feb 2026 11:36:00 -0600

> error prone to me that we fill the catalog table and then throw the contents

Before I go any further with this approach, I thought of something else we
could do that I believe is worth considering...

As of commit 3bcfcd815e, the only reason we are dumping any of
pg_largeobject_metadata at all is to avoid an ERROR during COMMENT ON or
SECURITY LABEL ON because the call to LargeObjectExists() in
get_object_address() returns false.  If we bypass that check in
binary-upgrade mode, we can skip dumping pg_largeobject_metadata entirely.

The attached patch passes our existing tests, and it seems to create the
expected binary-upgrade-mode dump files, too.  I haven't updated any of the
comments yet.

-- 
nathan
>From 1fa1890f6bf98b718118f070226d8b102073e165 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Thu, 5 Feb 2026 11:26:39 -0600
Subject: [PATCH v2 1/1] fix pg_largeobject_metadata file transfer

---
 src/backend/catalog/objectaddress.c |  2 +-
 src/bin/pg_dump/pg_dump.c           | 53 +++++++++++++----------------
 2 files changed, 24 insertions(+), 31 deletions(-)

diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 02af64b82c6..1762146c09a 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -1046,7 +1046,7 @@ get_object_address(ObjectType objtype, Node *object,
 				address.classId = LargeObjectRelationId;
 				address.objectId = oidparse(object);
 				address.objectSubId = 0;
-				if (!LargeObjectExists(address.objectId))
+				if (!LargeObjectExists(address.objectId) && !IsBinaryUpgrade)
 				{
 					if (!missing_ok)
 						ereport(ERROR,
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 2bebefd0ba2..6bcb2f61fe8 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1129,19 +1129,11 @@ main(int argc, char **argv)
 	 */
 	if (dopt.binary_upgrade && fout->remoteVersion >= 120000)
 	{
-		TableInfo  *lo_metadata = findTableByOid(LargeObjectMetadataRelationId);
-		TableInfo  *shdepend = findTableByOid(SharedDependRelationId);
+		TableInfo  *shdepend;
 
-		makeTableDataInfo(&dopt, lo_metadata);
+		shdepend = findTableByOid(SharedDependRelationId);
 		makeTableDataInfo(&dopt, shdepend);
 
-		/*
-		 * Save pg_largeobject_metadata's dump ID for use as a dependency for
-		 * pg_shdepend and any large object comments/seclabels.
-		 */
-		lo_metadata_dumpId = lo_metadata->dataObj->dobj.dumpId;
-		addObjectDependency(&shdepend->dataObj->dobj, lo_metadata_dumpId);
-
 		/*
 		 * Only dump large object shdepend rows for this database.
 		 */
@@ -1149,22 +1141,20 @@ main(int argc, char **argv)
 			"AND dbid = (SELECT oid FROM pg_database "
 			"            WHERE datname = current_database())";
 
-		/*
-		 * If upgrading from v16 or newer, only dump large objects with
-		 * comments/seclabels.  For these upgrades, pg_upgrade can copy/link
-		 * pg_largeobject_metadata's files (which is usually faster) but we
-		 * still need to dump LOs with comments/seclabels here so that the
-		 * subsequent COMMENT and SECURITY LABEL commands work.  pg_upgrade
-		 * can't copy/link the files from older versions because aclitem
-		 * (needed by pg_largeobject_metadata.lomacl) changed its storage
-		 * format in v16.
-		 */
-		if (fout->remoteVersion >= 160000)
-			lo_metadata->dataObj->filtercond = "WHERE oid IN "
-				"(SELECT objoid FROM pg_description "
-				"WHERE classoid = " CppAsString2(LargeObjectRelationId) " "
-				"UNION SELECT objoid FROM pg_seclabel "
-				"WHERE classoid = " CppAsString2(LargeObjectRelationId) ")";
+		if (fout->remoteVersion < 160000)
+		{
+			TableInfo  *lo_metadata;
+
+			lo_metadata = findTableByOid(LargeObjectMetadataRelationId);
+			makeTableDataInfo(&dopt, lo_metadata);
+
+			/*
+			 * Save pg_largeobject_metadata's dump ID for use as a dependency
+			 * for pg_shdepend and any large object comments/seclabels.
+			 */
+			lo_metadata_dumpId = lo_metadata->dataObj->dobj.dumpId;
+			addObjectDependency(&shdepend->dataObj->dobj, lo_metadata_dumpId);
+		}
 	}
 
 	/*
@@ -4079,7 +4069,7 @@ getLOs(Archive *fout)
 				 * We should've saved pg_largeobject_metadata's dump ID before
 				 * this point.
 				 */
-				Assert(lo_metadata_dumpId);
+				Assert(lo_metadata_dumpId || fout->remoteVersion >= 160000);
 
 				loinfo->dobj.dump &= ~(DUMP_COMPONENT_DATA | DUMP_COMPONENT_ACL | DUMP_COMPONENT_DEFINITION);
 
@@ -4088,9 +4078,12 @@ getLOs(Archive *fout)
 				 * pg_largeobject_metadata so that any large object
 				 * comments/seclables are dumped after it.
 				 */
-				loinfo->dobj.dependencies = (DumpId *) pg_malloc(sizeof(DumpId));
-				loinfo->dobj.dependencies[0] = lo_metadata_dumpId;
-				loinfo->dobj.nDeps = loinfo->dobj.allocDeps = 1;
+				if (fout->remoteVersion < 160000)
+				{
+					loinfo->dobj.dependencies = (DumpId *) pg_malloc(sizeof(DumpId));
+					loinfo->dobj.dependencies[0] = lo_metadata_dumpId;
+					loinfo->dobj.nDeps = loinfo->dobj.allocDeps = 1;
+				}
 			}
 			else
 				loinfo->dobj.dump &= ~DUMP_COMPONENT_DATA;
-- 
2.50.1 (Apple Git-155)

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 5 Feb 2026 11:40:50 -0600

> +				if (!LargeObjectExists(address.objectId) && !IsBinaryUpgrade)

(Probably better to set missing_ok for only the specific commands we want
to bypass this check, but you get the idea...)

-- 
nathan