public inbox for [email protected]help / color / mirror / Atom feed
Re: [PATCH] Preserve replication origin OIDs in pg_upgrade 4+ messages / 3 participants [nested] [flat]
* Re: [PATCH] Preserve replication origin OIDs in pg_upgrade @ 2026-05-18 10:42 Ajin Cherian <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Ajin Cherian @ 2026-05-18 10:42 UTC (permalink / raw) To: Zsolt Parragi <[email protected]>; +Cc: vignesh C <[email protected]>; Hayato Kuroda (Fujitsu) <[email protected]>; PostgreSQL Hackers <[email protected]> Rebased the patch as it was no longer applying. regards, Ajin Cherian Fujitsu Australia Attachments: [application/octet-stream] v6-0002-Preserve-replication-origin-OIDs-during-pg_upgrad.patch (22.0K, 2-v6-0002-Preserve-replication-origin-OIDs-during-pg_upgrad.patch) download | inline diff: From c03447597e366d13fee22b288a822a66233329a7 Mon Sep 17 00:00:00 2001 From: Ajin Cherian <[email protected]> Date: Mon, 18 May 2026 20:34:44 +1000 Subject: [PATCH v6 2/2] Preserve replication origin OIDs during pg_upgrade When pg_upgrade migrates a subscriber, replication origin OIDs (roident) can change across the upgrade. This is a problem because commit-timestamp records embed roident and are copied directly from the old cluster's pg_commit_ts directory, causing spurious "update_origin_differs" conflicts after the upgrade. Fix this by dumping replication origins as global objects via pg_dumpall during binary upgrade, using a new function binary_upgrade_create_replication_origin(oid, name, lsn) to recreate each origin with its preserved roident and remote_lsn. To avoid conflicts with this, CreateSubscription() skips replorigin_create() in binary-upgrade mode since the origin is already created by the time the subscription is restored. Author: Ajin Cherian <[email protected]> Reviewer: Hayato Kuroda (Fujitsu) <[email protected]> Reviewer: Zsolt Parragi <[email protected]> --- src/backend/commands/subscriptioncmds.c | 11 +- src/backend/utils/adt/pg_upgrade_support.c | 159 +++++++++++++++------ src/bin/pg_dump/pg_dump.c | 38 ++--- src/bin/pg_dump/pg_dumpall.c | 64 +++++++++ src/bin/pg_upgrade/check.c | 13 +- src/bin/pg_upgrade/info.c | 9 ++ src/bin/pg_upgrade/pg_upgrade.h | 1 + src/bin/pg_upgrade/t/004_subscription.pl | 42 +++++- src/include/catalog/pg_proc.dat | 8 +- 9 files changed, 253 insertions(+), 92 deletions(-) diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c index 7c1f05a5fd5..b31e15256a2 100644 --- a/src/backend/commands/subscriptioncmds.c +++ b/src/backend/commands/subscriptioncmds.c @@ -19,6 +19,7 @@ #include "access/table.h" #include "access/twophase.h" #include "access/xact.h" +#include "catalog/binary_upgrade.h" #include "catalog/catalog.h" #include "catalog/dependency.h" #include "catalog/indexing.h" @@ -867,9 +868,15 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt, * apply workers initialization, and to handle origin creation dynamically * when tables are added to the subscription. It is not clear whether * preventing creation of origins is worth additional complexity. + * + * In binary-upgrade mode, skip origin creation here. This is required to + * preserve the roident from the old cluster for this subscription. */ - ReplicationOriginNameForLogicalRep(subid, InvalidOid, originname, sizeof(originname)); - replorigin_create(originname); + if (!IsBinaryUpgrade) + { + ReplicationOriginNameForLogicalRep(subid, InvalidOid, originname, sizeof(originname)); + replorigin_create(originname); + } /* * Connect to remote side to execute requested commands and fetch table diff --git a/src/backend/utils/adt/pg_upgrade_support.c b/src/backend/utils/adt/pg_upgrade_support.c index 59c3e7f0146..41fde3f09fc 100644 --- a/src/backend/utils/adt/pg_upgrade_support.c +++ b/src/backend/utils/adt/pg_upgrade_support.c @@ -11,10 +11,13 @@ #include "postgres.h" +#include "access/genam.h" #include "access/relation.h" +#include "access/skey.h" #include "access/table.h" #include "catalog/binary_upgrade.h" #include "catalog/heap.h" +#include "catalog/indexing.h" #include "catalog/namespace.h" #include "catalog/pg_subscription_rel.h" #include "catalog/pg_type.h" @@ -27,8 +30,10 @@ #include "storage/lmgr.h" #include "utils/array.h" #include "utils/builtins.h" +#include "utils/fmgroids.h" #include "utils/lsyscache.h" #include "utils/pg_lsn.h" +#include "utils/snapmgr.h" #define CHECK_IS_BINARY_UPGRADE \ @@ -377,71 +382,133 @@ binary_upgrade_add_sub_rel_state(PG_FUNCTION_ARGS) } /* - * binary_upgrade_replorigin_advance + * binary_upgrade_create_conflict_detection_slot * - * Update the remote_lsn for the subscriber's replication origin. + * Create a replication slot to retain information necessary for conflict + * detection such as dead tuples, commit timestamps, and origins. */ Datum -binary_upgrade_replorigin_advance(PG_FUNCTION_ARGS) +binary_upgrade_create_conflict_detection_slot(PG_FUNCTION_ARGS) { - Relation rel; - Oid subid; - char *subname; - char originname[NAMEDATALEN]; - ReplOriginId node; - XLogRecPtr remote_commit; - CHECK_IS_BINARY_UPGRADE; - /* - * We must ensure a non-NULL subscription name before dereferencing the - * arguments. - */ - if (PG_ARGISNULL(0)) - elog(ERROR, "null argument to binary_upgrade_replorigin_advance is not allowed"); - - subname = text_to_cstring(PG_GETARG_TEXT_PP(0)); - remote_commit = PG_ARGISNULL(1) ? InvalidXLogRecPtr : PG_GETARG_LSN(1); - - rel = table_open(SubscriptionRelationId, RowExclusiveLock); - subid = get_subscription_oid(subname, false); - - ReplicationOriginNameForLogicalRep(subid, InvalidOid, originname, sizeof(originname)); - - /* Lock to prevent the replication origin from vanishing */ - LockRelationOid(ReplicationOriginRelationId, RowExclusiveLock); - node = replorigin_by_name(originname, false); - - /* - * The server will be stopped after setting up the objects in the new - * cluster and the origins will be flushed during the shutdown checkpoint. - * This will ensure that the latest LSN values for origin will be - * available after the upgrade. - */ - replorigin_advance(node, remote_commit, InvalidXLogRecPtr, - false /* backward */ , - false /* WAL log */ ); + CreateConflictDetectionSlot(); - UnlockRelationOid(ReplicationOriginRelationId, RowExclusiveLock); - table_close(rel, RowExclusiveLock); + ReplicationSlotRelease(); PG_RETURN_VOID(); } /* - * binary_upgrade_create_conflict_detection_slot + * binary_upgrade_create_replication_origin * - * Create a replication slot to retain information necessary for conflict - * detection such as dead tuples, commit timestamps, and origins. + * Create a replication origin with a specific OID and name, optionally + * restoring its remote_lsn. Used by pg_upgrade to preserve replication + * origin OIDs across the upgrade. */ Datum -binary_upgrade_create_conflict_detection_slot(PG_FUNCTION_ARGS) +binary_upgrade_create_replication_origin(PG_FUNCTION_ARGS) { + Oid node_oid; + ReplOriginId node; + Name originname; + Relation rel; + HeapTuple tuple; + Datum roname_d; + SysScanDesc scan; + ScanKeyData key; + bool nulls[Natts_pg_replication_origin]; + Datum values[Natts_pg_replication_origin]; + bool collides; + CHECK_IS_BINARY_UPGRADE; - CreateConflictDetectionSlot(); + if (PG_ARGISNULL(0) || PG_ARGISNULL(1)) + elog(ERROR, + "null argument to binary_upgrade_create_replication_origin is not allowed"); + + node_oid = PG_GETARG_OID(0); + + if (node_oid == InvalidOid || node_oid > PG_UINT16_MAX) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("replication origin ID %u is out of range", node_oid))); + + node = (ReplOriginId) node_oid; + originname = PG_GETARG_NAME(1); + + if (strlen(NameStr(*originname)) > MAX_RONAME_LEN) + ereport(ERROR, + (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), + errmsg("replication origin name is too long"), + errdetail("Replication origin names must be no longer than %d bytes.", + MAX_RONAME_LEN))); + + roname_d = CStringGetTextDatum(NameStr(*originname)); + + Assert(IsTransactionState()); + + rel = table_open(ReplicationOriginRelationId, RowExclusiveLock); + + Assert(!OidIsValid(rel->rd_rel->reltoastrelid)); + + /* Check for OID collision */ + ScanKeyInit(&key, + Anum_pg_replication_origin_roident, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(node)); + scan = systable_beginscan(rel, ReplicationOriginIdentIndex, + true /* indexOK */, + SnapshotSelf, + 1, &key); + collides = HeapTupleIsValid(systable_getnext(scan)); + systable_endscan(scan); + + if (collides) + ereport(ERROR, + (errcode(ERRCODE_DUPLICATE_OBJECT), + errmsg("replication origin with ID %u already exists", node_oid))); + + /* Check for name collision */ + ScanKeyInit(&key, + Anum_pg_replication_origin_roname, + BTEqualStrategyNumber, F_TEXTEQ, + roname_d); + scan = systable_beginscan(rel, ReplicationOriginNameIndex, + true /* indexOK */, + SnapshotSelf, + 1, &key); + collides = HeapTupleIsValid(systable_getnext(scan)); + systable_endscan(scan); + + if (collides) + ereport(ERROR, + (errcode(ERRCODE_DUPLICATE_OBJECT), + errmsg("replication origin \"%s\" already exists", + NameStr(*originname)))); + + memset(&nulls, 0, sizeof(nulls)); + memset(&values, 0, sizeof(values)); + + values[Anum_pg_replication_origin_roident - 1] = ObjectIdGetDatum(node); + values[Anum_pg_replication_origin_roname - 1] = roname_d; + + tuple = heap_form_tuple(RelationGetDescr(rel), values, nulls); + CatalogTupleInsert(rel, tuple); + heap_freetuple(tuple); + CommandCounterIncrement(); + + /* Restore the remote_lsn if provided, while still holding the lock */ + if (!PG_ARGISNULL(2)) + { + XLogRecPtr remote_commit = PG_GETARG_LSN(2); - ReplicationSlotRelease(); + replorigin_advance(node, remote_commit, InvalidXLogRecPtr, + false /* backward */, + false /* WAL log */); + } + + table_close(rel, RowExclusiveLock); PG_RETURN_VOID(); } diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 452d0b5e98a..a5fb2b42c3d 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -5668,37 +5668,15 @@ dumpSubscription(Archive *fout, const SubscriptionInfo *subinfo) * In binary-upgrade mode, we allow the replication to continue after the * upgrade. */ - if (dopt->binary_upgrade && fout->remoteVersion >= 170000) + if (dopt->binary_upgrade && subinfo->subenabled && fout->remoteVersion >= 170000) { - if (subinfo->suboriginremotelsn) - { - /* - * Preserve the remote_lsn for the subscriber's replication - * origin. This value is required to start the replication from - * the position before the upgrade. This value will be stale if - * the publisher gets upgraded before the subscriber node. - * However, this shouldn't be a problem as the upgrade of the - * publisher ensures that all the transactions were replicated - * before upgrading it. - */ - appendPQExpBufferStr(query, - "\n-- For binary upgrade, must preserve the remote_lsn for the subscriber's replication origin.\n"); - appendPQExpBufferStr(query, - "SELECT pg_catalog.binary_upgrade_replorigin_advance("); - appendStringLiteralAH(query, subinfo->dobj.name, fout); - appendPQExpBuffer(query, ", '%s');\n", subinfo->suboriginremotelsn); - } - - if (subinfo->subenabled) - { - /* - * Enable the subscription to allow the replication to continue - * after the upgrade. - */ - appendPQExpBufferStr(query, - "\n-- For binary upgrade, must preserve the subscriber's running state.\n"); - appendPQExpBuffer(query, "ALTER SUBSCRIPTION %s ENABLE;\n", qsubname); - } + /* + * Enable the subscription to allow the replication to continue + * after the upgrade. + */ + appendPQExpBufferStr(query, + "\n-- For binary upgrade, must preserve the subscriber's running state.\n"); + appendPQExpBuffer(query, "ALTER SUBSCRIPTION %s ENABLE;\n", qsubname); } if (subinfo->dobj.dump & DUMP_COMPONENT_DEFINITION) diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c index c1f43113c53..e16918feccf 100644 --- a/src/bin/pg_dump/pg_dumpall.c +++ b/src/bin/pg_dump/pg_dumpall.c @@ -25,6 +25,7 @@ #include <time.h> #include <unistd.h> +#include "access/xlogdefs.h" #include "catalog/pg_authid_d.h" #include "common/connect.h" #include "common/file_perm.h" @@ -76,6 +77,7 @@ static void dropDBs(PGconn *conn); static void dumpUserConfig(PGconn *conn, const char *username); static void dumpDatabases(PGconn *conn); static void dumpTimestamp(const char *msg); +static void dumpReplicationOrigins(PGconn *conn); static int runPgDump(const char *dbname, const char *create_opts, char *dbfile); static void buildShSecLabels(PGconn *conn, const char *catalog_name, Oid objectId, @@ -813,6 +815,10 @@ main(int argc, char *argv[]) /* Dump role GUC privileges */ if (server_version >= 150000 && !skip_acls) dumpRoleGUCPrivs(conn); + + /* Dump replication origins */ + if (server_version >= 170000 && binary_upgrade && archDumpFormat == archNull) + dumpReplicationOrigins(conn); } /* Dump tablespaces */ @@ -2339,6 +2345,64 @@ dumpTimestamp(const char *msg) fprintf(OPF, "-- %s %s\n\n", msg, buf); } +static void +dumpReplicationOrigins(PGconn *conn) +{ + PQExpBuffer buf = createPQExpBuffer(); + PGresult *res; + int i_roident; + int i_roname; + int i_remotelsn; + + /* Get replication origins from catalogs */ + appendPQExpBufferStr(buf, + "SELECT o.*, os.remote_lsn " + "FROM pg_catalog.pg_replication_origin o " + "LEFT OUTER JOIN pg_catalog.pg_replication_origin_status os ON o.roident = os.local_id "); + + res = executeQuery(conn, buf->data); + + i_roident = PQfnumber(res, "roident"); + i_roname = PQfnumber(res, "roname"); + i_remotelsn = PQfnumber(res, "remote_lsn"); + + if (PQntuples(res) > 0) + fprintf(OPF, "--\n-- Replication Origins \n--\n\n"); + + for (int i = 0; i < PQntuples(res); i++) + { + ReplOriginId roident; + const char *roname; + + roident = atooid(PQgetvalue(res, i, i_roident)); + roname = PQgetvalue(res, i, i_roname); + + resetPQExpBuffer(buf); + + appendPQExpBufferStr(buf, "\n-- For binary upgrade, must preserve replication origin roident and remote_lsn\n"); + appendPQExpBuffer(buf, + "SELECT pg_catalog.binary_upgrade_create_replication_origin(" + "'%u'::pg_catalog.oid, ", roident); + appendStringLiteralConn(buf, roname, conn); + appendPQExpBufferStr(buf, "::pg_catalog.name"); + + if (!PQgetisnull(res, i, i_remotelsn)) + { + appendPQExpBufferStr(buf, ", "); + appendStringLiteralConn(buf, PQgetvalue(res, i, i_remotelsn), conn); + appendPQExpBufferStr(buf, "::pg_catalog.pg_lsn"); + } + else + appendPQExpBufferStr(buf, ", NULL"); + + appendPQExpBufferStr(buf, ");\n"); + fprintf(OPF, "%s", buf->data); + } + + PQclear(res); + destroyPQExpBuffer(buf); +} + /* * read_dumpall_filters - retrieve database identifier patterns from file * diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c index f5c93e611d2..a3c027c7cd0 100644 --- a/src/bin/pg_upgrade/check.c +++ b/src/bin/pg_upgrade/check.c @@ -2306,8 +2306,7 @@ check_new_cluster_replication_slots(void) * check_new_cluster_subscription_configuration() * * Verify that the max_active_replication_origins configuration specified is - * enough for creating the subscriptions. This is required to create the - * replication origin for each subscription. + * enough for creating all the replication origins. */ static void check_new_cluster_subscription_configuration(void) @@ -2320,8 +2319,8 @@ check_new_cluster_subscription_configuration(void) if (GET_MAJOR_VERSION(old_cluster.major_version) < 1700) return; - /* Quick return if there are no subscriptions to be migrated. */ - if (old_cluster.nsubs == 0) + /* Quick return if there are no replication origins to be migrated. */ + if (old_cluster.nrepl_origins == 0) return; prep_status("Checking new cluster configuration for subscriptions"); @@ -2335,10 +2334,10 @@ check_new_cluster_subscription_configuration(void) pg_fatal("could not determine parameter settings on new cluster"); max_active_replication_origins = atoi(PQgetvalue(res, 0, 0)); - if (old_cluster.nsubs > max_active_replication_origins) + if (old_cluster.nrepl_origins > max_active_replication_origins) pg_fatal("\"max_active_replication_origins\" (%d) must be greater than or equal to the number of " - "subscriptions (%d) on the old cluster", - max_active_replication_origins, old_cluster.nsubs); + "replication origins (%d) on the old cluster", + max_active_replication_origins, old_cluster.nrepl_origins); PQclear(res); PQfinish(conn); diff --git a/src/bin/pg_upgrade/info.c b/src/bin/pg_upgrade/info.c index 37fff93892f..630f3f06e24 100644 --- a/src/bin/pg_upgrade/info.c +++ b/src/bin/pg_upgrade/info.c @@ -843,6 +843,7 @@ get_subscription_info(ClusterInfo *cluster) PGconn *conn; PGresult *res; int i_nsub; + int i_nrepl_origins; int i_retain_dead_tuples; conn = connectToServer(cluster, "template1"); @@ -862,6 +863,14 @@ get_subscription_info(ClusterInfo *cluster) cluster->sub_retain_dead_tuples = (strcmp(PQgetvalue(res, 0, i_retain_dead_tuples), "t") == 0); PQclear(res); + + res = executeQueryOrDie(conn, + "SELECT count(*) AS nrepl_origins " + "FROM pg_catalog.pg_replication_origin"); + i_nrepl_origins = PQfnumber(res, "nrepl_origins"); + cluster->nrepl_origins = atoi(PQgetvalue(res, 0, i_nrepl_origins)); + PQclear(res); + PQfinish(conn); } diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h index ccd1ac0d013..77e7ca1b4cd 100644 --- a/src/bin/pg_upgrade/pg_upgrade.h +++ b/src/bin/pg_upgrade/pg_upgrade.h @@ -311,6 +311,7 @@ typedef struct int num_tablespaces; const char *tablespace_suffix; /* directory specification */ int nsubs; /* number of subscriptions */ + int nrepl_origins; /* number of replication origins */ bool sub_retain_dead_tuples; /* whether a subscription enables * retain_dead_tuples. */ } ClusterInfo; diff --git a/src/bin/pg_upgrade/t/004_subscription.pl b/src/bin/pg_upgrade/t/004_subscription.pl index 646767f2a65..e8b11d39dd0 100644 --- a/src/bin/pg_upgrade/t/004_subscription.pl +++ b/src/bin/pg_upgrade/t/004_subscription.pl @@ -42,7 +42,7 @@ my $connstr = $publisher->connstr . ' dbname=postgres'; # ------------------------------------------------------ # Check that pg_upgrade fails when max_active_replication_origins configured -# in the new cluster is less than the number of subscriptions in the old +# in the new cluster is less than the number of replication origins in the old # cluster. # ------------------------------------------------------ # It is sufficient to use disabled subscription to test upgrade failure. @@ -74,7 +74,7 @@ command_checks_all( ], 1, [ - qr/"max_active_replication_origins" \(0\) must be greater than or equal to the number of subscriptions \(1\) on the old cluster/ + qr/"max_active_replication_origins" \(0\) must be greater than or equal to the number of replication origins \(1\) on the old cluster/ ], [qr//], 'run of pg_upgrade where the new cluster has insufficient max_active_replication_origins' @@ -301,8 +301,30 @@ is($result, qq(t), "Check that the table is in init state"); # Get the replication origin's remote_lsn of the old subscriber my $remote_lsn = $old_sub->safe_psql('postgres', - "SELECT remote_lsn FROM pg_replication_origin_status os, pg_subscription s WHERE os.external_id = 'pg_' || s.oid AND s.subname = 'regress_sub4'" + "SELECT os.remote_lsn + FROM pg_replication_origin_status os + JOIN pg_replication_origin o ON o.roident = os.local_id + JOIN pg_subscription s ON o.roname = 'pg_' || s.oid::text + WHERE s.subname = 'regress_sub4'" ); + +# Get the replication origin OIDs (roident) for all subscriptions, keyed by +# subscription name (which is stable across upgrade, unlike suboid). These +# must be preserved after upgrade. A mismatch would cause spurious +# update_origin_differs conflicts. +my %pre_upgrade_roident; +my $roident_rows = $old_sub->safe_psql('postgres', + "SELECT s.subname, o.roident + FROM pg_subscription s + JOIN pg_replication_origin o ON o.roname = 'pg_' || s.oid::text + ORDER BY s.subname" +); +for my $row (split /\n/, $roident_rows) +{ + my ($subname, $roident) = split /\|/, $row; + $pre_upgrade_roident{$subname} = $roident; +} + # Have the subscription in disabled state before upgrade $old_sub->safe_psql('postgres', "ALTER SUBSCRIPTION regress_sub5 DISABLE"); @@ -378,6 +400,20 @@ regress_sub5|f|f|f), "check that the subscription's running status, failover, and retain_dead_tuples are preserved" ); +# Verify that replication origin OIDs are preserved after upgrade. +my $post_roident_rows = $new_sub->safe_psql('postgres', + "SELECT s.subname, o.roident + FROM pg_subscription s + JOIN pg_replication_origin o ON o.roname = 'pg_' || s.oid::text + ORDER BY s.subname" +); +for my $row (split /\n/, $post_roident_rows) +{ + my ($subname, $roident) = split /\|/, $row; + is($roident, $pre_upgrade_roident{$subname}, + "roident preserved for subscription '$subname' after upgrade"); +} + # Subscription relations should be preserved $result = $new_sub->safe_psql('postgres', "SELECT srrelid, srsubstate FROM pg_subscription_rel ORDER BY srrelid"); diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 3a28406981d..1d2d624392e 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -11960,10 +11960,6 @@ provolatile => 'v', proparallel => 'u', prorettype => 'void', proargtypes => 'text oid char pg_lsn', prosrc => 'binary_upgrade_add_sub_rel_state' }, -{ oid => '6320', descr => 'for use by pg_upgrade (remote_lsn for origin)', - proname => 'binary_upgrade_replorigin_advance', proisstrict => 'f', - provolatile => 'v', proparallel => 'u', prorettype => 'void', - proargtypes => 'text pg_lsn', prosrc => 'binary_upgrade_replorigin_advance' }, { oid => '6505', descr => 'for use by pg_upgrade (conflict detection slot)', proname => 'binary_upgrade_create_conflict_detection_slot', proisstrict => 'f', provolatile => 'v', proparallel => 'u', @@ -11973,6 +11969,10 @@ proname => 'binary_upgrade_set_next_pg_subscription_oid', provolatile => 'v', proparallel => 'r', prorettype => 'void', proargtypes => 'oid', prosrc => 'binary_upgrade_set_next_pg_subscription_oid' }, +{ oid => '9161', descr => 'for use by pg_upgrade (replication origin)', + proname => 'binary_upgrade_create_replication_origin', proisstrict => 'f', + provolatile => 'v', proparallel => 'u', prorettype => 'void', + proargtypes => 'oid name pg_lsn', prosrc => 'binary_upgrade_create_replication_origin' }, # conversion functions { oid => '4310', descr => 'internal conversion function for KOI8R to WIN1251', -- 2.47.3 [application/octet-stream] v6-0001-Preserve-subscription-OIDs-during-pg_upgrade.patch (9.1K, 3-v6-0001-Preserve-subscription-OIDs-during-pg_upgrade.patch) download | inline diff: From 9be4a488196d12cac3ccaba132b0efcef14a7a87 Mon Sep 17 00:00:00 2001 From: Ajin Cherian <[email protected]> Date: Mon, 18 May 2026 18:41:05 +1000 Subject: [PATCH v6 1/2] Preserve subscription OIDs during pg_upgrade Currently subscription OIDs can be changed when a cluster is upgraded using pg_upgrade. This is required for a subsequent patch which will preserve the replication oids after upgrade. Author: Vignesh C <[email protected]> --- src/backend/commands/subscriptioncmds.c | 25 +++++++++++++++++-- src/backend/utils/adt/pg_upgrade_support.c | 10 ++++++++ src/bin/pg_dump/pg_dump.c | 8 ++++++ src/bin/pg_upgrade/pg_upgrade.c | 3 +++ src/bin/pg_upgrade/t/004_subscription.pl | 7 ++++++ src/include/catalog/binary_upgrade.h | 1 + src/include/catalog/pg_proc.dat | 4 +++ .../expected/spgist_name_ops.out | 6 +++-- 8 files changed, 60 insertions(+), 4 deletions(-) diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c index 523959ba0ce..7c1f05a5fd5 100644 --- a/src/backend/commands/subscriptioncmds.c +++ b/src/backend/commands/subscriptioncmds.c @@ -83,6 +83,12 @@ /* check if the 'val' has 'bits' set */ #define IsSet(val, bits) (((val) & (bits)) == (bits)) +/* + * This will be set by the pg_upgrade_support function -- + * binary_upgrade_set_next_pg_subscription_oid(). + */ +Oid binary_upgrade_next_pg_subscription_oid = InvalidOid; + /* * Structure to hold a bitmap representing the user-provided CREATE/ALTER * SUBSCRIPTION command options and the parsed/default values of each of them. @@ -772,8 +778,23 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt, memset(values, 0, sizeof(values)); memset(nulls, false, sizeof(nulls)); - subid = GetNewOidWithIndex(rel, SubscriptionObjectIndexId, - Anum_pg_subscription_oid); + /* Use binary-upgrade override for pg_subscription.oid? */ + if (IsBinaryUpgrade) + { + if (!OidIsValid(binary_upgrade_next_pg_subscription_oid)) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("pg_subscription OID value not set when in binary upgrade mode"))); + + subid = binary_upgrade_next_pg_subscription_oid; + binary_upgrade_next_pg_subscription_oid = InvalidOid; + } + else + { + subid = GetNewOidWithIndex(rel, SubscriptionObjectIndexId, + Anum_pg_subscription_oid); + } + values[Anum_pg_subscription_oid - 1] = ObjectIdGetDatum(subid); values[Anum_pg_subscription_subdbid - 1] = ObjectIdGetDatum(MyDatabaseId); values[Anum_pg_subscription_subskiplsn - 1] = LSNGetDatum(InvalidXLogRecPtr); diff --git a/src/backend/utils/adt/pg_upgrade_support.c b/src/backend/utils/adt/pg_upgrade_support.c index b505a6b4fee..59c3e7f0146 100644 --- a/src/backend/utils/adt/pg_upgrade_support.c +++ b/src/backend/utils/adt/pg_upgrade_support.c @@ -181,6 +181,16 @@ binary_upgrade_set_next_pg_authid_oid(PG_FUNCTION_ARGS) PG_RETURN_VOID(); } +Datum +binary_upgrade_set_next_pg_subscription_oid(PG_FUNCTION_ARGS) +{ + Oid subid = PG_GETARG_OID(0); + + CHECK_IS_BINARY_UPGRADE; + binary_upgrade_next_pg_subscription_oid = subid; + PG_RETURN_VOID(); +} + Datum binary_upgrade_create_empty_extension(PG_FUNCTION_ARGS) { diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index d56dcc701ce..452d0b5e98a 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -5583,6 +5583,14 @@ dumpSubscription(Archive *fout, const SubscriptionInfo *subinfo) appendPQExpBuffer(delq, "DROP SUBSCRIPTION %s;\n", qsubname); + if (dopt->binary_upgrade) + { + appendPQExpBufferStr(query, "\n-- For binary upgrade, must preserve pg_subscription.oid\n"); + appendPQExpBuffer(query, + "SELECT pg_catalog.binary_upgrade_set_next_pg_subscription_oid('%u'::pg_catalog.oid);\n\n", + subinfo->dobj.catId.oid); + } + appendPQExpBuffer(query, "CREATE SUBSCRIPTION %s ", qsubname); if (subinfo->subservername) diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c index 2127d297bfe..4e853096698 100644 --- a/src/bin/pg_upgrade/pg_upgrade.c +++ b/src/bin/pg_upgrade/pg_upgrade.c @@ -35,6 +35,9 @@ * * We control all assignments of pg_database.oid because we want the directory * names to match between the old and new cluster. + * + * We control assignment of pg_subscription.oid because we want the oid to + * match between the old and new cluster. */ diff --git a/src/bin/pg_upgrade/t/004_subscription.pl b/src/bin/pg_upgrade/t/004_subscription.pl index c94a82deae0..646767f2a65 100644 --- a/src/bin/pg_upgrade/t/004_subscription.pl +++ b/src/bin/pg_upgrade/t/004_subscription.pl @@ -313,6 +313,9 @@ my $tab_upgraded1_oid = $old_sub->safe_psql('postgres', my $tab_upgraded2_oid = $old_sub->safe_psql('postgres', "SELECT oid FROM pg_class WHERE relname = 'tab_upgraded2'"); +$sub_oid = $old_sub->safe_psql('postgres', + "SELECT oid FROM pg_subscription ORDER BY subname"); + $old_sub->stop; # Change configuration so that initial table sync does not get started @@ -359,6 +362,10 @@ $publisher->safe_psql( $new_sub->start; +# The subscription oid should be preserved +$result = $new_sub->safe_psql('postgres', "SELECT oid FROM pg_subscription ORDER BY subname"); +is($result, qq($sub_oid), "subscription oid should have been preserved"); + # The subscription's running status, failover option, and retain_dead_tuples # option should be preserved in the upgraded instance. So regress_sub4 should # still have subenabled, subfailover, and subretaindeadtuples set to true, diff --git a/src/include/catalog/binary_upgrade.h b/src/include/catalog/binary_upgrade.h index 7bf7ae44385..b15b18e7dc9 100644 --- a/src/include/catalog/binary_upgrade.h +++ b/src/include/catalog/binary_upgrade.h @@ -32,6 +32,7 @@ extern PGDLLIMPORT RelFileNumber binary_upgrade_next_toast_pg_class_relfilenumbe extern PGDLLIMPORT Oid binary_upgrade_next_pg_enum_oid; extern PGDLLIMPORT Oid binary_upgrade_next_pg_authid_oid; +extern PGDLLIMPORT Oid binary_upgrade_next_pg_subscription_oid; extern PGDLLIMPORT bool binary_upgrade_record_init_privs; diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index be157a5fbe9..3a28406981d 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -11969,6 +11969,10 @@ proisstrict => 'f', provolatile => 'v', proparallel => 'u', prorettype => 'void', proargtypes => '', prosrc => 'binary_upgrade_create_conflict_detection_slot' }, +{ oid => '9160', descr => 'for use by pg_upgrade', + proname => 'binary_upgrade_set_next_pg_subscription_oid', provolatile => 'v', + proparallel => 'r', prorettype => 'void', proargtypes => 'oid', + prosrc => 'binary_upgrade_set_next_pg_subscription_oid' }, # conversion functions { oid => '4310', descr => 'internal conversion function for KOI8R to WIN1251', diff --git a/src/test/modules/spgist_name_ops/expected/spgist_name_ops.out b/src/test/modules/spgist_name_ops/expected/spgist_name_ops.out index 1ee65ede243..39d43368c42 100644 --- a/src/test/modules/spgist_name_ops/expected/spgist_name_ops.out +++ b/src/test/modules/spgist_name_ops/expected/spgist_name_ops.out @@ -59,11 +59,12 @@ select * from t binary_upgrade_set_next_multirange_pg_type_oid | 1 | binary_upgrade_set_next_multirange_pg_type_oid binary_upgrade_set_next_pg_authid_oid | | binary_upgrade_set_next_pg_authid_oid binary_upgrade_set_next_pg_enum_oid | | binary_upgrade_set_next_pg_enum_oid + binary_upgrade_set_next_pg_subscription_oid | | binary_upgrade_set_next_pg_subscription_oid binary_upgrade_set_next_pg_tablespace_oid | | binary_upgrade_set_next_pg_tablespace_oid binary_upgrade_set_next_pg_type_oid | | binary_upgrade_set_next_pg_type_oid binary_upgrade_set_next_toast_pg_class_oid | 1 | binary_upgrade_set_next_toast_pg_class_oid binary_upgrade_set_next_toast_relfilenode | | binary_upgrade_set_next_toast_relfilenode -(13 rows) +(14 rows) -- Verify clean failure when INCLUDE'd columns result in overlength tuple -- The error message details are platform-dependent, so show only SQLSTATE @@ -108,11 +109,12 @@ select * from t binary_upgrade_set_next_multirange_pg_type_oid | 1 | binary_upgrade_set_next_multirange_pg_type_oid binary_upgrade_set_next_pg_authid_oid | | binary_upgrade_set_next_pg_authid_oid binary_upgrade_set_next_pg_enum_oid | | binary_upgrade_set_next_pg_enum_oid + binary_upgrade_set_next_pg_subscription_oid | | binary_upgrade_set_next_pg_subscription_oid binary_upgrade_set_next_pg_tablespace_oid | | binary_upgrade_set_next_pg_tablespace_oid binary_upgrade_set_next_pg_type_oid | | binary_upgrade_set_next_pg_type_oid binary_upgrade_set_next_toast_pg_class_oid | 1 | binary_upgrade_set_next_toast_pg_class_oid binary_upgrade_set_next_toast_relfilenode | | binary_upgrade_set_next_toast_relfilenode -(13 rows) +(14 rows) \set VERBOSITY sqlstate insert into t values(repeat('xyzzy', 12), 42, repeat('xyzzy', 4000)); -- 2.47.3 ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: [PATCH] Preserve replication origin OIDs in pg_upgrade @ 2026-05-22 09:46 Shlok Kyal <[email protected]> parent: Ajin Cherian <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Shlok Kyal @ 2026-05-22 09:46 UTC (permalink / raw) To: Ajin Cherian <[email protected]>; +Cc: Zsolt Parragi <[email protected]>; vignesh C <[email protected]>; Hayato Kuroda (Fujitsu) <[email protected]>; PostgreSQL Hackers <[email protected]> On Mon, 18 May 2026 at 16:13, Ajin Cherian <[email protected]> wrote: > > Rebased the patch as it was no longer applying. > Hi Ajin, I have started reviewing the patch. Here is my comment for v6-0002 patch: Suppose we have a replication setup: publisher -> subscriber and we are upgrading subscriber to subscriber_new. And if initially 'subscriber_new' has a replication origin, upgrading the cluster can error out. Example: We set up a logical replication between publisher node and subscriber node. On subscriber node: postgres=# SELECT * FROM pg_replication_origin; roident | roname ---------+---------- 1 | pg_16393 (1 row) And initially subscriber_new has a replication origin: postgres=# select pg_replication_origin_create('myname'); pg_replication_origin_create ------------------------------ 1 (1 row) postgres=# SELECT * FROM pg_replication_origin; roident | roname ---------+-------- 1 | myname (1 row) Now, if we run pg_upgrade to upgrade subscriber node to subscriber_new node, we get an error: ``` SELECT pg_catalog.binary_upgrade_create_replication_origin('1'::pg_catalog.oid, 'pg_16393'::pg_catalog.name, '0/01743078'::pg_catalog.pg_lsn); psql:subscriber_new/pg_upgrade_output.d/20260522T140312.807/dump/pg_upgrade_dump_globals.sql:37: ERROR: replication origin with ID 1 already exists ``` This error occurs in "Performing Upgrade" stage. Should we add a check in the "Performing Consistency Checks" stage so that we don't need to re-initdb the new cluster to perform the upgrade? Maybe we can add a check similar to check_new_cluster_replication_slots(), where pg_upgrade errors out if the new cluster already contains replication origins. Thoughts? Thanks, Shlok Kyal ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: [PATCH] Preserve replication origin OIDs in pg_upgrade @ 2026-05-22 10:27 shveta malik <[email protected]> parent: Shlok Kyal <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: shveta malik @ 2026-05-22 10:27 UTC (permalink / raw) To: Shlok Kyal <[email protected]>; Ajin Cherian <[email protected]>; +Cc: Zsolt Parragi <[email protected]>; vignesh C <[email protected]>; Hayato Kuroda (Fujitsu) <[email protected]>; PostgreSQL Hackers <[email protected]>; shveta malik <[email protected]> On Fri, May 22, 2026 at 3:16 PM Shlok Kyal <[email protected]> wrote: > > On Mon, 18 May 2026 at 16:13, Ajin Cherian <[email protected]> wrote: > > > > Rebased the patch as it was no longer applying. > > > Hi Ajin, > > I have started reviewing the patch. Here is my comment for v6-0002 patch: > > Suppose we have a replication setup: publisher -> subscriber > and we are upgrading subscriber to subscriber_new. > And if initially 'subscriber_new' has a replication origin, upgrading > the cluster can error out. > > Example: > We set up a logical replication between publisher node and subscriber node. > > On subscriber node: > postgres=# SELECT * FROM pg_replication_origin; > roident | roname > ---------+---------- > 1 | pg_16393 > (1 row) > > And initially subscriber_new has a replication origin: > postgres=# select pg_replication_origin_create('myname'); > pg_replication_origin_create > ------------------------------ > 1 > (1 row) > > postgres=# SELECT * FROM pg_replication_origin; > roident | roname > ---------+-------- > 1 | myname > (1 row) > > Now, if we run pg_upgrade to upgrade subscriber node to subscriber_new > node, we get an error: > ``` > SELECT pg_catalog.binary_upgrade_create_replication_origin('1'::pg_catalog.oid, > 'pg_16393'::pg_catalog.name, '0/01743078'::pg_catalog.pg_lsn); > psql:subscriber_new/pg_upgrade_output.d/20260522T140312.807/dump/pg_upgrade_dump_globals.sql:37: > ERROR: replication origin with ID 1 already exists > ``` > > This error occurs in "Performing Upgrade" stage. Should we add a check > in the "Performing Consistency Checks" stage so that we don't need to > re-initdb the new cluster to perform the upgrade? > Maybe we can add a check similar to > check_new_cluster_replication_slots(), where pg_upgrade errors out if > the new cluster already contains replication origins. Thoughts? +1. I had the same thought while reviewing the patch today. We should have it unless there is a reason we have avoided it?? Few trivial comments: 1) +#include "access/skey.h" +#include "catalog/indexing.h" pg_upgrade_support.c compiles without above. 2) + Assert(!OidIsValid(rel->rd_rel->reltoastrelid)); Is there a reason for this sanity check? I generally do not see a Null-Toast table sanity check after every table_open. 3) + + /* Dump replication origins */ + if (server_version >= 170000 && binary_upgrade && archDumpFormat == archNull) + dumpReplicationOrigins(conn); why the check is for PG17 specifically? thanks Shveta ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: [PATCH] Preserve replication origin OIDs in pg_upgrade @ 2026-05-25 07:13 shveta malik <[email protected]> parent: shveta malik <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: shveta malik @ 2026-05-25 07:13 UTC (permalink / raw) To: Shlok Kyal <[email protected]>; Ajin Cherian <[email protected]>; +Cc: Zsolt Parragi <[email protected]>; vignesh C <[email protected]>; Hayato Kuroda (Fujitsu) <[email protected]>; PostgreSQL Hackers <[email protected]>; shveta malik <[email protected]> On Fri, May 22, 2026 at 3:57 PM shveta malik <[email protected]> wrote: > > On Fri, May 22, 2026 at 3:16 PM Shlok Kyal <[email protected]> wrote: > > > > On Mon, 18 May 2026 at 16:13, Ajin Cherian <[email protected]> wrote: > > > > > > Rebased the patch as it was no longer applying. > > > > > Hi Ajin, > > > > I have started reviewing the patch. Here is my comment for v6-0002 patch: > > > > Suppose we have a replication setup: publisher -> subscriber > > and we are upgrading subscriber to subscriber_new. > > And if initially 'subscriber_new' has a replication origin, upgrading > > the cluster can error out. > > > > Example: > > We set up a logical replication between publisher node and subscriber node. > > > > On subscriber node: > > postgres=# SELECT * FROM pg_replication_origin; > > roident | roname > > ---------+---------- > > 1 | pg_16393 > > (1 row) > > > > And initially subscriber_new has a replication origin: > > postgres=# select pg_replication_origin_create('myname'); > > pg_replication_origin_create > > ------------------------------ > > 1 > > (1 row) > > > > postgres=# SELECT * FROM pg_replication_origin; > > roident | roname > > ---------+-------- > > 1 | myname > > (1 row) > > > > Now, if we run pg_upgrade to upgrade subscriber node to subscriber_new > > node, we get an error: > > ``` > > SELECT pg_catalog.binary_upgrade_create_replication_origin('1'::pg_catalog.oid, > > 'pg_16393'::pg_catalog.name, '0/01743078'::pg_catalog.pg_lsn); > > psql:subscriber_new/pg_upgrade_output.d/20260522T140312.807/dump/pg_upgrade_dump_globals.sql:37: > > ERROR: replication origin with ID 1 already exists > > ``` > > > > This error occurs in "Performing Upgrade" stage. Should we add a check > > in the "Performing Consistency Checks" stage so that we don't need to > > re-initdb the new cluster to perform the upgrade? > > Maybe we can add a check similar to > > check_new_cluster_replication_slots(), where pg_upgrade errors out if > > the new cluster already contains replication origins. Thoughts? > > +1. I had the same thought while reviewing the patch today. We should > have it unless there is a reason we have avoided it?? > > Few trivial comments: > > 1) > > +#include "access/skey.h" > +#include "catalog/indexing.h" > > pg_upgrade_support.c compiles without above. > > 2) > + Assert(!OidIsValid(rel->rd_rel->reltoastrelid)); > > Is there a reason for this sanity check? I generally do not see a > Null-Toast table sanity check after every table_open. > > 3) > > + > + /* Dump replication origins */ > + if (server_version >= 170000 && binary_upgrade && archDumpFormat == archNull) > + dumpReplicationOrigins(conn); > > why the check is for PG17 specifically? > One issue in 002: binary_upgrade_create_replication_origin() has this: + originname = PG_GETARG_NAME(1); + + roname_d = CStringGetTextDatum(NameStr(*originname)); + We are getting origin-name (text) into Name-type which can not be more than 64 bytes. So if an origin has name more than 64, it will end up trimming the name post-upgrade. I tried this: Old-setup: postgres=# SELECT pg_replication_origin_create('this_is_a_very_long_replication_origin_name_that_exceeds_the_limit_of_64'); pg_replication_origin_create ------------------------------ 1 postgres=# select * from pg_replication_origin; roident | roname ---------+---------------------------------------------------------------------- 1 | this_is_a_very_long_replication_origin_name_that_exceeds_the_limit_of_64 Post-upgrade: name got trimmed to 64 length. ------------------------- postgres=# select * from pg_replication_origin; roident | roname ---------+----------------------------------------------------------------- 1 | this_is_a_very_long_replication_origin_name_that_exceeds_the_li thanks Shveta ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2026-05-25 07:13 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-05-18 10:42 Re: [PATCH] Preserve replication origin OIDs in pg_upgrade Ajin Cherian <[email protected]> 2026-05-22 09:46 ` Shlok Kyal <[email protected]> 2026-05-22 10:27 ` shveta malik <[email protected]> 2026-05-25 07:13 ` shveta malik <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox