On Thu, May 28, 2026 at 8:18 AM Ajin Cherian <[email protected]> wrote:
>
>
>
>> 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.
>
Okay, please add a comment in code for this.
Please find a few comments on v007:
1)
+ node = (ReplOriginId) node_oid;
+ originname = text_to_cstring(PG_GETARG_TEXT_PP(1));
+
+ if (strlen(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(originname);
CStringGetTextDatum is:
#define CStringGetTextDatum(s) PointerGetDatum(cstring_to_text(s))
---------
We are first converting text to C-string , then Cstring to text and
then getting Datum. Double allocation and double conversion. Can we
please try this instead and verify if it works:
text *origin_text = PG_GETARG_TEXT_PP(1);
originname = text_to_cstring(origin_text);
roname_d = PointerGetDatum(origin_text);
2)
+check_new_cluster_replication_origins(void)
+{
+ PGconn *conn;
+ PGresult *res;
+ int norigins;
+
+ /* Quick return if there are no replication origins to migrate. */
+ if (old_cluster.nrepl_origins == 0)
+ return;
Don't we need a version check simialr to what we have introduced in
pg_dumpall for origins? (Similar to
check_new_cluster_replication_slots as well)
3)
Since we have introduced check_new_cluster_replication_origins, do we
even need below checks in binary_upgrade_create_replication_origin()?
In which conditions will they be hit?
+ /* Check for OID collision */
+ ....
+ if (collides)
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("replication origin with ID %u already exists", node_oid)));
+
+ /* Check for name collision */
+ ....
+ if (collides)
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("replication origin \"%s\" already exists",
+ originname)));
4)
Now since check_new_cluster_subscription_configuration() is purely
about checking max-origin GUC configuration, I think the name is
misleading. IMO, we should merge it to
check_new_cluster_replication_origins(). See how
check_new_cluster_replication_slots() does it: checking both
new-cluster's count and the max-configuration for slots.
thanks
Shveta
