From 991c5a4a14a841123237cd370fc1ec4756fad352 Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi Date: Tue, 25 Oct 2016 18:01:56 +0900 Subject: [PATCH 5/7] Use resource owner to prevent wait event set from leaking Wait event sets created for async execution can live for some iterations so it leaks in the case of errors during the iterations. This commit uses resource owner to prevent such leaks. --- src/backend/executor/execAsync.c | 28 ++++++++++++++-- src/backend/storage/ipc/latch.c | 19 ++++++++++- src/backend/utils/resowner/resowner.c | 63 +++++++++++++++++++++++++++++++++++ src/include/utils/resowner_private.h | 8 +++++ 4 files changed, 114 insertions(+), 4 deletions(-) diff --git a/src/backend/executor/execAsync.c b/src/backend/executor/execAsync.c index 33496a9..40e3f67 100644 --- a/src/backend/executor/execAsync.c +++ b/src/backend/executor/execAsync.c @@ -20,6 +20,7 @@ #include "miscadmin.h" #include "pgstat.h" #include "storage/latch.h" +#include "utils/resowner_private.h" static bool ExecAsyncEventWait(EState *estate, long timeout); static bool ExecAsyncConfigureWait(EState *estate, PendingAsyncRequest *areq, @@ -277,6 +278,8 @@ ExecAsyncEventWait(EState *estate, long timeout) if (estate->es_wait_event_set == NULL) { + ResourceOwner savedOwner; + /* * Allow for a few extra events without reinitializing. It * doesn't seem worth the complexity of doing anything very @@ -284,9 +287,28 @@ ExecAsyncEventWait(EState *estate, long timeout) * of external FDs are likely to run afoul of kernel limits anyway. */ estate->es_allocated_fd_events = estate->es_total_fd_events + 16; - estate->es_wait_event_set = - CreateWaitEventSet(estate->es_query_cxt, - estate->es_allocated_fd_events + 1); + + /* + * The wait event set created here should be released in case of + * error. + */ + savedOwner = CurrentResourceOwner; + CurrentResourceOwner = TopTransactionResourceOwner; + + PG_TRY(); + { + estate->es_wait_event_set = + CreateWaitEventSet(estate->es_query_cxt, + estate->es_allocated_fd_events + 1); + } + PG_CATCH(); + { + CurrentResourceOwner = savedOwner; + PG_RE_THROW(); + } + PG_END_TRY(); + + CurrentResourceOwner = savedOwner; AddWaitEventToSet(estate->es_wait_event_set, WL_LATCH_SET, PGINVALID_SOCKET, MyLatch, NULL); reinit = true; diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c index d45a41d..3b64e83 100644 --- a/src/backend/storage/ipc/latch.c +++ b/src/backend/storage/ipc/latch.c @@ -62,6 +62,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" @@ -90,6 +91,7 @@ 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. @@ -324,7 +326,13 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, pgsocket sock, int ret = 0; int rc; WaitEvent event; - WaitEventSet *set = CreateWaitEventSet(CurrentMemoryContext, 3); + WaitEventSet *set; + ResourceOwner savedOwner = CurrentResourceOwner; + + /* This function doesn't need resowner for event set */ + CurrentResourceOwner = NULL; + set = CreateWaitEventSet(CurrentMemoryContext, 3); + CurrentResourceOwner = savedOwner; if (wakeEvents & WL_TIMEOUT) Assert(timeout >= 0); @@ -488,6 +496,9 @@ CreateWaitEventSet(MemoryContext context, int nevents) char *data; Size sz = 0; + if (CurrentResourceOwner) + ResourceOwnerEnlargeWESs(CurrentResourceOwner); + /* * 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 @@ -547,6 +558,9 @@ CreateWaitEventSet(MemoryContext context, int nevents) StaticAssertStmt(WSA_INVALID_EVENT == NULL, ""); #endif + set->resowner = CurrentResourceOwner; + if (CurrentResourceOwner) + ResourceOwnerRememberWES(set->resowner, set); return set; } @@ -582,6 +596,9 @@ FreeWaitEventSet(WaitEventSet *set) } #endif + if (set->resowner != NULL) + ResourceOwnerForgetWES(set->resowner, set); + pfree(set); } diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.c index af46d78..34c7e37 100644 --- a/src/backend/utils/resowner/resowner.c +++ b/src/backend/utils/resowner/resowner.c @@ -124,6 +124,7 @@ typedef struct ResourceOwnerData ResourceArray snapshotarr; /* snapshot references */ ResourceArray filearr; /* open temporary files */ ResourceArray dsmarr; /* dynamic shmem segments */ + ResourceArray wesarr; /* wait event sets */ /* We can remember up to MAX_RESOWNER_LOCKS references to local locks. */ int nlocks; /* number of owned locks */ @@ -169,6 +170,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); /***************************************************************************** @@ -437,6 +439,7 @@ ResourceOwnerCreate(ResourceOwner parent, const char *name) ResourceArrayInit(&(owner->snapshotarr), PointerGetDatum(NULL)); ResourceArrayInit(&(owner->filearr), FileGetDatum(-1)); ResourceArrayInit(&(owner->dsmarr), PointerGetDatum(NULL)); + ResourceArrayInit(&(owner->wesarr), PointerGetDatum(NULL)); return owner; } @@ -552,6 +555,16 @@ ResourceOwnerReleaseInternal(ResourceOwner owner, PrintDSMLeakWarning(res); dsm_detach(res); } + + /* 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) { @@ -699,6 +712,7 @@ ResourceOwnerDelete(ResourceOwner owner) Assert(owner->snapshotarr.nitems == 0); Assert(owner->filearr.nitems == 0); Assert(owner->dsmarr.nitems == 0); + Assert(owner->wesarr.nitems == 0); Assert(owner->nlocks == 0 || owner->nlocks == MAX_RESOWNER_LOCKS + 1); /* @@ -725,6 +739,7 @@ ResourceOwnerDelete(ResourceOwner owner) ResourceArrayFree(&(owner->snapshotarr)); ResourceArrayFree(&(owner->filearr)); ResourceArrayFree(&(owner->dsmarr)); + ResourceArrayFree(&(owner->wesarr)); pfree(owner); } @@ -1267,3 +1282,51 @@ PrintDSMLeakWarning(dsm_segment *seg) elog(WARNING, "dynamic shared memory leak: segment %u still referenced", dsm_segment_handle(seg)); } + +/* + * Make sure there is room for at least one more entry in a ResourceOwner's + * 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 identify a wait event set */ + 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 identify a wait event set */ + elog(WARNING, "wait event set leak: %p still referenced", + events); +} + diff --git a/src/include/utils/resowner_private.h b/src/include/utils/resowner_private.h index 411d08f..0c6979a 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" @@ -88,4 +89,11 @@ extern void ResourceOwnerRememberDSM(ResourceOwner owner, extern void ResourceOwnerForgetDSM(ResourceOwner owner, dsm_segment *); +/* 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.9.2