Re: pg_upgrade: transfer pg_largeobject_metadata's files when possible

Hi,

On 2026-02-11 15:00:51 -0600, Nathan Bossart wrote:
> On Sun, Feb 08, 2026 at 04:00:40PM -0600, Nathan Bossart wrote:
> > IIRC the issue is that getTableAttrs() won't pick up the OID column on
> > older versions.  It might be easy to fix that by adjusting its query for
> > binary upgrades from <v12.  That could be worth doing, if for no other
> > reason than to simplify some of the pg_dump code.  I'll make a note of it.
> 
> This was a little more painful than I expected, but this seems to be what
> is required to allow COPY-ing pg_largeobject_metadata during binary
> upgrades from < v12.

Nice!


> @@ -2406,11 +2404,14 @@ dumpTableData_copy(Archive *fout, const void *dcontext)
>  	column_list = fmtCopyColumnList(tbinfo, clistBuf);
>  
>  	/*
> -	 * Use COPY (SELECT ...) TO when dumping a foreign table's data, and when
> -	 * a filter condition was specified.  For other cases a simple COPY
> -	 * suffices.
> +	 * Use COPY (SELECT ...) TO when dumping a foreign table's data, when a
> +	 * filter condition was specified, and when in binary upgrade mode and
> +	 * dumping an old pg_largeobject_metadata defined WITH OIDS.  For other
> +	 * cases a simple COPY suffices.
>  	 */
> -	if (tdinfo->filtercond || tbinfo->relkind == RELKIND_FOREIGN_TABLE)
> +	if (tdinfo->filtercond || tbinfo->relkind == RELKIND_FOREIGN_TABLE ||
> +		(fout->dopt->binary_upgrade && fout->remoteVersion < 120000 &&
> +		 tbinfo->dobj.catId.oid == LargeObjectMetadataRelationId))
>  	{
>  		/* Temporary allows to access to foreign tables to dump data */
>  		if (tbinfo->relkind == RELKIND_FOREIGN_TABLE)

Not really the fault of this patch, but it seems somewhat grotty that this has
binary upgrade specific code in this place.  I was certainly confused when
first trying to use pg_dump in binary upgrade mode with large objects, because
no data was dumped when using plain text mode, which is what I had been using
for simplicity...


> @@ -2406,11 +2404,14 @@ dumpTableData_copy(Archive *fout, const void *dcontext)
>  	column_list = fmtCopyColumnList(tbinfo, clistBuf);
>  
>  	/*
> -	 * Use COPY (SELECT ...) TO when dumping a foreign table's data, and when
> -	 * a filter condition was specified.  For other cases a simple COPY
> -	 * suffices.
> +	 * Use COPY (SELECT ...) TO when dumping a foreign table's data, when a
> +	 * filter condition was specified, and when in binary upgrade mode and
> +	 * dumping an old pg_largeobject_metadata defined WITH OIDS.  For other
> +	 * cases a simple COPY suffices.
>  	 */
> -	if (tdinfo->filtercond || tbinfo->relkind == RELKIND_FOREIGN_TABLE)
> +	if (tdinfo->filtercond || tbinfo->relkind == RELKIND_FOREIGN_TABLE ||
> +		(fout->dopt->binary_upgrade && fout->remoteVersion < 120000 &&
> +		 tbinfo->dobj.catId.oid == LargeObjectMetadataRelationId))
>  	{
>  		/* Temporary allows to access to foreign tables to dump data */
>  		if (tbinfo->relkind == RELKIND_FOREIGN_TABLE)

I guess you could instead generate a COPY using WITH OIDS. But it's probably
not worth having that path, given we already need to support COPY (SELECT ..).


OTOH, I think it'd perhaps avoid needing to deal with this:

> @@ -9442,7 +9428,18 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
>  							 "(pt.classoid = co.tableoid AND pt.objoid = co.oid)\n");
>  
>  	appendPQExpBufferStr(q,
> -						 "WHERE a.attnum > 0::pg_catalog.int2\n"
> +						 "WHERE a.attnum > 0::pg_catalog.int2\n");
> +
> +	/*
> +	 * For binary upgrades from <v12, be sure to pick up
> +	 * pg_largeobject_metadata's oid column.
> +	 */
> +	if (fout->dopt->binary_upgrade && fout->remoteVersion < 120000)
> +		appendPQExpBufferStr(q,
> +							 "OR (a.attnum = -2::pg_catalog.int2 AND src.tbloid = "
> +							 CppAsString2(LargeObjectMetadataRelationId) ")\n");
> +

as we'd just include the oid column without needing to somehow include it in
the attribute list.


> @@ -9544,7 +9541,9 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
>  
>  		for (int j = 0; j < numatts; j++, r++)
>  		{
> -			if (j + 1 != atoi(PQgetvalue(res, r, i_attnum)))
> +			if (j + 1 != atoi(PQgetvalue(res, r, i_attnum)) &&
> +				!(fout->dopt->binary_upgrade && fout->remoteVersion < 120000 &&
> +				  tbinfo->dobj.catId.oid == LargeObjectMetadataRelationId))
>  				pg_fatal("invalid column numbering in table \"%s\"",
>  						 tbinfo->dobj.name);
>  			tbinfo->attnames[j] = pg_strdup(PQgetvalue(res, r, i_attname));
> -- 
> 2.50.1 (Apple Git-155)

I guess WITH OIDs also would avoid the need for this.

Greetings,

Andres Freund