Re: pg_upgrade: transfer pg_largeobject_metadata's files when possible

On Tue, Sep 02, 2025 at 09:43:40AM -0500, Nathan Bossart wrote:
> Do you think a new pg_upgrade test for security labels is worth the
> trouble? It seems doable, but it'd be an awfully expensive test for this.
> On the other hand, I'm not sure there's any coverage for pg_upgrade with
> security labels, so perhaps this is a good time to establish some tests.

I would argue in favor of these additions. Security labels are not
the most popular thing ever, AFAIK, but your patch makes the need more
relevant to have. The cheapest approach would be to add a LO creation
pattern in src/bin/pg_dump/t/002_pg_dump.pl, with an EXTRA_INSTALL
pointing at src/test/modules/dummy_seclabel/ to be able to create the
security label (we already do that in pg_upgrade and pg_basebackup so
the trick works). That should be enough to make sure that the binary
upgrade dumps have the seclabel data included. It's a bit funky, I
agree. So if you think that this is not worth the test cycles, I
won't push hard on this point, either.

> No, that stuff is discarded for upgrades from those versions. My intent
> was to maintain readability by avoiding lots of version checks. FWIW I
> originally put all of the pg_large_object_metadata stuff in a separate
> block, but that resulted in a lot of copy/pasted code. I'm happy to adjust
> it as you see fit.

+ if (fout->remoteVersion >= 160000)
+ ArchiveEntry(fout, nilCatalogId, createDumpId(),
+ ARCHIVE_OPTS(.tag = "pg_largeobject_metadata",
+ .description = "pg_largeobject_metadata",
+ .section = SECTION_PRE_DATA,
+ .createStmt = lomOutQry->data));

So it's filtered out depending on the old cluster version here. I
have managed to miss this part. Some tests with LOs and across
multiple backend versions (pg_dump -C --binary-upgrade) are showing me
that I am wrong and that you are right, with diffs showing up properly
in the binary upgrade dumps for pg_largeobject_metadata. This logic
looks OK to me, even if it's a waste to fill lomOutQry when upgrading
from an instance in the 9.3~15 range, discarding ArchiveEntry() at the
end. It's not a big deal.
--
Michael