Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Hi Ajin,

Thanks -- 0/0 lines up now, and the pg_upgrade.c comment is fixed too. But
`if (IsBinaryUpgrade)` also advances origins that were never tracked.

Old cluster, max_active_replication_origins = 1. The second origin can be
created because it is never tracked:

SELECT pg_replication_origin_create('advanced_one');
SELECT pg_replication_origin_advance('advanced_one', '0/1000');
SELECT pg_replication_origin_create('unadvanced_one');

SELECT * FROM pg_replication_origin;
roident | roname
---------+----------------
1 | advanced_one
2 | unadvanced_one

SELECT * FROM pg_replication_origin_status;
local_id | external_id | remote_lsn | local_lsn
----------+--------------+------------+------------
1 | advanced_one | 0/00001000 | 0/00000000

Upgrading to a new cluster with the same max_active_replication_origins = 1:

Performing Consistency Checks
...
Checking replication origins in new cluster ok
Performing Upgrade
...
Restoring global objects in the new cluster
*failure*

ERROR: could not find free replication state slot for replication origin
with ID 2

The check passed because it counted the one status row; the restore then wanted
two slots. And it gets there after "If pg_upgrade fails after this point, you
must re-initdb the new cluster before continuing.", so a worse landing place
than the refusal I reported on v13.

HEAD guards this in the dumpSubscription() path the patch replaces:

if (subinfo->suboriginremotelsn)

NULL exactly when there was no status row, so HEAD advances iff the origin was
tracked, 0/0 included. dumpReplicationOrigins() still draws the same line --

if (!PQgetisnull(res, i, i_remotelsn))

emits the lsn, and NULL otherwise -- so the distinction survives the dump; it
just gets dropped in the backend, where a missing LSN and a real 0/0 are both
InvalidXLogRecPtr by the time replorigin_create_with_id() sees them. An
untracked origin then starts counting against max_active_replication_origins,
which is what counting status rows was meant to avoid.

The caller still knows which is which -- NULL vs '0/0' -- so passing it down:

void
replorigin_create_with_id(ReplOriginId roident, const char *roname,
- XLogRecPtr remote_lsn, Relation rel)
+ XLogRecPtr remote_lsn, bool need_advance, Relation rel)
@@
- if (IsBinaryUpgrade)
+ if (need_advance)
{
- /* If part of a binary upgrade, advance the origin as well */
+ Assert(IsBinaryUpgrade);
replorigin_advance(roident, remote_lsn, InvalidXLogRecPtr,
@@ replorigin_create()
- replorigin_create_with_id(roident, roname, InvalidXLogRecPtr, rel);
+ replorigin_create_with_id(roident, roname, InvalidXLogRecPtr, false, rel);
@@ binary_upgrade_create_replication_origin()
- replorigin_create_with_id(node, originname, remote_lsn, rel);
+ replorigin_create_with_id(node, originname, remote_lsn,
+ !PG_ARGISNULL(2), rel);

reproduces the old cluster's tracking exactly: the case above upgrades, and an
origin sitting at 0/0 still keeps its status row.

004_subscription.pl already creates a user origin and never advances it, so it
only wanted an assertion that the origin is still untracked afterwards; that
one fails on v14 and passes with the change. Attached is the whole thing, code
and test, on top of v14 -- the pg_upgrade suite and the main regression tests
are green with it.

The other way to close the gap is to leave the advance alone and count all
origins in the check instead. That works too, but it makes pg_upgrade ask for a
max_active_replication_origins the server itself doesn't require -- untracked
origins are free to create today -- so matching the old cluster's tracking
looked like the smaller change.

Thanks,
Rui