From 6acc78cdf70263adc4e6cfe0fca1ebc7ac31dc2b Mon Sep 17 00:00:00 2001 From: Jeff Davis Date: Thu, 30 Jul 2026 18:30:48 -0700 Subject: [PATCH v4 8/8] When changing owner of a subscription, do not throw an error. Errors will be caught when the connection is actually used. Restore uses multiple DDL commands to restore a subscription, so checks of the intermediate state risk restore errors. In the future we could address this with a more careful restoration order, but the DDL-time errors are merely for convenience. Addresses finding 2 in the report from the linked discussion. Reported-by: Noah Misch Discussion: https://postgr.es/m/20260710195902.4f.noahmisch%40microsoft.com Discussion: https://postgr.es/m/e103ae8daf74485e0c0ebde297fae735d38f54d1.camel@j-davis.com Backpatch-through: 19 --- src/backend/commands/subscriptioncmds.c | 24 +++++++--------------- src/test/regress/expected/subscription.out | 5 +++++ src/test/regress/sql/subscription.sql | 4 ++++ 3 files changed, 16 insertions(+), 17 deletions(-) diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c index 9c9c0de04eb..8cb7d607412 100644 --- a/src/backend/commands/subscriptioncmds.c +++ b/src/backend/commands/subscriptioncmds.c @@ -3006,25 +3006,15 @@ AlterSubscriptionOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId) get_database_name(MyDatabaseId)); /* - * If the subscription uses a server, check that the new owner has USAGE - * privileges on the server and that a user mapping exists. Note: does not - * re-check the resulting connection string. + * The privileges will be checked before the connection is actually used, + * so it does not need to be done here. Avoid unnecessary risk of errors + * here, which could interfere with restore. + * + * However, it is convenient to check if a user mapping exists, and raise + * a WARNING if not. */ if (OidIsValid(form->subserver)) - { - ForeignServer *server = GetForeignServer(form->subserver); - - aclresult = object_aclcheck(ForeignServerRelationId, server->serverid, newOwnerId, ACL_USAGE); - if (aclresult != ACLCHECK_OK) - ereport(ERROR, - errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("new subscription owner \"%s\" does not have permission on foreign server \"%s\"", - GetUserNameFromId(newOwnerId, false), - server->servername)); - - /* make sure a user mapping exists */ - GetUserMapping(newOwnerId, server->serverid); - } + GetUserMappingExtended(newOwnerId, form->subserver, WARNING); form->subowner = newOwnerId; CatalogTupleUpdate(rel, &tup->t_self, tup); diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out index 5fcd6891e4c..7b672aac72e 100644 --- a/src/test/regress/expected/subscription.out +++ b/src/test/regress/expected/subscription.out @@ -191,6 +191,11 @@ RESET SESSION AUTHORIZATION; DROP SERVER test_server CASCADE; ERROR: cannot drop server test_server because subscription regress_testsub6 depends on it HINT: Drop subscription regress_testsub6 first. +-- ok, USAGE privilege on server not checked for OWNER TO, but warn +-- about user mapping +ALTER SUBSCRIPTION regress_testsub6 OWNER TO regress_subscription_user2; +WARNING: user mapping not found for user "regress_subscription_user2", server "test_server" +ALTER SUBSCRIPTION regress_testsub6 OWNER TO regress_subscription_user3; REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3; SET SESSION AUTHORIZATION regress_subscription_user3; -- ok, lacks USAGE on test_server, but replacing connection anyway diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql index 58082f1c268..c8d9f80a499 100644 --- a/src/test/regress/sql/subscription.sql +++ b/src/test/regress/sql/subscription.sql @@ -136,6 +136,10 @@ RESET SESSION AUTHORIZATION; -- fail, subscription depends on the server and cannot be dropped by CASCADE DROP SERVER test_server CASCADE; +-- ok, USAGE privilege on server not checked for OWNER TO, but warn +-- about user mapping +ALTER SUBSCRIPTION regress_testsub6 OWNER TO regress_subscription_user2; +ALTER SUBSCRIPTION regress_testsub6 OWNER TO regress_subscription_user3; REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3; SET SESSION AUTHORIZATION regress_subscription_user3; -- 2.43.0