public inbox for [email protected]
help / color / mirror / Atom feedFrom: vignesh C <[email protected]>
To: Ajin Cherian <[email protected]>
Cc: shveta malik <[email protected]>
Cc: Shlok Kyal <[email protected]>
Cc: Zsolt Parragi <[email protected]>
Cc: Hayato Kuroda (Fujitsu) <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: shveta malik <[email protected]>
Subject: Re: [PATCH] Preserve replication origin OIDs in pg_upgrade
Date: Fri, 5 Jun 2026 15:39:49 +0530
Message-ID: <CALDaNm1L7++aSjacbFayEvgsj5zjdfa9tc3sGw6N9NVt7DSOKQ@mail.gmail.com> (raw)
In-Reply-To: <CAFPTHDavPyxsWjK0cRO3yOaP4u8FGYrOJuXJB-4wzneAY3H3Ug@mail.gmail.com>
References: <CAFPTHDa5aP8ixTZBygGAe48E1N=DvuPzcXikw7bNKixLMN3pVg@mail.gmail.com>
<OS9PR01MB12149E188221BED862E4BE9E0F5342@OS9PR01MB12149.jpnprd01.prod.outlook.com>
<CALDaNm2-uwpbJ8fnrssp+hORvOutsqRoZAsa05xVVzXe5Bt3bw@mail.gmail.com>
<CAFPTHDaNLRqPAda1RUDVfSDH7eLTaONv0UEmc9H5sdMW1Li2Bg@mail.gmail.com>
<CAN4CZFOb3urMsLPsEyVrYR7-7yA+BC5kDgQQd0nAQ8xj2zyRcA@mail.gmail.com>
<CAFPTHDYqpuZ6o2-HuCJDYqJ7GY3+zV+Xo-gT7PAgi4Bkz+oTxw@mail.gmail.com>
<CAN4CZFN4oLdxLwUXHUPV-5mFmK+4dcnppP00fV3i4qmMYCAkGA@mail.gmail.com>
<CAFPTHDbBiTfjYkQwYTNA2a4LRp8Sp7_zB59fmV0R7977ztxgmg@mail.gmail.com>
<CAFPTHDaftSwzGTGbFEw8rwDBsub0XqcDm1wxQcquj-Y3PC2qrg@mail.gmail.com>
<CANhcyEVercsNbgC5CTkMNzh_w_Jv1uK7RQg9vEvAeNQhoBHCKQ@mail.gmail.com>
<CAJpy0uAXynzWr4w_9GkJC3i=LL9WsRCZYVWAV0PgKmgs8riWzg@mail.gmail.com>
<CAFPTHDavPyxsWjK0cRO3yOaP4u8FGYrOJuXJB-4wzneAY3H3Ug@mail.gmail.com>
On Thu, 28 May 2026 at 08:18, Ajin Cherian <[email protected]> wrote:
>
> Fixed this. Added a new check for replication origins and if the new cluster has any existing replication origins, then the check will fail.
>>
>>
>> Few trivial comments:
>>
>> 1)
>>
>> +#include "access/skey.h"
>> +#include "catalog/indexing.h"
>>
>> pg_upgrade_support.c compiles without above.
>>
>
> Removed.
>
>>
>> 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.
>>
>
> Removed.
>
>>
>> 3)
>>
>> +
>> + /* Dump replication origins */
>> + if (server_version >= 170000 && binary_upgrade && archDumpFormat == archNull)
>> + dumpReplicationOrigins(conn);
>>
>> why the check is for PG17 specifically?
>>
>
> In PG17, we started migrating pg_subscription_rel and the remote LSN during upgrades; prior to that, these were not migrated. Given that change, it also makes sense to migrate replication origins from them. Otherwise, when upgrading from PG17 to a later version, you could end up with a subscription where pg_subscription_rel and the remote LSN are migrated, but the corresponding replication origin is not created.
>
>
>
> On Mon, May 25, 2026 at 5:13 PM shveta malik <[email protected]> wrote:
>>
>>
>> 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
>
>
> Fixed this. Now binary_upgrade_create_replication_origin handles it similarly to the way pg_replication_origin_create handles the name of the origin.
Few comments:
1) pg_dumpall dumps all replication origins, not just subscription
origins and recreates every origin:
SELECT o.*, os.remote_lsn
FROM pg_replication_origin o
LEFT JOIN pg_replication_origin_status os
ON o.roident = os.local_id
Is pg_upgrade expected to preserve user-created origins?
If so, you can mention this in the commit message
2) Should the table lock be released here, if so should we mention
comments similar to how it is mentioned in
binary_upgarde_add_sub_rel_state:
+ /* 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);
3) We can use new style of ereport to exclude '(' before errcode:
3.a)
+ 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)));
3.b) Similarly here:
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("replication origin with ID %u already exists", node_oid)));
3.c) There are many others, you can search other ereport in the patch
and change it too.
4) Since only one column will be returned here, can we use
"PQgetvalue(res, 0, 0) and remove PQfnumber:
+ 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);
5) replorigin_create also has some duplicate code like below, will it
be possible to have a common function so that both of them can use?
/* 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",
+ 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();
Regards,
Vignesh
view thread (44+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH] Preserve replication origin OIDs in pg_upgrade
In-Reply-To: <CALDaNm1L7++aSjacbFayEvgsj5zjdfa9tc3sGw6N9NVt7DSOKQ@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox