public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v5 1/3] Allow wait event set to be registered to resource owner 3+ messages / 3 participants [nested] [flat]
* [PATCH v5 1/3] Allow wait event set to be registered to resource owner @ 2017-05-22 03:42 Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Kyotaro Horiguchi @ 2017-05-22 03:42 UTC (permalink / raw) WaitEventSet needs to be released using resource owner for a certain case. This change adds WaitEventSet reowner and allow the creator of a WaitEventSet to specify a resource owner. --- src/backend/libpq/pqcomm.c | 2 +- src/backend/storage/ipc/latch.c | 18 ++++- src/backend/storage/lmgr/condition_variable.c | 2 +- src/backend/utils/resowner/resowner.c | 67 +++++++++++++++++++ src/include/storage/latch.h | 4 +- src/include/utils/resowner_private.h | 8 +++ 6 files changed, 96 insertions(+), 5 deletions(-) diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c index 7717bb2719..16aefb03ee 100644 --- a/src/backend/libpq/pqcomm.c +++ b/src/backend/libpq/pqcomm.c @@ -218,7 +218,7 @@ pq_init(void) (errmsg("could not set socket to nonblocking mode: %m"))); #endif - FeBeWaitSet = CreateWaitEventSet(TopMemoryContext, 3); + FeBeWaitSet = CreateWaitEventSet(TopMemoryContext, NULL, 3); AddWaitEventToSet(FeBeWaitSet, WL_SOCKET_WRITEABLE, MyProcPort->sock, NULL, NULL); AddWaitEventToSet(FeBeWaitSet, WL_LATCH_SET, -1, MyLatch, NULL); diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c index 91fa4b619b..10d71b46cb 100644 --- a/src/backend/storage/ipc/latch.c +++ b/src/backend/storage/ipc/latch.c @@ -56,6 +56,7 @@ #include "storage/latch.h" #include "storage/pmsignal.h" #include "storage/shmem.h" +#include "utils/resowner_private.h" /* * Select the fd readiness primitive to use. Normally the "most modern" @@ -84,6 +85,8 @@ struct WaitEventSet int nevents; /* number of registered events */ int nevents_space; /* maximum number of events in this set */ + ResourceOwner resowner; /* Resource owner */ + /* * Array, of nevents_space length, storing the definition of events this * set is waiting for. @@ -393,7 +396,7 @@ WaitLatchOrSocket(Latch *latch, int wakeEvents, pgsocket sock, int ret = 0; int rc; WaitEvent event; - WaitEventSet *set = CreateWaitEventSet(CurrentMemoryContext, 3); + WaitEventSet *set = CreateWaitEventSet(CurrentMemoryContext, NULL, 3); if (wakeEvents & WL_TIMEOUT) Assert(timeout >= 0); @@ -560,12 +563,15 @@ ResetLatch(Latch *latch) * WaitEventSetWait(). */ WaitEventSet * -CreateWaitEventSet(MemoryContext context, int nevents) +CreateWaitEventSet(MemoryContext context, ResourceOwner res, int nevents) { WaitEventSet *set; char *data; Size sz = 0; + if (res) + ResourceOwnerEnlargeWESs(res); + /* * Use MAXALIGN size/alignment to guarantee that later uses of memory are * aligned correctly. E.g. epoll_event might need 8 byte alignment on some @@ -680,6 +686,11 @@ CreateWaitEventSet(MemoryContext context, int nevents) StaticAssertStmt(WSA_INVALID_EVENT == NULL, ""); #endif + /* Register this wait event set if requested */ + set->resowner = res; + if (res) + ResourceOwnerRememberWES(set->resowner, set); + return set; } @@ -725,6 +736,9 @@ FreeWaitEventSet(WaitEventSet *set) } #endif + if (set->resowner != NULL) + ResourceOwnerForgetWES(set->resowner, set); + pfree(set); } diff --git a/src/backend/storage/lmgr/condition_variable.c b/src/backend/storage/lmgr/condition_variable.c index 37b6a4eecd..fcc92138fe 100644 --- a/src/backend/storage/lmgr/condition_variable.c +++ b/src/backend/storage/lmgr/condition_variable.c @@ -70,7 +70,7 @@ ConditionVariablePrepareToSleep(ConditionVariable *cv) { WaitEventSet *new_event_set; - new_event_set = CreateWaitEventSet(TopMemoryContext, 2); + new_event_set = CreateWaitEventSet(TopMemoryContext, NULL, 2); AddWaitEventToSet(new_event_set, WL_LATCH_SET, PGINVALID_SOCKET, MyLatch, NULL); AddWaitEventToSet(new_event_set, WL_EXIT_ON_PM_DEATH, PGINVALID_SOCKET, diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.c index 8bc2c4e9ea..237ca9fa30 100644 --- a/src/backend/utils/resowner/resowner.c +++ b/src/backend/utils/resowner/resowner.c @@ -128,6 +128,7 @@ typedef struct ResourceOwnerData ResourceArray filearr; /* open temporary files */ ResourceArray dsmarr; /* dynamic shmem segments */ ResourceArray jitarr; /* JIT contexts */ + ResourceArray wesarr; /* wait event sets */ /* We can remember up to MAX_RESOWNER_LOCKS references to local locks. */ int nlocks; /* number of owned locks */ @@ -175,6 +176,7 @@ static void PrintTupleDescLeakWarning(TupleDesc tupdesc); static void PrintSnapshotLeakWarning(Snapshot snapshot); static void PrintFileLeakWarning(File file); static void PrintDSMLeakWarning(dsm_segment *seg); +static void PrintWESLeakWarning(WaitEventSet *events); /***************************************************************************** @@ -444,6 +446,7 @@ ResourceOwnerCreate(ResourceOwner parent, const char *name) ResourceArrayInit(&(owner->filearr), FileGetDatum(-1)); ResourceArrayInit(&(owner->dsmarr), PointerGetDatum(NULL)); ResourceArrayInit(&(owner->jitarr), PointerGetDatum(NULL)); + ResourceArrayInit(&(owner->wesarr), PointerGetDatum(NULL)); return owner; } @@ -553,6 +556,16 @@ ResourceOwnerReleaseInternal(ResourceOwner owner, jit_release_context(context); } + + /* Ditto for wait event sets */ + while (ResourceArrayGetAny(&(owner->wesarr), &foundres)) + { + WaitEventSet *event = (WaitEventSet *) DatumGetPointer(foundres); + + if (isCommit) + PrintWESLeakWarning(event); + FreeWaitEventSet(event); + } } else if (phase == RESOURCE_RELEASE_LOCKS) { @@ -725,6 +738,7 @@ ResourceOwnerDelete(ResourceOwner owner) Assert(owner->filearr.nitems == 0); Assert(owner->dsmarr.nitems == 0); Assert(owner->jitarr.nitems == 0); + Assert(owner->wesarr.nitems == 0); Assert(owner->nlocks == 0 || owner->nlocks == MAX_RESOWNER_LOCKS + 1); /* @@ -752,6 +766,7 @@ ResourceOwnerDelete(ResourceOwner owner) ResourceArrayFree(&(owner->filearr)); ResourceArrayFree(&(owner->dsmarr)); ResourceArrayFree(&(owner->jitarr)); + ResourceArrayFree(&(owner->wesarr)); pfree(owner); } @@ -1370,3 +1385,55 @@ ResourceOwnerForgetJIT(ResourceOwner owner, Datum handle) elog(ERROR, "JIT context %p is not owned by resource owner %s", DatumGetPointer(handle), owner->name); } + +/* + * wait event set reference array. + * + * This is separate from actually inserting an entry because if we run out + * of memory, it's critical to do so *before* acquiring the resource. + */ +void +ResourceOwnerEnlargeWESs(ResourceOwner owner) +{ + ResourceArrayEnlarge(&(owner->wesarr)); +} + +/* + * Remember that a wait event set is owned by a ResourceOwner + * + * Caller must have previously done ResourceOwnerEnlargeWESs() + */ +void +ResourceOwnerRememberWES(ResourceOwner owner, WaitEventSet *events) +{ + ResourceArrayAdd(&(owner->wesarr), PointerGetDatum(events)); +} + +/* + * Forget that a wait event set is owned by a ResourceOwner + */ +void +ResourceOwnerForgetWES(ResourceOwner owner, WaitEventSet *events) +{ + /* + * XXXX: There's no property to show as an identier of a wait event set, + * use its pointer instead. + */ + if (!ResourceArrayRemove(&(owner->wesarr), PointerGetDatum(events))) + elog(ERROR, "wait event set %p is not owned by resource owner %s", + events, owner->name); +} + +/* + * Debugging subroutine + */ +static void +PrintWESLeakWarning(WaitEventSet *events) +{ + /* + * XXXX: There's no property to show as an identier of a wait event set, + * use its pointer instead. + */ + elog(WARNING, "wait event set leak: %p still referenced", + events); +} diff --git a/src/include/storage/latch.h b/src/include/storage/latch.h index 46ae56cae3..b1b8375768 100644 --- a/src/include/storage/latch.h +++ b/src/include/storage/latch.h @@ -101,6 +101,7 @@ #define LATCH_H #include <signal.h> +#include "utils/resowner.h" /* * Latch structure should be treated as opaque and only accessed through @@ -163,7 +164,8 @@ extern void DisownLatch(Latch *latch); extern void SetLatch(Latch *latch); extern void ResetLatch(Latch *latch); -extern WaitEventSet *CreateWaitEventSet(MemoryContext context, int nevents); +extern WaitEventSet *CreateWaitEventSet(MemoryContext context, + ResourceOwner res, int nevents); extern void FreeWaitEventSet(WaitEventSet *set); extern int AddWaitEventToSet(WaitEventSet *set, uint32 events, pgsocket fd, Latch *latch, void *user_data); diff --git a/src/include/utils/resowner_private.h b/src/include/utils/resowner_private.h index a781a7a2aa..7d19dadd57 100644 --- a/src/include/utils/resowner_private.h +++ b/src/include/utils/resowner_private.h @@ -18,6 +18,7 @@ #include "storage/dsm.h" #include "storage/fd.h" +#include "storage/latch.h" #include "storage/lock.h" #include "utils/catcache.h" #include "utils/plancache.h" @@ -95,4 +96,11 @@ extern void ResourceOwnerRememberJIT(ResourceOwner owner, extern void ResourceOwnerForgetJIT(ResourceOwner owner, Datum handle); +/* support for wait event set management */ +extern void ResourceOwnerEnlargeWESs(ResourceOwner owner); +extern void ResourceOwnerRememberWES(ResourceOwner owner, + WaitEventSet *); +extern void ResourceOwnerForgetWES(ResourceOwner owner, + WaitEventSet *); + #endif /* RESOWNER_PRIVATE_H */ -- 2.18.4 ----Next_Part(Thu_Jul__2_11_14_48_2020_705)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v5-0002-Infrastructure-for-asynchronous-execution.patch" ^ permalink raw reply [nested|flat] 3+ messages in thread
* Disallow changing slot's failover option in transaction block @ 2024-04-16 06:32 Zhijie Hou (Fujitsu) <[email protected]> 2024-04-16 07:29 ` Re: Disallow changing slot's failover option in transaction block Bertrand Drouvot <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Zhijie Hou (Fujitsu) @ 2024-04-16 06:32 UTC (permalink / raw) To: pgsql-hackers; +Cc: Hayato Kuroda (Fujitsu) <[email protected]>; Amit Kapila <[email protected]> Hi, Kuroda-San reported an issue off-list that: If user execute ALTER SUBSCRIPTION SET (failover) command inside a txn block and rollback, only the subscription option change can be rolled back, while the replication slot's failover change is preserved. This is because ALTER SUBSCRIPTION SET (failover) command internally executes the replication command ALTER_REPLICATION_SLOT to change the replication slot's failover property, but this replication command execution cannot be rollback. To fix it, I think we can prevent user from executing ALTER SUBSCRIPTION set (failover) inside a txn block, which is also consistent to the ALTER SUBSCRIPTION REFRESH/DROP SUBSCRIPTION command. Attach a small patch to address this. Best Regards, Hou Zhijie Attachments: [application/octet-stream] v1-0001-Disallow-alter-subscription-s-failover-option-ins.patch (3.0K, ../../OS0PR01MB57165542B09DFA4943830BF294082@OS0PR01MB5716.jpnprd01.prod.outlook.com/2-v1-0001-Disallow-alter-subscription-s-failover-option-ins.patch) download | inline diff: From d792ab2fa2aefb8842774aa44478eebd35851abf Mon Sep 17 00:00:00 2001 From: Hou Zhijie <[email protected]> Date: Tue, 16 Apr 2024 11:31:32 +0800 Subject: [PATCH v1] Disallow alter subscription's failover option inside a transaction block Disallow ALTER SUBSCRIPTION in a transaction block when the replication slot is to be altered, since that cannot be rolled back. --- src/backend/commands/subscriptioncmds.c | 9 +++++++++ src/test/regress/expected/subscription.out | 5 +++++ src/test/regress/sql/subscription.sql | 5 +++++ 3 files changed, 19 insertions(+) diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c index 5a47fa984d..9fa5f4e5b4 100644 --- a/src/backend/commands/subscriptioncmds.c +++ b/src/backend/commands/subscriptioncmds.c @@ -1267,6 +1267,15 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt, errmsg("cannot set %s for enabled subscription", "failover"))); + /* + * Since altering a replication slot is not transactional, + * rolling back the transaction leaves the altered + * replication slot. So we cannot run ALTER SUBSCRIPTION + * inside a transaction block if altering a replication + * slot's failover option. + */ + PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION ... SET (failover)"); + values[Anum_pg_subscription_subfailover - 1] = BoolGetDatum(opts.failover); replaces[Anum_pg_subscription_subfailover - 1] = true; diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out index 1eee6b17b8..0e6c408be3 100644 --- a/src/test/regress/expected/subscription.out +++ b/src/test/regress/expected/subscription.out @@ -472,6 +472,11 @@ REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3; SET SESSION AUTHORIZATION regress_subscription_user3; ALTER SUBSCRIPTION regress_testsub RENAME TO regress_testsub2; ERROR: permission denied for database regression +-- fail - cannot do ALTER SUBSCRIPTION SET (failover) inside transaction block +BEGIN; +ALTER SUBSCRIPTION regress_testsub SET (failover); +ERROR: ALTER SUBSCRIPTION ... SET (failover) cannot run inside a transaction block +COMMIT; -- ok, owning it is enough for this stuff ALTER SUBSCRIPTION regress_testsub SET (slot_name = NONE); DROP SUBSCRIPTION regress_testsub; diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql index 1b2a23ba7b..07413d7e81 100644 --- a/src/test/regress/sql/subscription.sql +++ b/src/test/regress/sql/subscription.sql @@ -333,6 +333,11 @@ REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3; SET SESSION AUTHORIZATION regress_subscription_user3; ALTER SUBSCRIPTION regress_testsub RENAME TO regress_testsub2; +-- fail - cannot do ALTER SUBSCRIPTION SET (failover) inside transaction block +BEGIN; +ALTER SUBSCRIPTION regress_testsub SET (failover); +COMMIT; + -- ok, owning it is enough for this stuff ALTER SUBSCRIPTION regress_testsub SET (slot_name = NONE); DROP SUBSCRIPTION regress_testsub; -- 2.31.1 ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Disallow changing slot's failover option in transaction block 2024-04-16 06:32 Disallow changing slot's failover option in transaction block Zhijie Hou (Fujitsu) <[email protected]> @ 2024-04-16 07:29 ` Bertrand Drouvot <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Bertrand Drouvot @ 2024-04-16 07:29 UTC (permalink / raw) To: Zhijie Hou (Fujitsu) <[email protected]>; +Cc: pgsql-hackers; Hayato Kuroda (Fujitsu) <[email protected]>; Amit Kapila <[email protected]> Hi, On Tue, Apr 16, 2024 at 06:32:11AM +0000, Zhijie Hou (Fujitsu) wrote: > Hi, > > Kuroda-San reported an issue off-list that: > > If user execute ALTER SUBSCRIPTION SET (failover) command inside a txn block > and rollback, only the subscription option change can be rolled back, while the > replication slot's failover change is preserved. Nice catch, thanks! > To fix it, I think we can prevent user from executing ALTER SUBSCRIPTION set > (failover) inside a txn block, which is also consistent to the ALTER > SUBSCRIPTION REFRESH/DROP SUBSCRIPTION command. Attach a small > patch to address this. Agree. The patch looks pretty straightforward to me. Worth to add this case in the doc? (where we already mention that "Commands ALTER SUBSCRIPTION ... REFRESH PUBLICATION and ALTER SUBSCRIPTION ... {SET|ADD|DROP} PUBLICATION ... with refresh option as true cannot be executed inside a transaction block" Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2024-04-16 07:29 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2017-05-22 03:42 [PATCH v5 1/3] Allow wait event set to be registered to resource owner Kyotaro Horiguchi <[email protected]> 2024-04-16 06:32 Disallow changing slot's failover option in transaction block Zhijie Hou (Fujitsu) <[email protected]> 2024-04-16 07:29 ` Re: Disallow changing slot's failover option in transaction block Bertrand Drouvot <[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