From 1797d4b1a7eb1450c3611e3ba039e2bf977fb6e4 Mon Sep 17 00:00:00 2001 From: Jeff Davis Date: Thu, 30 Jul 2026 13:40:02 -0700 Subject: [PATCH v3 06/11] Be precise about when ALTER SUBSCRIPTION needs conninfo. Decide early whether the original conninfo is needed so that errors happen consistently. Addresses finding 12 in report from linked discussion. Co-authored-by: Shlok Kyal Reported-by: Noah Misch Reviewed-by: Hayato Kuroda (Fujitsu) Discussion: https://postgr.es/m/20260710195902.4f.noahmisch%40microsoft.com Backpatch-through: 19 --- src/backend/commands/subscriptioncmds.c | 83 ++++++++++++++-------- src/test/regress/expected/subscription.out | 6 ++ src/test/regress/sql/subscription.sql | 7 ++ 3 files changed, 68 insertions(+), 28 deletions(-) diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c index 6ca215f8bf3..d05ef3fb4b2 100644 --- a/src/backend/commands/subscriptioncmds.c +++ b/src/backend/commands/subscriptioncmds.c @@ -1632,7 +1632,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt, Datum values[Natts_pg_subscription]; HeapTuple tup; Oid subid; - bool orig_conninfo_needed = true; + bool orig_conninfo_needed = false; bool update_tuple = false; bool update_failover = false; bool update_two_phase = false; @@ -1714,37 +1714,64 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt, if (supported_opts > 0) parse_subscription_options(pstate, stmt->options, supported_opts, &opts); + sub = GetSubscription(subid, false); + /* - * Ensure that ALTER SUBSCRIPTION commands that could be used to fix a - * broken connection or prepare to drop a broken subscription don't - * attempt to construct the conninfo. Otherwise, we might encounter the - * error the user is trying to fix. - * - * Specifically, ALTER SUBSCRIPTION DISABLE, ALTER SUBSCRIPTION SERVER, - * ALTER SUBSCRIPTION CONNECTION, or ALTER SUBSCRIPTION SET - * (slot_name=NONE). - * - * NB: if the user specifies multiple SET options, then we may still need - * to construct conninfo even if slot_name is set to NONE. + * Determine in advance whether we need the original conninfo or not, so + * that errors are generated consistently in cases where we do need it; + * and not generated at all if we don't. */ - if (stmt->kind == ALTER_SUBSCRIPTION_ENABLED) - { - if (opts.specified_opts == SUBOPT_ENABLED && !opts.enabled) - orig_conninfo_needed = false; - } - else if (stmt->kind == ALTER_SUBSCRIPTION_SERVER || - stmt->kind == ALTER_SUBSCRIPTION_CONNECTION) - { - orig_conninfo_needed = false; - } - else if (stmt->kind == ALTER_SUBSCRIPTION_OPTIONS) + + /* conninfo needed when refreshing */ + switch (stmt->kind) { - /* ... SET (slot_name = NONE) with no other options */ - if (opts.specified_opts == SUBOPT_SLOT_NAME && !opts.slot_name) - orig_conninfo_needed = false; - } + case ALTER_SUBSCRIPTION_REFRESH_PUBLICATION: + case ALTER_SUBSCRIPTION_REFRESH_SEQUENCES: + orig_conninfo_needed = true; + break; - sub = GetSubscription(subid, false); + case ALTER_SUBSCRIPTION_SET_PUBLICATION: + case ALTER_SUBSCRIPTION_ADD_PUBLICATION: + case ALTER_SUBSCRIPTION_DROP_PUBLICATION: + /* opts.refresh defaults to true when the option is supported */ + orig_conninfo_needed = opts.refresh; + break; + + case ALTER_SUBSCRIPTION_ENABLED: + orig_conninfo_needed = opts.enabled && sub->retaindeadtuples; + break; + + case ALTER_SUBSCRIPTION_OPTIONS: + { + if (sub->slotname) + { + if (IsSet(opts.specified_opts, SUBOPT_FAILOVER)) + orig_conninfo_needed = true; + if (IsSet(opts.specified_opts, SUBOPT_TWOPHASE_COMMIT) && + !opts.twophase) + orig_conninfo_needed = true; + } + + if (IsSet(opts.specified_opts, SUBOPT_RETAIN_DEAD_TUPLES) && + opts.retaindeadtuples) + orig_conninfo_needed = true; + + if (IsSet(opts.specified_opts, SUBOPT_ORIGIN)) + { + bool rdt; + + rdt = IsSet(opts.specified_opts, SUBOPT_RETAIN_DEAD_TUPLES) ? + opts.retaindeadtuples : sub->retaindeadtuples; + + if (rdt && pg_strcasecmp(opts.origin, LOGICALREP_ORIGIN_ANY) == 0) + orig_conninfo_needed = true; + } + } + break; + + default: + break; + } /* * Skip ACL checks on the subscription's foreign server, if any. If diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out index 259db747334..d0955ca1159 100644 --- a/src/test/regress/expected/subscription.out +++ b/src/test/regress/expected/subscription.out @@ -229,6 +229,12 @@ CREATE SUBSCRIPTION regress_testsub6 SERVER test_server WARNING: subscription was created, but is not connected HINT: To initiate replication, you must manually create the replication slot, enable the subscription, and alter the subscription to refresh publications. DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server; +-- ok, catalog-only forms don't construct conninfo +ALTER SUBSCRIPTION regress_testsub6 SET (synchronous_commit = local); +ALTER SUBSCRIPTION regress_testsub6 SET (synchronous_commit = off); +ALTER SUBSCRIPTION regress_testsub6 SET (disable_on_error = true); +ALTER SUBSCRIPTION regress_testsub6 SET (disable_on_error = false); +ALTER SUBSCRIPTION regress_testsub6 SET PUBLICATION testpub WITH (refresh = false); -- ok, test_server lacks user mapping, but replacing connection anyway BEGIN; ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret'; diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql index 7718c742974..98304737adc 100644 --- a/src/test/regress/sql/subscription.sql +++ b/src/test/regress/sql/subscription.sql @@ -176,6 +176,13 @@ CREATE SUBSCRIPTION regress_testsub6 SERVER test_server DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server; +-- ok, catalog-only forms don't construct conninfo +ALTER SUBSCRIPTION regress_testsub6 SET (synchronous_commit = local); +ALTER SUBSCRIPTION regress_testsub6 SET (synchronous_commit = off); +ALTER SUBSCRIPTION regress_testsub6 SET (disable_on_error = true); +ALTER SUBSCRIPTION regress_testsub6 SET (disable_on_error = false); +ALTER SUBSCRIPTION regress_testsub6 SET PUBLICATION testpub WITH (refresh = false); + -- ok, test_server lacks user mapping, but replacing connection anyway BEGIN; ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret'; -- 2.43.0