From ceb84cd6551887f55d540e295e71f82fce157df0 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Wed, 15 Feb 2023 11:17:00 +0200 Subject: [PATCH v13 3/5] Release resources in priority order. XXX: To be squashed with previous commit, but keeping it separate for now to show what changed since last patchset version. Allow each resource kind to specify a release priority, and release the resources in that order. To make that possible, this restricts what you can do between phases. After calling ResourceOwnerRelease, you are no longer allowed to use the resource owner to remember any more resources in it, or to forget any previously remembered resources by calling ResourceOwnerForget. There was one case where that was done previously. At subtransaction commit, AtEOSubXact_Inval() would handle the invalidation messages and call RelationFlushRelation(), which temporarily increased the reference count on the relation being flushed. We now switch to the parent subtransaction's resource owner before calling AtEOSubXact_Inval(), so that there is a valid ResourceOwner to temporarily hold that relcache reference. Other end-of-xact routines make similar calls to AtEOXact_Inval() between release phases, but I didn't see any regression test failures from those, so I'm not sure if they could reach a codepath that needs remembering extra resources. Also, the release callback no longer needs to call ResourceOwnerForget on the resource being released. ResourceOwnerRelease unregisters the resource from the owner before calling the callback. That needed some changes in bufmgr.c and some other files, where releasing the resources previously always called ResourceOwnerForget. Add tests for ResourceOwners in src/test/modules/test_resowner. - XXX in llvmjit.c --- src/backend/access/common/tupdesc.c | 16 +- src/backend/access/transam/xact.c | 15 + src/backend/jit/llvm/llvmjit.c | 12 +- src/backend/storage/buffer/bufmgr.c | 49 ++- src/backend/storage/file/fd.c | 18 +- src/backend/storage/ipc/dsm.c | 13 +- src/backend/utils/cache/catcache.c | 35 +- src/backend/utils/cache/plancache.c | 17 +- src/backend/utils/cache/relcache.c | 24 +- src/backend/utils/resowner/README | 60 +++ src/backend/utils/resowner/resowner.c | 385 ++++++++++++------ src/backend/utils/time/snapmgr.c | 20 +- src/common/cryptohash_openssl.c | 17 +- src/common/hmac_openssl.c | 17 +- src/include/utils/plancache.h | 4 +- src/include/utils/resowner.h | 42 +- src/pl/plpgsql/src/pl_exec.c | 2 +- src/pl/plpgsql/src/pl_handler.c | 6 +- src/test/modules/Makefile | 1 + src/test/modules/test_resowner/.gitignore | 4 + src/test/modules/test_resowner/Makefile | 24 ++ .../test_resowner/expected/test_resowner.out | 197 +++++++++ src/test/modules/test_resowner/meson.build | 37 ++ .../test_resowner/test_resowner--1.0.sql | 30 ++ .../test_resowner/test_resowner.control | 4 + .../test_resowner/test_resowner_basic.c | 210 ++++++++++ .../test_resowner/test_resowner_many.c | 287 +++++++++++++ 27 files changed, 1337 insertions(+), 209 deletions(-) create mode 100644 src/test/modules/test_resowner/.gitignore create mode 100644 src/test/modules/test_resowner/Makefile create mode 100644 src/test/modules/test_resowner/expected/test_resowner.out create mode 100644 src/test/modules/test_resowner/meson.build create mode 100644 src/test/modules/test_resowner/test_resowner--1.0.sql create mode 100644 src/test/modules/test_resowner/test_resowner.control create mode 100644 src/test/modules/test_resowner/test_resowner_basic.c create mode 100644 src/test/modules/test_resowner/test_resowner_many.c diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c index a4b830d307f..de61541796b 100644 --- a/src/backend/access/common/tupdesc.c +++ b/src/backend/access/common/tupdesc.c @@ -39,9 +39,9 @@ static void ResOwnerPrintTupleDescLeakWarning(Datum res); static ResourceOwnerFuncs tupdesc_resowner_funcs = { - /* relcache references */ .name = "tupdesc reference", - .phase = RESOURCE_RELEASE_AFTER_LOCKS, + .release_phase = RESOURCE_RELEASE_AFTER_LOCKS, + .release_priority = RELEASE_PRIO_TUPDESC_REFS, .ReleaseResource = ResOwnerReleaseTupleDesc, .PrintLeakWarning = ResOwnerPrintTupleDescLeakWarning }; @@ -939,13 +939,17 @@ BuildDescFromLists(List *names, List *types, List *typmods, List *collations) } -/* - * ResourceOwner callbacks - */ +/* ResourceOwner callbacks */ + static void ResOwnerReleaseTupleDesc(Datum res) { - DecrTupleDescRefCount((TupleDesc) DatumGetPointer(res)); + TupleDesc tupdesc = (TupleDesc) DatumGetPointer(res); + + /* Like DecrTupleDescRefCount, but don't call ResourceOwnerForget() */ + Assert(tupdesc->tdrefcount > 0); + if (--tupdesc->tdrefcount == 0) + FreeTupleDesc(tupdesc); } static void diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index b8764012607..aba2bd3b961 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -5180,9 +5180,24 @@ AbortSubTransaction(void) ResourceOwnerRelease(s->curTransactionOwner, RESOURCE_RELEASE_BEFORE_LOCKS, false, false); + AtEOSubXact_RelationCache(false, s->subTransactionId, s->parent->subTransactionId); + + + /* + * AtEOSubXact_Inval sometimes needs to temporarily + * bump the refcount on the relcache entries that it processes. + * We cannot use the subtransaction's resource owner anymore, + * because we've already started releasing it. But we can use + * the parent resource owner. + */ + CurrentResourceOwner = s->parent->curTransactionOwner; + AtEOSubXact_Inval(false); + + CurrentResourceOwner = s->curTransactionOwner; + ResourceOwnerRelease(s->curTransactionOwner, RESOURCE_RELEASE_LOCKS, false, false); diff --git a/src/backend/jit/llvm/llvmjit.c b/src/backend/jit/llvm/llvmjit.c index ab9b8fcfde9..e6265eff3dd 100644 --- a/src/backend/jit/llvm/llvmjit.c +++ b/src/backend/jit/llvm/llvmjit.c @@ -127,9 +127,9 @@ static void ResOwnerPrintJitContextLeakWarning(Datum res); static ResourceOwnerFuncs jit_resowner_funcs = { - /* relcache references */ .name = "LLVM JIT context", - .phase = RESOURCE_RELEASE_BEFORE_LOCKS, + .release_phase = RESOURCE_RELEASE_BEFORE_LOCKS, + .release_priority = RELEASE_PRIO_JIT_CONTEXTS, .ReleaseResource = ResOwnerReleaseJitContext, .PrintLeakWarning = ResOwnerPrintJitContextLeakWarning }; @@ -241,7 +241,8 @@ llvm_release_context(JitContext *context) list_free(llvm_context->handles); llvm_context->handles = NIL; - ResourceOwnerForgetJIT(context->resowner, context); + if (context->resowner) + ResourceOwnerForgetJIT(context->resowner, context); } /* @@ -1294,7 +1295,10 @@ llvm_error_message(LLVMErrorRef error) static void ResOwnerReleaseJitContext(Datum res) { - jit_release_context((JitContext *) DatumGetPointer(res)); + JitContext *context = (JitContext *) DatumGetPointer(res); + + context->resowner = NULL; + jit_release_context(context); } static void diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 8fb05a72fa0..3b1bffd5279 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -215,7 +215,8 @@ static void ResOwnerPrintBufferLeakWarning(Datum res); ResourceOwnerFuncs buffer_resowner_funcs = { .name = "buffer", - .phase = RESOURCE_RELEASE_BEFORE_LOCKS, + .release_phase = RESOURCE_RELEASE_BEFORE_LOCKS, + .release_priority = RELEASE_PRIO_BUFFERS, .ReleaseResource = ResOwnerReleaseBuffer, .PrintLeakWarning = ResOwnerPrintBufferLeakWarning }; @@ -469,6 +470,7 @@ static Buffer ReadBuffer_common(SMgrRelation smgr, char relpersistence, static bool PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy); static void PinBuffer_Locked(BufferDesc *buf); static void UnpinBuffer(BufferDesc *buf); +static void UnpinBufferNoOwner(BufferDesc *buf); static void BufferSync(int flags); static uint32 WaitBufHdrUnlocked(BufferDesc *buf); static int SyncOneBuffer(int buf_id, bool skip_recently_used, @@ -1908,6 +1910,15 @@ PinBuffer_Locked(BufferDesc *buf) */ static void UnpinBuffer(BufferDesc *buf) +{ + Buffer b = BufferDescriptorGetBuffer(buf); + + ResourceOwnerForgetBuffer(CurrentResourceOwner, b); + UnpinBufferNoOwner(buf); +} + +static void +UnpinBufferNoOwner(BufferDesc *buf) { PrivateRefCountEntry *ref; Buffer b = BufferDescriptorGetBuffer(buf); @@ -1915,10 +1926,8 @@ UnpinBuffer(BufferDesc *buf) /* not moving as we're likely deleting it soon anyway */ ref = GetPrivateRefCountEntry(b, false); Assert(ref != NULL); - - ResourceOwnerForgetBuffer(CurrentResourceOwner, b); - Assert(ref->refcount > 0); + ref->refcount--; if (ref->refcount == 0) { @@ -4009,16 +4018,17 @@ ReleaseBuffer(Buffer buffer) if (!BufferIsValid(buffer)) elog(ERROR, "bad buffer ID: %d", buffer); + ResourceOwnerForgetBuffer(CurrentResourceOwner, buffer); + if (BufferIsLocal(buffer)) { - ResourceOwnerForgetBuffer(CurrentResourceOwner, buffer); - Assert(LocalRefCount[-buffer - 1] > 0); LocalRefCount[-buffer - 1]--; - return; } - - UnpinBuffer(GetBufferDescriptor(buffer - 1)); + else + { + UnpinBufferNoOwner(GetBufferDescriptor(buffer - 1)); + } } /* @@ -5100,13 +5110,26 @@ TestForOldSnapshot_impl(Snapshot snapshot, Relation relation) errmsg("snapshot too old"))); } -/* - * ResourceOwner callbacks - */ +/* ResourceOwner callbacks */ + static void ResOwnerReleaseBuffer(Datum res) { - ReleaseBuffer(DatumGetInt32(res)); + Buffer buffer = DatumGetInt32(res); + + /* Like ReleaseBuffer, but don't call ResourceOwnerForgetBuffer */ + if (!BufferIsValid(buffer)) + elog(ERROR, "bad buffer ID: %d", buffer); + + if (BufferIsLocal(buffer)) + { + Assert(LocalRefCount[-buffer - 1] > 0); + LocalRefCount[-buffer - 1]--; + } + else + { + UnpinBufferNoOwner(GetBufferDescriptor(buffer - 1)); + } } static void diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index 7bd716542e7..296da188296 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -356,7 +356,8 @@ static void ResOwnerPrintFileLeakWarning(Datum res); static ResourceOwnerFuncs file_resowner_funcs = { .name = "File", - .phase = RESOURCE_RELEASE_AFTER_LOCKS, + .release_phase = RESOURCE_RELEASE_AFTER_LOCKS, + .release_priority = RELEASE_PRIO_FILES, .ReleaseResource = ResOwnerReleaseFile, .PrintLeakWarning = ResOwnerPrintFileLeakWarning }; @@ -3756,13 +3757,20 @@ data_sync_elevel(int elevel) return data_sync_retry ? elevel : PANIC; } -/* - * ResourceOwner callbacks - */ +/* ResourceOwner callbacks */ + static void ResOwnerReleaseFile(Datum res) { - FileClose((File) DatumGetInt32(res)); + File file = (File) DatumGetInt32(res); + Vfd *vfdP; + + Assert(FileIsValid(file)); + + vfdP = &VfdCache[file]; + vfdP->resowner = NULL; + + FileClose(file); } static void diff --git a/src/backend/storage/ipc/dsm.c b/src/backend/storage/ipc/dsm.c index 61927f7ccc0..21c53bc6ae4 100644 --- a/src/backend/storage/ipc/dsm.c +++ b/src/backend/storage/ipc/dsm.c @@ -150,7 +150,8 @@ static void ResOwnerPrintDSMLeakWarning(Datum res); static ResourceOwnerFuncs dsm_resowner_funcs = { .name = "dynamic shared memory segment", - .phase = RESOURCE_RELEASE_BEFORE_LOCKS, + .release_phase = RESOURCE_RELEASE_BEFORE_LOCKS, + .release_priority = RELEASE_PRIO_DSMS, .ReleaseResource = ResOwnerReleaseDSM, .PrintLeakWarning = ResOwnerPrintDSMLeakWarning }; @@ -1275,13 +1276,15 @@ is_main_region_dsm_handle(dsm_handle handle) return handle & 1; } -/* - * ResourceOwner callbacks - */ +/* ResourceOwner callbacks */ + static void ResOwnerReleaseDSM(Datum res) { - dsm_detach((dsm_segment *) DatumGetPointer(res)); + dsm_segment *seg = (dsm_segment *) res; + + seg->resowner = NULL; + dsm_detach(seg); } static void ResOwnerPrintDSMLeakWarning(Datum res) diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c index af90d1e0134..f05ab9f04fe 100644 --- a/src/backend/utils/cache/catcache.c +++ b/src/backend/utils/cache/catcache.c @@ -95,6 +95,8 @@ static CatCTup *CatalogCacheCreateEntry(CatCache *cache, HeapTuple ntp, uint32 hashValue, Index hashIndex, bool negative); +static void ReleaseCatCacheWithOwner(HeapTuple tuple, ResourceOwner resowner); +static void ReleaseCatCacheListWithOwner(CatCList *list, ResourceOwner resowner); static void CatCacheFreeKeys(TupleDesc tupdesc, int nkeys, int *attnos, Datum *keys); static void CatCacheCopyKeys(TupleDesc tupdesc, int nkeys, int *attnos, @@ -116,7 +118,8 @@ static ResourceOwnerFuncs catcache_resowner_funcs = { /* catcache references */ .name = "catcache reference", - .phase = RESOURCE_RELEASE_AFTER_LOCKS, + .release_phase = RESOURCE_RELEASE_AFTER_LOCKS, + .release_priority = RELEASE_PRIO_CATCACHE_REFS, .ReleaseResource = ResOwnerReleaseCatCache, .PrintLeakWarning = ResOwnerPrintCatCacheLeakWarning }; @@ -125,7 +128,8 @@ static ResourceOwnerFuncs catlistref_resowner_funcs = { /* catcache-list pins */ .name = "catcache list reference", - .phase = RESOURCE_RELEASE_AFTER_LOCKS, + .release_phase = RESOURCE_RELEASE_AFTER_LOCKS, + .release_priority = RELEASE_PRIO_CATCACHE_LIST_REFS, .ReleaseResource = ResOwnerReleaseCatCacheList, .PrintLeakWarning = ResOwnerPrintCatCacheListLeakWarning }; @@ -1470,6 +1474,12 @@ SearchCatCacheMiss(CatCache *cache, */ void ReleaseCatCache(HeapTuple tuple) +{ + ReleaseCatCacheWithOwner(tuple, CurrentResourceOwner); +} + +static void +ReleaseCatCacheWithOwner(HeapTuple tuple, ResourceOwner resowner) { CatCTup *ct = (CatCTup *) (((char *) tuple) - offsetof(CatCTup, tuple)); @@ -1479,7 +1489,8 @@ ReleaseCatCache(HeapTuple tuple) Assert(ct->refcount > 0); ct->refcount--; - ResourceOwnerForgetCatCacheRef(CurrentResourceOwner, &ct->tuple); + if (resowner) + ResourceOwnerForgetCatCacheRef(CurrentResourceOwner, &ct->tuple); if ( #ifndef CATCACHE_FORCE_RELEASE @@ -1818,12 +1829,19 @@ SearchCatCacheList(CatCache *cache, */ void ReleaseCatCacheList(CatCList *list) +{ + ReleaseCatCacheListWithOwner(list, CurrentResourceOwner); +} + +static void +ReleaseCatCacheListWithOwner(CatCList *list, ResourceOwner resowner) { /* Safety checks to ensure we were handed a cache entry */ Assert(list->cl_magic == CL_MAGIC); Assert(list->refcount > 0); list->refcount--; - ResourceOwnerForgetCatCacheListRef(CurrentResourceOwner, list); + if (resowner) + ResourceOwnerForgetCatCacheListRef(CurrentResourceOwner, list); if ( #ifndef CATCACHE_FORCE_RELEASE @@ -2099,13 +2117,12 @@ PrepareToInvalidateCacheTuple(Relation relation, } } -/* - * ResourceOwner callbacks - */ +/* ResourceOwner callbacks */ + static void ResOwnerReleaseCatCache(Datum res) { - ReleaseCatCache((HeapTuple) DatumGetPointer(res)); + ReleaseCatCacheWithOwner((HeapTuple) DatumGetPointer(res), NULL); } static void @@ -2128,7 +2145,7 @@ ResOwnerPrintCatCacheLeakWarning(Datum res) static void ResOwnerReleaseCatCacheList(Datum res) { - ReleaseCatCacheList((CatCList *) DatumGetPointer(res)); + ReleaseCatCacheListWithOwner((CatCList *) DatumGetPointer(res), NULL); } static void diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c index 5fa486c5eb5..b295fcb2c20 100644 --- a/src/backend/utils/cache/plancache.c +++ b/src/backend/utils/cache/plancache.c @@ -119,11 +119,11 @@ static void PlanCacheSysCallback(Datum arg, int cacheid, uint32 hashvalue); static void ResOwnerReleaseCachedPlan(Datum res); static void ResOwnerPrintPlanCacheLeakWarning(Datum res); -/* this is exported for ResourceOwnerReleaseAllPlanCacheRefs() */ -ResourceOwnerFuncs planref_resowner_funcs = +static ResourceOwnerFuncs planref_resowner_funcs = { .name = "plancache reference", - .phase = RESOURCE_RELEASE_AFTER_LOCKS, + .release_phase = RESOURCE_RELEASE_AFTER_LOCKS, + .release_priority = RELEASE_PRIO_PLANCACHE_REFS, .ReleaseResource = ResOwnerReleaseCachedPlan, .PrintLeakWarning = ResOwnerPrintPlanCacheLeakWarning }; @@ -2228,13 +2228,20 @@ ResetPlanCache(void) } /* - * ResourceOwner callbacks + * Release all CachedPlans remembered by 'owner' */ +void +ReleaseAllPlanCacheRefsInOwner(ResourceOwner owner) +{ + ResourceOwnerReleaseAllOfKind(owner, &planref_resowner_funcs); +} + +/* ResourceOwner callbacks */ static void ResOwnerReleaseCachedPlan(Datum res) { - ReleaseCachedPlan((CachedPlan *) DatumGetPointer(res), CurrentResourceOwner); + ReleaseCachedPlan((CachedPlan *) DatumGetPointer(res), NULL); } static void diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index 286155e20ac..26430859934 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -274,6 +274,7 @@ static HTAB *OpClassCache = NULL; /* non-export function prototypes */ +static void RelationCloseCleanup(Relation relation); static void RelationDestroyRelation(Relation relation, bool remember_tupdesc); static void RelationClearRelation(Relation relation, bool rebuild); @@ -2123,7 +2124,8 @@ static void ResOwnerPrintRelCacheLeakWarning(Datum res); static ResourceOwnerFuncs relref_resowner_funcs = { .name = "relcache reference", - .phase = RESOURCE_RELEASE_BEFORE_LOCKS, + .release_phase = RESOURCE_RELEASE_BEFORE_LOCKS, + .release_priority = RELEASE_PRIO_RELCACHE_REFS, .ReleaseResource = ResOwnerReleaseRelation, .PrintLeakWarning = ResOwnerPrintRelCacheLeakWarning }; @@ -2181,6 +2183,12 @@ RelationClose(Relation relation) /* Note: no locking manipulations needed */ RelationDecrementReferenceCount(relation); + RelationCloseCleanup(relation); +} + +static void +RelationCloseCleanup(Relation relation) +{ /* * If the relation is no longer open in this session, we can clean up any * stale partition descriptors it has. This is unlikely, so check to see @@ -6809,7 +6817,7 @@ unlink_initfile(const char *initfilename, int elevel) static void ResOwnerPrintRelCacheLeakWarning(Datum res) { - Relation rel = (Relation) res; + Relation rel = (Relation) DatumGetPointer(res); elog(WARNING, "relcache reference leak: relation \"%s\" not closed", RelationGetRelationName(rel)); @@ -6818,5 +6826,15 @@ ResOwnerPrintRelCacheLeakWarning(Datum res) static void ResOwnerReleaseRelation(Datum res) { - RelationClose((Relation) res); + Relation rel = (Relation) DatumGetPointer(res); + + /* + * This reference has already been removed from ther resource owner, so + * decrement reference count without calling + * ResourceOwnerForgetRelationRef. + */ + Assert(rel->rd_refcnt > 0); + rel->rd_refcnt -= 1; + + RelationCloseCleanup((Relation) res); } diff --git a/src/backend/utils/resowner/README b/src/backend/utils/resowner/README index e32f7a27384..2a703a395dc 100644 --- a/src/backend/utils/resowner/README +++ b/src/backend/utils/resowner/README @@ -85,3 +85,63 @@ CurrentResourceOwner must point to the same resource owner that was current when the buffer, lock, or cache reference was acquired. It would be possible to relax this restriction given additional bookkeeping effort, but at present there seems no need. + + +Releasing +--------- + +Releasing the resources of a ResourceOwner happens in three phases: + +1. "Before-locks" resources + +2. Locks + +3. "After-locks" resources + +Each resource type specifies whether it needs to be released before or after +locks. Each resource type also has a priority, which determines the order +that the resources are released in. Note that the phases are performed fully +for the whole tree of resource owners, before moving to the next phase, but +the priority within each phase only determines the order within that +ResourceOwner. Child resource owners are always handled before the parent, +within each phase. + +For example, imagine that you have two ResourceOwners, parent and child, +as follows: + +Parent + parent resource BEFORE_LOCKS priority 1 + parent resource BEFORE_LOCKS priority 2 + parent resource AFTER_LOCKS priority 10001 + parent resource AFTER_LOCKS priority 10002 + Child + child resource BEFORE_LOCKS priority 1 + child resource BEFORE_LOCKS priority 2 + child resource AFTER_LOCKS priority 10001 + child resource AFTER_LOCKS priority 10002 + +These resources would be released in the following order: + +child resource BEFORE_LOCKS priority 1 +child resource BEFORE_LOCKS priority 2 +parent resource BEFORE_LOCKS priority 1 +parent resource BEFORE_LOCKS priority 2 +(locks) +child resource AFTER_LOCKS priority 10001 +child resource AFTER_LOCKS priority 10002 +parent resource AFTER_LOCKS priority 10001 +parent resource AFTER_LOCKS priority 10002 + +To release all the resources, you need to call ResourceOwnerRelease() three +times, once for each phase. You may perform additional tasks between the +phases, but after the first call to ResourceOwnerRelease(), you cannot use +the ResourceOwner to remember any more resources. You also cannot call +ResourceOwnerForget on the resource owner to release any previously +remembered resources "in retail", after you have started the release process. + +Normally, you are expected to call ResourceOwnerForget on every resource so +that at commit, the ResourceOwner is empty (locks are an exception). If there +are any resources still held at commit, ResourceOwnerRelease will call the +PrintLeakWarning callback on each such resource. At abort, however, we truly +rely on the ResourceOwner mechanism and it is normal that there are resources +to be released. diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.c index 7e594e3f2e4..6de09913bd0 100644 --- a/src/backend/utils/resowner/resowner.c +++ b/src/backend/utils/resowner/resowner.c @@ -6,7 +6,31 @@ * Query-lifespan resources are tracked by associating them with * ResourceOwner objects. This provides a simple mechanism for ensuring * that such resources are freed at the right time. - * See utils/resowner/README for more info. + * See utils/resowner/README for more info on how to use it. + * + * The implementation consists of a small fixed-size array and a hash table. + * New entries are inserted to the fixed-size array, and when the array fills + * up, all the entries are moved to the hash table. This way, the array + * always contains a few most recently remembered references. To find a + * particular reference, you need to search both the array and the hash table. + * + * The most frequent usage is that a resource is remembered, and forgotten + * shortly thereafter. For example, pin a buffer, read one tuple from it, + * release the pin. Linearly scanning the small array handles that case + * efficiently. However, some resources are held for a longer time, and + * sometimes a lot of resources need to be held simultaneously. The hash + * table handles those cases. + * + * When it's time to release the resources, we sort them according to the + * release-priority of each resource, and release them in that order. + * + * Local lock references are special, they are not stored in the array or the + * hash table. Instead, each resource owner has a separate small cache of + * locks it owns. The lock manager has the same information in its local lock + * hash table, and we fall back on that if cache overflows, but traversing the + * hash table is slower when there are a lot of locks belonging to other + * resource owners. This is to speed up bulk releasing or reassigning locks + * from a resource owner to its parent. * * * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group @@ -25,7 +49,6 @@ #include "storage/predicate.h" #include "storage/proc.h" #include "utils/memutils.h" -#include "utils/plancache.h" #include "utils/resowner.h" /* @@ -41,33 +64,34 @@ typedef struct ResourceElem } ResourceElem; /* - * Size of the small fixed-size array to hold most-recently remembered resources. + * Size of the fixed-size array to hold most-recently remembered resources. */ #define RESOWNER_ARRAY_SIZE 32 /* - * Initially allocated size of a ResourceOwner's hash. Must be power of two since - * we'll use (capacity - 1) as mask for hashing. + * Initially allocated size of a ResourceOwner's hash table. Must be power of + * two because we use (capacity - 1) as mask for hashing. */ #define RESOWNER_HASH_INIT_SIZE 64 /* - * How many items may be stored in a hash of given capacity. - * When this number is reached, we must resize. + * How many items may be stored in a hash table of given capacity. When this + * number is reached, we must resize. + * + * The hash table must always have enough free space that we can copy the + * entries from the array to it, in ResouceOwnerSort. We also insist that the + * initial size is large enough that we don't hit the max size immediately + * when it's created. Aside from those limitations, 0.75 is a reasonable fill + * factor. */ -#define RESOWNER_HASH_MAX_ITEMS(capacity) ((capacity)/4 * 3) +#define RESOWNER_HASH_MAX_ITEMS(capacity) \ + Min(capacity - RESOWNER_ARRAY_SIZE, (capacity)/4 * 3) -StaticAssertDecl(RESOWNER_HASH_MAX_ITEMS(RESOWNER_HASH_INIT_SIZE) > RESOWNER_ARRAY_SIZE, +StaticAssertDecl(RESOWNER_HASH_MAX_ITEMS(RESOWNER_HASH_INIT_SIZE) >= RESOWNER_ARRAY_SIZE, "initial hash size too small compared to array size"); /* - * To speed up bulk releasing or reassigning locks from a resource owner to - * its parent, each resource owner has a small cache of locks it owns. The - * lock manager has the same information in its local lock hash table, and - * we fall back on that if cache overflows, but traversing the hash table - * is slower when there are a lot of locks belonging to other resource owners. - * - * MAX_RESOWNER_LOCKS is the size of the per-resource owner cache. It's + * MAX_RESOWNER_LOCKS is the size of the per-resource owner locks cache. It's * chosen based on some testing with pg_dump with a large schema. When the * tests were done (on 9.2), resource owners in a pg_dump run contained up * to 9 locks, regardless of the schema size, except for the top resource @@ -88,33 +112,35 @@ typedef struct ResourceOwnerData ResourceOwner nextchild; /* next child of same parent */ const char *name; /* name (just for debugging) */ + bool releasing; /* has ResourceOwnerRelease been called? */ + /* - * These structs keep track of the objects registered with this owner. + * The fixed-size array for recent resources. * - * We manage a small set of references by keeping them in a simple array. - * When the array gets full, all the elements in the array are moved to a - * hash table. This way, the array always contains a few most recently - * remembered references. To find a particular reference, you need to - * search both the array and the hash table. + * If 'releasing' is set, the contents are sorted by release priority. */ - ResourceElem arr[RESOWNER_ARRAY_SIZE]; uint32 narr; /* how many items are stored in the array */ + ResourceElem arr[RESOWNER_ARRAY_SIZE]; /* * The hash table. Uses open-addressing. 'nhash' is the number of items * present; if it would exceed 'grow_at', we enlarge it and re-hash. * 'grow_at' should be rather less than 'capacity' so that we don't waste * too much time searching for empty slots. + * + * If 'releasing' is set, the contents are no longer hashed, but sorted by + * release priority. The first 'nhash' elements are occupied, the rest + * are empty. */ ResourceElem *hash; uint32 nhash; /* how many items are stored in the hash */ uint32 capacity; /* allocated length of hash[] */ uint32 grow_at; /* grow hash when reach this */ - /* We can remember up to MAX_RESOWNER_LOCKS references to local locks. */ - int nlocks; /* number of owned locks */ + /* The local locks cache. */ + uint32 nlocks; /* number of owned locks */ LOCALLOCK *locks[MAX_RESOWNER_LOCKS]; /* list of owned locks */ -} ResourceOwnerData; +} ResourceOwnerData; /***************************************************************************** @@ -155,6 +181,8 @@ static ResourceReleaseCallbackItem *ResourceRelease_callbacks = NULL; static inline uint32 hash_resource_elem(Datum value, ResourceOwnerFuncs *kind); static void ResourceOwnerAddToHash(ResourceOwner owner, Datum value, ResourceOwnerFuncs *kind); +static int resource_priority_cmp(const void *a, const void *b); +static void ResourceOwnerSort(ResourceOwner owner); static void ResourceOwnerReleaseAll(ResourceOwner owner, ResourceReleasePhase phase, bool printLeakWarnings); @@ -186,12 +214,12 @@ hash_resource_elem(Datum value, ResourceOwnerFuncs *kind) static void ResourceOwnerAddToHash(ResourceOwner owner, Datum value, ResourceOwnerFuncs *kind) { - /* Insert into first free slot at or after hash location. */ uint32 mask = owner->capacity - 1; uint32 idx; Assert(kind != NULL); + /* Insert into first free slot at or after hash location. */ idx = hash_resource_elem(value, kind) & mask; for (;;) { @@ -205,84 +233,140 @@ ResourceOwnerAddToHash(ResourceOwner owner, Datum value, ResourceOwnerFuncs *kin } /* - * Call the ReleaseResource callback on entries with given 'phase'. + * Comparison function to sort by release phase and priority + */ +static int +resource_priority_cmp(const void *a, const void *b) +{ + const ResourceElem *ra = (const ResourceElem *) a; + const ResourceElem *rb = (const ResourceElem *) b; + + /* Note: reverse order */ + if (ra->kind->release_phase == rb->kind->release_phase) + { + if (ra->kind->release_priority == rb->kind->release_priority) + return 0; + else if (ra->kind->release_priority > rb->kind->release_priority) + return -1; + else + return 1; + } + else if (ra->kind->release_phase > rb->kind->release_phase) + return -1; + else + return 1; +} + +/* + * Sort resources in reverse release priority. + * + * If the hash table is in use, all the elements from the fixed-size array are + * moved to the hash table, and then the hash table is sorted. If there is no + * hash table, then the fixed-size array is sorted directly. In either case, + * the result is one sorted array that contains all the resources. */ static void -ResourceOwnerReleaseAll(ResourceOwner owner, ResourceReleasePhase phase, - bool printLeakWarnings) +ResourceOwnerSort(ResourceOwner owner) { - bool found; - int capacity; + ResourceElem *items; + uint32 nitems; - /* - * First handle all the entries in the array. - * - * Note that ReleaseResource() will call ResourceOwnerForget) and remove - * the entry from our array, so we just have to iterate till there is - * nothing left to remove. - */ - do + if (!owner->hash) { - found = false; - for (int i = 0; i < owner->narr; i++) + items = owner->arr; + nitems = owner->narr; + } + else + { + /* + * Compact the hash table, so that all the elements are in the + * beginning of the 'hash' array, with no empty elements. + */ + uint32 dst = 0; + + for (int idx = 0; idx < owner->capacity; idx++) { - if (owner->arr[i].kind->phase == phase) + if (owner->hash[idx].kind != NULL) { - Datum value = owner->arr[i].item; - ResourceOwnerFuncs *kind = owner->arr[i].kind; - - if (printLeakWarnings) - kind->PrintLeakWarning(value); - kind->ReleaseResource(value); - found = true; + if (dst != idx) + owner->hash[dst] = owner->hash[idx]; + dst++; } } /* - * If any resources were released, check again because some of the - * elements might have been moved by the callbacks. We don't want to - * miss them. + * Move all entries from the fixed-size array to 'hash'. + * + * RESOWNER_HASH_MAX_ITEMS is defined so that there is always enough + * free space to move all the elements from the fixed-size array to + * the hash. */ - } while (found && owner->narr > 0); + Assert(dst + owner->narr <= owner->capacity); + for (int idx = 0; idx < owner->narr; idx++) + { + owner->hash[dst] = owner->arr[idx]; + dst++; + } + Assert(dst == owner->nhash + owner->narr); + owner->narr = 0; + owner->nhash = dst; + + items = owner->hash; + nitems = owner->nhash; + } + + qsort(items, nitems, sizeof(ResourceElem), resource_priority_cmp); +} + +/* + * Call the ReleaseResource callback on entries with given 'phase'. + */ +static void +ResourceOwnerReleaseAll(ResourceOwner owner, ResourceReleasePhase phase, + bool printLeakWarnings) +{ + ResourceElem *items; + uint32 nitems; + + /* ResourceOwnerSort must've been called already */ + Assert(owner->releasing); + if (!owner->hash) + { + items = owner->arr; + nitems = owner->narr; + } + else + { + Assert(owner->narr == 0); + items = owner->hash; + nitems = owner->nhash; + } /* - * Ok, the array has now been handled. Then the hash. Like with the - * array, ReleaseResource() will remove the entry from the hash. + * The resources are sorted in reverse priority order. Release them + * starting from the end, until we hit the end of the phase that we are + * releasing now. We will continue from there when called again for the + * next phase. */ - do + while (nitems > 0) { - capacity = owner->capacity; - for (int idx = 0; idx < capacity; idx++) - { - while (owner->hash[idx].kind != NULL && - owner->hash[idx].kind->phase == phase) - { - Datum value = owner->hash[idx].item; - ResourceOwnerFuncs *kind = owner->hash[idx].kind; - - if (printLeakWarnings) - kind->PrintLeakWarning(value); - kind->ReleaseResource(value); - - /* - * If the same resource is remembered more than once in this - * resource owner, the ReleaseResource callback might've - * released a different copy of it. Because of that, loop to - * check the same index again. - */ - } - } + uint32 idx = nitems - 1; + Datum value = items[idx].item; + ResourceOwnerFuncs *kind = items[idx].kind; - /* - * It's possible that the callbacks acquired more resources, causing - * the hash table to grow and the existing entries to be moved around. - * If that happened, scan the hash table again, so that we don't miss - * entries that were moved. (XXX: I'm not sure if any of the - * callbacks actually do that, but this is cheap to check, and better - * safe than sorry.) - */ - Assert(owner->capacity >= capacity); - } while (capacity != owner->capacity); + if (kind->release_phase > phase) + break; + Assert(kind->release_phase == phase); + + if (printLeakWarnings) + kind->PrintLeakWarning(value); + kind->ReleaseResource(value); + nitems--; + } + if (!owner->hash) + owner->narr = nitems; + else + owner->nhash = nitems; } @@ -335,10 +419,16 @@ ResourceOwnerCreate(ResourceOwner parent, const char *name) void ResourceOwnerEnlarge(ResourceOwner owner) { + /* Mustn't try to remember more resources after we have already started releasing */ + if (owner->releasing) + elog(ERROR, "ResourceOwnerEnlarge called after release started"); + if (owner->narr < RESOWNER_ARRAY_SIZE) return; /* no work needed */ - /* Is there space in the hash? If not, enlarge it. */ + /* + * Is there space in the hash? If not, enlarge it. + */ if (owner->narr + owner->nhash >= owner->grow_at) { uint32 i, @@ -383,14 +473,11 @@ ResourceOwnerEnlarge(ResourceOwner owner) } /* Move items from the array to the hash */ - Assert(owner->narr == RESOWNER_ARRAY_SIZE); for (int i = 0; i < owner->narr; i++) - { ResourceOwnerAddToHash(owner, owner->arr[i].item, owner->arr[i].kind); - } owner->narr = 0; - Assert(owner->nhash < owner->grow_at); + Assert(owner->nhash <= owner->grow_at); } /* @@ -408,13 +495,19 @@ ResourceOwnerRemember(ResourceOwner owner, Datum value, ResourceOwnerFuncs *kind resowner_trace_counter++, owner, DatumGetUInt64(value), kind->name); #endif + /* + * Mustn't try to remember more resources after we have already started + * releasing. We already checked this in ResourceOwnerEnlarge. + */ + Assert(!owner->releasing); + if (owner->narr >= RESOWNER_ARRAY_SIZE) { /* forgot to call ResourceOwnerEnlarge? */ elog(ERROR, "ResourceOwnerRemember called but array was full"); } - /* Append to linear array. */ + /* Append to the array. */ idx = owner->narr; owner->arr[idx].item = value; owner->arr[idx].kind = kind; @@ -424,22 +517,27 @@ ResourceOwnerRemember(ResourceOwner owner, Datum value, ResourceOwnerFuncs *kind /* * Forget that an object is owned by a ResourceOwner * - * Note: if same resource ID is associated with the ResourceOwner more than once, - * one instance is removed. + * Note: if same resource ID is associated with the ResourceOwner more than + * once, one instance is removed. */ void ResourceOwnerForget(ResourceOwner owner, Datum value, ResourceOwnerFuncs *kind) { - uint32 i, - idx; - #ifdef RESOWNER_TRACE elog(LOG, "FORGET %d: owner %p value " UINT64_FORMAT ", kind: %s", resowner_trace_counter++, owner, DatumGetUInt64(value), kind->name); #endif - /* Search through all items, but check the array first. */ - for (i = 0; i < owner->narr; i++) + /* + * Mustn't call this after we have already started releasing resources. + * (Release callback functions are not allowed to release additional + * resources.) + */ + if (owner->releasing) + elog(ERROR, "ResourceOwnerForget called for %s after release started", kind->name); + + /* Search through all items in the array first. */ + for (uint32 i = 0; i < owner->narr; i++) { if (owner->arr[i].item == value && owner->arr[i].kind == kind) @@ -459,9 +557,10 @@ ResourceOwnerForget(ResourceOwner owner, Datum value, ResourceOwnerFuncs *kind) if (owner->nhash > 0) { uint32 mask = owner->capacity - 1; + uint32 idx; idx = hash_resource_elem(value, kind) & mask; - for (i = 0; i < owner->capacity; i++) + for (uint32 i = 0; i < owner->capacity; i++) { if (owner->hash[idx].item == value && owner->hash[idx].kind == kind) @@ -473,6 +572,7 @@ ResourceOwnerForget(ResourceOwner owner, Datum value, ResourceOwnerFuncs *kind) #ifdef RESOWNER_STATS nhash_lookups++; #endif + return; } idx = (idx + 1) & mask; @@ -513,6 +613,12 @@ ResourceOwnerForget(ResourceOwner owner, Datum value, ResourceOwnerFuncs *kind) * isTopLevel is passed when we are releasing TopTransactionResourceOwner * at completion of a main transaction. This generally means that *all* * resources will be released, and so we can optimize things a bit. + * + * NOTE: After starting the release process, by calling this function, no new + * resources can be remembered in the resource owner. You also cannot call + * ResourceOwnerForget on any previously remembered resources to release + * resources "in retail" after that, you must let the bulk release take care + * of them. */ void ResourceOwnerRelease(ResourceOwner owner, @@ -549,8 +655,32 @@ ResourceOwnerReleaseInternal(ResourceOwner owner, ResourceOwnerReleaseInternal(child, phase, isCommit, isTopLevel); /* - * Make CurrentResourceOwner point to me, so that ReleaseBuffer etc don't - * get confused. + * To release the resources in the right order, sort them by phase and + * priority. + * + * The ReleaseResource callback functions are not allowed to remember or + * forget any other resources after this. Otherwise we lose track of where + * we are in processing the hash/array. + */ + if (!owner->releasing) + { + Assert(phase == RESOURCE_RELEASE_BEFORE_LOCKS); + ResourceOwnerSort(owner); + owner->releasing = true; + } + else + { + /* + * Phase is normally > RESOURCE_RELEASE_BEFORE_LOCKS, if this is not + * the first call to ReleaseOwnerRelease. But if an error happens + * between the release phases, we might get called again for the same + * ResourceOwner from AbortTransaction. + */ + } + + /* + * Make CurrentResourceOwner point to me, so that the release callback + * functions know which resource owner is been released. */ save = CurrentResourceOwner; CurrentResourceOwner = owner; @@ -634,52 +764,53 @@ ResourceOwnerReleaseInternal(ResourceOwner owner, } /* - * ResourceOwnerReleaseAllPlanCacheRefs - * Release the plancache references (only) held by this owner. - * - * We might eventually add similar functions for other resource types, - * but for now, only this is needed. + * ResourceOwnerReleaseAllOfKind + * Release all resources of a certain type held by this owner. */ void -ResourceOwnerReleaseAllPlanCacheRefs(ResourceOwner owner) +ResourceOwnerReleaseAllOfKind(ResourceOwner owner, ResourceOwnerFuncs *kind) { - /* array first */ + /* Mustn't call this after we have already started releasing resources. */ + if (owner->releasing) + elog(ERROR, "ResourceOwnerForget called for %s after release started", kind->name); + + /* + * Temporarily set 'releasing', to prevent calls to ResourceOwnerRemember + * while we're scanning the owner. Enlarging the hash would cause us to + * lose track of the point we're scanning. + */ + owner->releasing = true; + + /* Array first */ for (int i = 0; i < owner->narr; i++) { - if (owner->arr[i].kind == &planref_resowner_funcs) + if (owner->arr[i].kind == kind) { - CachedPlan *planref = (CachedPlan *) DatumGetPointer(owner->arr[i].item); + Datum value = owner->arr[i].item; owner->arr[i] = owner->arr[owner->narr - 1]; owner->narr--; i--; - /* - * pass 'NULL' because we already removed the entry from the - * resowner - */ - ReleaseCachedPlan(planref, NULL); + kind->ReleaseResource(value); } } /* Then hash */ for (int i = 0; i < owner->capacity; i++) { - if (owner->hash[i].kind == &planref_resowner_funcs) + if (owner->hash[i].kind == kind) { - CachedPlan *planref = (CachedPlan *) DatumGetPointer(owner->hash[i].item); + Datum value = owner->hash[i].item; owner->hash[i].item = (Datum) 0; owner->hash[i].kind = NULL; owner->nhash--; - /* - * pass 'NULL' because we already removed the entry from the - * resowner - */ - ReleaseCachedPlan(planref, NULL); + kind->ReleaseResource(value); } } + owner->releasing = false; } /* @@ -857,6 +988,8 @@ ReleaseAuxProcessResources(bool isCommit) ResourceOwnerRelease(AuxProcessResourceOwner, RESOURCE_RELEASE_AFTER_LOCKS, isCommit, true); + /* allow it to be reused */ + AuxProcessResourceOwner->releasing = false; } /* @@ -874,7 +1007,7 @@ ReleaseAuxProcessResourcesCallback(int code, Datum arg) /* * Remember that a Local Lock is owned by a ResourceOwner * - * This is different from the other Remember functions in that the list of + * This is different from the generic ResourceOwnerRemember in that the list of * locks is only a lossy cache. It can hold up to MAX_RESOWNER_LOCKS entries, * and when it overflows, we stop tracking locks. The point of only remembering * only up to MAX_RESOWNER_LOCKS entries is that if a lot of locks are held, diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c index 7da51381cd2..6ab630821a8 100644 --- a/src/backend/utils/time/snapmgr.c +++ b/src/backend/utils/time/snapmgr.c @@ -173,6 +173,7 @@ static List *exportedSnapshots = NIL; /* Prototypes for local functions */ static TimestampTz AlignTimestampToMinuteBoundary(TimestampTz ts); static Snapshot CopySnapshot(Snapshot snapshot); +static void UnregisterSnapshotNoOwner(Snapshot snapshot); static void FreeSnapshot(Snapshot snapshot); static void SnapshotResetXmin(void); @@ -183,7 +184,8 @@ static void ResOwnerPrintSnapshotLeakWarning(Datum res); static ResourceOwnerFuncs snapshot_resowner_funcs = { .name = "snapshot reference", - .phase = RESOURCE_RELEASE_AFTER_LOCKS, + .release_phase = RESOURCE_RELEASE_AFTER_LOCKS, + .release_priority = RELEASE_PRIO_SNAPSHOT_REFS, .ReleaseResource = ResOwnerReleaseSnapshot, .PrintLeakWarning = ResOwnerPrintSnapshotLeakWarning }; @@ -905,11 +907,16 @@ UnregisterSnapshotFromOwner(Snapshot snapshot, ResourceOwner owner) if (snapshot == NULL) return; + ResourceOwnerForgetSnapshot(owner, snapshot); + UnregisterSnapshotNoOwner(snapshot); +} + +static void +UnregisterSnapshotNoOwner(Snapshot snapshot) +{ Assert(snapshot->regd_count > 0); Assert(!pairingheap_is_empty(&RegisteredSnapshots)); - ResourceOwnerForgetSnapshot(owner, snapshot); - snapshot->regd_count--; if (snapshot->regd_count == 0) pairingheap_remove(&RegisteredSnapshots, &snapshot->ph_node); @@ -2399,13 +2406,12 @@ XidInMVCCSnapshot(TransactionId xid, Snapshot snapshot) return false; } -/* - * ResourceOwner callbacks - */ +/* ResourceOwner callbacks */ + static void ResOwnerReleaseSnapshot(Datum res) { - UnregisterSnapshot((Snapshot) DatumGetPointer(res)); + UnregisterSnapshotNoOwner((Snapshot) DatumGetPointer(res)); } static void diff --git a/src/common/cryptohash_openssl.c b/src/common/cryptohash_openssl.c index 0fb6dd6736d..5254e53f114 100644 --- a/src/common/cryptohash_openssl.c +++ b/src/common/cryptohash_openssl.c @@ -80,9 +80,9 @@ static void ResOwnerPrintCryptoHashLeakWarning(Datum res); static ResourceOwnerFuncs cryptohash_resowner_funcs = { - /* relcache references */ .name = "OpenSSL cryptohash context", - .phase = RESOURCE_RELEASE_BEFORE_LOCKS, + .release_phase = RESOURCE_RELEASE_BEFORE_LOCKS, + .release_priority = RELEASE_PRIO_CRYPTOHASH_CONTEXTS, .ReleaseResource = ResOwnerReleaseCryptoHash, .PrintLeakWarning = ResOwnerPrintCryptoHashLeakWarning, }; @@ -326,7 +326,8 @@ pg_cryptohash_free(pg_cryptohash_ctx *ctx) EVP_MD_CTX_destroy(ctx->evpctx); #ifndef FRONTEND - ResourceOwnerForgetCryptoHash(ctx->resowner, ctx); + if (ctx->resowner) + ResourceOwnerForgetCryptoHash(ctx->resowner, ctx); #endif explicit_bzero(ctx, sizeof(pg_cryptohash_ctx)); @@ -370,14 +371,16 @@ pg_cryptohash_error(pg_cryptohash_ctx *ctx) return _("success"); } -/* - * ResourceOwner callbacks - */ +/* ResourceOwner callbacks */ + #ifndef FRONTEND static void ResOwnerReleaseCryptoHash(Datum res) { - pg_cryptohash_free((pg_cryptohash_ctx *) DatumGetPointer(res)); + pg_cryptohash_ctx *ctx = (pg_cryptohash_ctx *) DatumGetPointer(res); + + ctx->resowner = NULL; + pg_cryptohash_free(ctx); } static void diff --git a/src/common/hmac_openssl.c b/src/common/hmac_openssl.c index 9f68462fb4a..9b47ad63ba1 100644 --- a/src/common/hmac_openssl.c +++ b/src/common/hmac_openssl.c @@ -79,9 +79,9 @@ static void ResOwnerPrintHMACLeakWarning(Datum res); static ResourceOwnerFuncs hmac_resowner_funcs = { - /* relcache references */ .name = "OpenSSL HMAC context", - .phase = RESOURCE_RELEASE_BEFORE_LOCKS, + .release_phase = RESOURCE_RELEASE_BEFORE_LOCKS, + .release_priority = RELEASE_PRIO_HMAC_CONTEXTS, .ReleaseResource = ResOwnerReleaseHMAC, .PrintLeakWarning = ResOwnerPrintHMACLeakWarning, }; @@ -323,7 +323,8 @@ pg_hmac_free(pg_hmac_ctx *ctx) #ifdef HAVE_HMAC_CTX_FREE HMAC_CTX_free(ctx->hmacctx); #ifndef FRONTEND - ResourceOwnerForgetHMAC(ctx->resowner, ctx); + if (ctx->resowner) + ResourceOwnerForgetHMAC(ctx->resowner, ctx); #endif #else explicit_bzero(ctx->hmacctx, sizeof(HMAC_CTX)); @@ -367,14 +368,16 @@ pg_hmac_error(pg_hmac_ctx *ctx) return _("success"); } -/* - * ResourceOwner callbacks - */ +/* ResourceOwner callbacks */ + #ifndef FRONTEND static void ResOwnerReleaseHMAC(Datum res) { - pg_hmac_free((pg_hmac_ctx *) DatumGetPointer(res)); + pg_hmac_ctx *ctx = (pg_hmac_ctx *) DatumGetPointer(res); + + ctx->resowner = NULL; + pg_hmac_free(ctx); } static void diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h index bc4fdec36ec..60b4904a4d9 100644 --- a/src/include/utils/plancache.h +++ b/src/include/utils/plancache.h @@ -188,6 +188,8 @@ typedef struct CachedExpression extern void InitPlanCache(void); extern void ResetPlanCache(void); +extern void ReleaseAllPlanCacheRefsInOwner(ResourceOwner owner); + extern CachedPlanSource *CreateCachedPlan(struct RawStmt *raw_parse_tree, const char *query_string, CommandTag commandTag); @@ -233,6 +235,4 @@ extern bool CachedPlanIsSimplyValid(CachedPlanSource *plansource, extern CachedExpression *GetCachedExpression(Node *expr); extern void FreeCachedExpression(CachedExpression *cexpr); -extern ResourceOwnerFuncs planref_resowner_funcs; - #endif /* PLANCACHE_H */ diff --git a/src/include/utils/resowner.h b/src/include/utils/resowner.h index 441b346bdfb..0e0a32548c5 100644 --- a/src/include/utils/resowner.h +++ b/src/include/utils/resowner.h @@ -42,6 +42,11 @@ extern PGDLLIMPORT ResourceOwner AuxProcessResourceOwner; * when we release a lock that another backend may be waiting on, it will * see us as being fully out of our transaction. The post-lock phase * should be used for backend-internal cleanup. + * + * Within each phase, resources are released in priority order. Priority is + * just an integer specified in ResourceOwnerFuncs. The priorities of + * built-in resource types are given below, extensions may use any priority + * relative to those or RELEASE_PRIO_FIRST/LAST. */ typedef enum { @@ -50,6 +55,27 @@ typedef enum RESOURCE_RELEASE_AFTER_LOCKS } ResourceReleasePhase; +typedef uint32 ResourceReleasePriority; + +/* priorities of built-in BEFORE_LOCKS resources */ +#define RELEASE_PRIO_BUFFERS 100 +#define RELEASE_PRIO_RELCACHE_REFS 200 +#define RELEASE_PRIO_DSMS 300 +#define RELEASE_PRIO_JIT_CONTEXTS 400 +#define RELEASE_PRIO_CRYPTOHASH_CONTEXTS 500 +#define RELEASE_PRIO_HMAC_CONTEXTS 600 + +/* priorities of built-in AFTER_LOCKS resources */ +#define RELEASE_PRIO_CATCACHE_REFS 100 +#define RELEASE_PRIO_CATCACHE_LIST_REFS 200 +#define RELEASE_PRIO_PLANCACHE_REFS 300 +#define RELEASE_PRIO_TUPDESC_REFS 400 +#define RELEASE_PRIO_SNAPSHOT_REFS 500 +#define RELEASE_PRIO_FILES 600 + +#define RELEASE_PRIO_FIRST 0 +#define RELEASE_PRIO_LAST UINT32_MAX + /* * In order to track an object, resowner.c needs a few callbacks for it. * The callbacks for resources of a specific kind are encapsulated in @@ -62,13 +88,18 @@ typedef struct ResourceOwnerFuncs { const char *name; /* name for the object kind, for debugging */ - ResourceReleasePhase phase; /* when are these objects released? */ + /* when are these objects released? */ + ResourceReleasePhase release_phase; + ResourceReleasePriority release_priority; /* * Release resource. * - * NOTE: this must call ResourceOwnerForget to disassociate it with the - * resource owner. + * This is called for each resource in the resource owner, in the order + * specified by 'release_phase' and 'release_priority' when the whole + * resource owner is been released or when ResourceOwnerReleaseAllOfKind() + * is called. The resource is implicitly removed from the owner, the + * callback function doesn't need to call ResourceOwnerForget. */ void (*ReleaseResource) (Datum res); @@ -110,6 +141,8 @@ extern void ResourceOwnerEnlarge(ResourceOwner owner); extern void ResourceOwnerRemember(ResourceOwner owner, Datum res, ResourceOwnerFuncs *kind); extern void ResourceOwnerForget(ResourceOwner owner, Datum res, ResourceOwnerFuncs *kind); +extern void ResourceOwnerReleaseAllOfKind(ResourceOwner owner, ResourceOwnerFuncs *kind); + extern void RegisterResourceReleaseCallback(ResourceReleaseCallback callback, void *arg); extern void UnregisterResourceReleaseCallback(ResourceReleaseCallback callback, @@ -123,7 +156,4 @@ struct LOCALLOCK; extern void ResourceOwnerRememberLock(ResourceOwner owner, struct LOCALLOCK *locallock); extern void ResourceOwnerForgetLock(ResourceOwner owner, struct LOCALLOCK *locallock); -/* special function to relase all plancache references */ -extern void ResourceOwnerReleaseAllPlanCacheRefs(ResourceOwner owner); - #endif /* RESOWNER_H */ diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index ffd6d2e3bc3..e3cda017a9b 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -8431,7 +8431,7 @@ plpgsql_xact_cb(XactEvent event, void *arg) FreeExecutorState(shared_simple_eval_estate); shared_simple_eval_estate = NULL; if (shared_simple_eval_resowner) - ResourceOwnerReleaseAllPlanCacheRefs(shared_simple_eval_resowner); + ReleaseAllPlanCacheRefsInOwner(shared_simple_eval_resowner); shared_simple_eval_resowner = NULL; } else if (event == XACT_EVENT_ABORT || diff --git a/src/pl/plpgsql/src/pl_handler.c b/src/pl/plpgsql/src/pl_handler.c index d8994538b76..462ba438d61 100644 --- a/src/pl/plpgsql/src/pl_handler.c +++ b/src/pl/plpgsql/src/pl_handler.c @@ -288,7 +288,7 @@ plpgsql_call_handler(PG_FUNCTION_ARGS) /* Be sure to release the procedure resowner if any */ if (procedure_resowner) { - ResourceOwnerReleaseAllPlanCacheRefs(procedure_resowner); + ReleaseAllPlanCacheRefsInOwner(procedure_resowner); ResourceOwnerDelete(procedure_resowner); } } @@ -393,7 +393,7 @@ plpgsql_inline_handler(PG_FUNCTION_ARGS) /* Clean up the private EState and resowner */ FreeExecutorState(simple_eval_estate); - ResourceOwnerReleaseAllPlanCacheRefs(simple_eval_resowner); + ReleaseAllPlanCacheRefsInOwner(simple_eval_resowner); ResourceOwnerDelete(simple_eval_resowner); /* Function should now have no remaining use-counts ... */ @@ -410,7 +410,7 @@ plpgsql_inline_handler(PG_FUNCTION_ARGS) /* Clean up the private EState and resowner */ FreeExecutorState(simple_eval_estate); - ResourceOwnerReleaseAllPlanCacheRefs(simple_eval_resowner); + ReleaseAllPlanCacheRefsInOwner(simple_eval_resowner); ResourceOwnerDelete(simple_eval_resowner); /* Function should now have no remaining use-counts ... */ diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile index c629cbe3830..cc0abadf750 100644 --- a/src/test/modules/Makefile +++ b/src/test/modules/Makefile @@ -30,6 +30,7 @@ SUBDIRS = \ test_predtest \ test_rbtree \ test_regex \ + test_resowner \ test_rls_hooks \ test_shm_mq \ test_slru \ diff --git a/src/test/modules/test_resowner/.gitignore b/src/test/modules/test_resowner/.gitignore new file mode 100644 index 00000000000..5dcb3ff9723 --- /dev/null +++ b/src/test/modules/test_resowner/.gitignore @@ -0,0 +1,4 @@ +# Generated subdirectories +/log/ +/results/ +/tmp_check/ diff --git a/src/test/modules/test_resowner/Makefile b/src/test/modules/test_resowner/Makefile new file mode 100644 index 00000000000..d28da3c2869 --- /dev/null +++ b/src/test/modules/test_resowner/Makefile @@ -0,0 +1,24 @@ +# src/test/modules/test_resowner/Makefile + +MODULE_big = test_resowner +OBJS = \ + $(WIN32RES) \ + test_resowner_basic.o \ + test_resowner_many.o +PGFILEDESC = "test_resowner - test code for ResourceOwners" + +EXTENSION = test_resowner +DATA = test_resowner--1.0.sql + +REGRESS = test_resowner + +ifdef USE_PGXS +PG_CONFIG = pg_config +PGXS := $(shell $(PG_CONFIG) --pgxs) +include $(PGXS) +else +subdir = src/test/modules/test_resowner +top_builddir = ../../../.. +include $(top_builddir)/src/Makefile.global +include $(top_srcdir)/contrib/contrib-global.mk +endif diff --git a/src/test/modules/test_resowner/expected/test_resowner.out b/src/test/modules/test_resowner/expected/test_resowner.out new file mode 100644 index 00000000000..c7865e3ac19 --- /dev/null +++ b/src/test/modules/test_resowner/expected/test_resowner.out @@ -0,0 +1,197 @@ +CREATE EXTENSION test_resowner; +-- This is small enough that everything fits in the small array +SELECT test_resowner_priorities(2, 3); +NOTICE: releasing resources before locks +NOTICE: releasing string: child before locks priority 0 +NOTICE: releasing string: child before locks priority 0 +NOTICE: releasing string: child before locks priority 1 +NOTICE: releasing string: parent before locks priority 0 +NOTICE: releasing string: parent before locks priority 0 +NOTICE: releasing string: parent before locks priority 1 +NOTICE: releasing locks +NOTICE: releasing resources after locks +NOTICE: releasing string: child after locks priority 0 +NOTICE: releasing string: child after locks priority 0 +NOTICE: releasing string: child after locks priority 1 +NOTICE: releasing string: parent after locks priority 0 +NOTICE: releasing string: parent after locks priority 0 +NOTICE: releasing string: parent after locks priority 1 + test_resowner_priorities +-------------------------- + +(1 row) + +-- Same test with more resources, to exercise the hash table +SELECT test_resowner_priorities(2, 32); +NOTICE: releasing resources before locks +NOTICE: releasing string: child before locks priority 0 +NOTICE: releasing string: child before locks priority 0 +NOTICE: releasing string: child before locks priority 0 +NOTICE: releasing string: child before locks priority 0 +NOTICE: releasing string: child before locks priority 0 +NOTICE: releasing string: child before locks priority 0 +NOTICE: releasing string: child before locks priority 0 +NOTICE: releasing string: child before locks priority 0 +NOTICE: releasing string: child before locks priority 0 +NOTICE: releasing string: child before locks priority 0 +NOTICE: releasing string: child before locks priority 0 +NOTICE: releasing string: child before locks priority 0 +NOTICE: releasing string: child before locks priority 0 +NOTICE: releasing string: child before locks priority 0 +NOTICE: releasing string: child before locks priority 0 +NOTICE: releasing string: child before locks priority 0 +NOTICE: releasing string: child before locks priority 1 +NOTICE: releasing string: child before locks priority 1 +NOTICE: releasing string: child before locks priority 1 +NOTICE: releasing string: child before locks priority 1 +NOTICE: releasing string: child before locks priority 1 +NOTICE: releasing string: child before locks priority 1 +NOTICE: releasing string: child before locks priority 1 +NOTICE: releasing string: child before locks priority 1 +NOTICE: releasing string: child before locks priority 1 +NOTICE: releasing string: child before locks priority 1 +NOTICE: releasing string: child before locks priority 1 +NOTICE: releasing string: child before locks priority 1 +NOTICE: releasing string: child before locks priority 1 +NOTICE: releasing string: child before locks priority 1 +NOTICE: releasing string: child before locks priority 1 +NOTICE: releasing string: child before locks priority 1 +NOTICE: releasing string: parent before locks priority 0 +NOTICE: releasing string: parent before locks priority 0 +NOTICE: releasing string: parent before locks priority 0 +NOTICE: releasing string: parent before locks priority 0 +NOTICE: releasing string: parent before locks priority 0 +NOTICE: releasing string: parent before locks priority 0 +NOTICE: releasing string: parent before locks priority 0 +NOTICE: releasing string: parent before locks priority 0 +NOTICE: releasing string: parent before locks priority 0 +NOTICE: releasing string: parent before locks priority 0 +NOTICE: releasing string: parent before locks priority 0 +NOTICE: releasing string: parent before locks priority 0 +NOTICE: releasing string: parent before locks priority 0 +NOTICE: releasing string: parent before locks priority 0 +NOTICE: releasing string: parent before locks priority 0 +NOTICE: releasing string: parent before locks priority 0 +NOTICE: releasing string: parent before locks priority 1 +NOTICE: releasing string: parent before locks priority 1 +NOTICE: releasing string: parent before locks priority 1 +NOTICE: releasing string: parent before locks priority 1 +NOTICE: releasing string: parent before locks priority 1 +NOTICE: releasing string: parent before locks priority 1 +NOTICE: releasing string: parent before locks priority 1 +NOTICE: releasing string: parent before locks priority 1 +NOTICE: releasing string: parent before locks priority 1 +NOTICE: releasing string: parent before locks priority 1 +NOTICE: releasing string: parent before locks priority 1 +NOTICE: releasing string: parent before locks priority 1 +NOTICE: releasing string: parent before locks priority 1 +NOTICE: releasing string: parent before locks priority 1 +NOTICE: releasing string: parent before locks priority 1 +NOTICE: releasing string: parent before locks priority 1 +NOTICE: releasing locks +NOTICE: releasing resources after locks +NOTICE: releasing string: child after locks priority 0 +NOTICE: releasing string: child after locks priority 0 +NOTICE: releasing string: child after locks priority 0 +NOTICE: releasing string: child after locks priority 0 +NOTICE: releasing string: child after locks priority 0 +NOTICE: releasing string: child after locks priority 0 +NOTICE: releasing string: child after locks priority 0 +NOTICE: releasing string: child after locks priority 0 +NOTICE: releasing string: child after locks priority 0 +NOTICE: releasing string: child after locks priority 0 +NOTICE: releasing string: child after locks priority 0 +NOTICE: releasing string: child after locks priority 0 +NOTICE: releasing string: child after locks priority 0 +NOTICE: releasing string: child after locks priority 0 +NOTICE: releasing string: child after locks priority 0 +NOTICE: releasing string: child after locks priority 0 +NOTICE: releasing string: child after locks priority 1 +NOTICE: releasing string: child after locks priority 1 +NOTICE: releasing string: child after locks priority 1 +NOTICE: releasing string: child after locks priority 1 +NOTICE: releasing string: child after locks priority 1 +NOTICE: releasing string: child after locks priority 1 +NOTICE: releasing string: child after locks priority 1 +NOTICE: releasing string: child after locks priority 1 +NOTICE: releasing string: child after locks priority 1 +NOTICE: releasing string: child after locks priority 1 +NOTICE: releasing string: child after locks priority 1 +NOTICE: releasing string: child after locks priority 1 +NOTICE: releasing string: child after locks priority 1 +NOTICE: releasing string: child after locks priority 1 +NOTICE: releasing string: child after locks priority 1 +NOTICE: releasing string: child after locks priority 1 +NOTICE: releasing string: parent after locks priority 0 +NOTICE: releasing string: parent after locks priority 0 +NOTICE: releasing string: parent after locks priority 0 +NOTICE: releasing string: parent after locks priority 0 +NOTICE: releasing string: parent after locks priority 0 +NOTICE: releasing string: parent after locks priority 0 +NOTICE: releasing string: parent after locks priority 0 +NOTICE: releasing string: parent after locks priority 0 +NOTICE: releasing string: parent after locks priority 0 +NOTICE: releasing string: parent after locks priority 0 +NOTICE: releasing string: parent after locks priority 0 +NOTICE: releasing string: parent after locks priority 0 +NOTICE: releasing string: parent after locks priority 0 +NOTICE: releasing string: parent after locks priority 0 +NOTICE: releasing string: parent after locks priority 0 +NOTICE: releasing string: parent after locks priority 0 +NOTICE: releasing string: parent after locks priority 1 +NOTICE: releasing string: parent after locks priority 1 +NOTICE: releasing string: parent after locks priority 1 +NOTICE: releasing string: parent after locks priority 1 +NOTICE: releasing string: parent after locks priority 1 +NOTICE: releasing string: parent after locks priority 1 +NOTICE: releasing string: parent after locks priority 1 +NOTICE: releasing string: parent after locks priority 1 +NOTICE: releasing string: parent after locks priority 1 +NOTICE: releasing string: parent after locks priority 1 +NOTICE: releasing string: parent after locks priority 1 +NOTICE: releasing string: parent after locks priority 1 +NOTICE: releasing string: parent after locks priority 1 +NOTICE: releasing string: parent after locks priority 1 +NOTICE: releasing string: parent after locks priority 1 +NOTICE: releasing string: parent after locks priority 1 + test_resowner_priorities +-------------------------- + +(1 row) + +-- Basic test with lots more resources, to test extending the hash table +SELECT test_resowner_many( + 3, -- # of different resource kinds + 100000, -- before-locks resources to remember + 500, -- before-locks resources to forget + 100000, -- after-locks resources to remember + 500 -- after-locks resources to forget +); +NOTICE: remembering 100000 before-locks resources +NOTICE: remembering 100000 after-locks resources +NOTICE: forgetting 500 before-locks resources +NOTICE: forgetting 500 after-locks resources +NOTICE: releasing resources before locks +NOTICE: releasing locks +NOTICE: releasing resources after locks + test_resowner_many +-------------------- + +(1 row) + +-- Test resource leak warning +SELECT test_resowner_leak(); +WARNING: string leak: my string +NOTICE: releasing string: my string + test_resowner_leak +-------------------- + +(1 row) + +-- Negative tests, using a resource owner after release-phase has started. +set client_min_messages='warning'; -- order between ERROR and NOTICE varies +SELECT test_resowner_remember_between_phases(); +ERROR: ResourceOwnerEnlarge called after release started +SELECT test_resowner_forget_between_phases(); +ERROR: ResourceOwnerForget called for test resource after release started +reset client_min_messages; diff --git a/src/test/modules/test_resowner/meson.build b/src/test/modules/test_resowner/meson.build new file mode 100644 index 00000000000..e92f385a809 --- /dev/null +++ b/src/test/modules/test_resowner/meson.build @@ -0,0 +1,37 @@ +# Copyright (c) 2022-2023, PostgreSQL Global Development Group + +# FIXME: prevent install during main install, but not during test :/ + +test_resowner_sources = files( + 'test_resowner_basic.c', + 'test_resowner_many.c', +) + +if host_system == 'windows' + test_resowner_sources += rc_lib_gen.process(win32ver_rc, extra_args: [ + '--NAME', 'test_resowner', + '--FILEDESC', 'test_resowner - test code for ResourceOwners',]) +endif + +test_resowner = shared_module('test_resowner', + test_resowner_sources, + kwargs: pg_mod_args, +) +testprep_targets += test_resowner + +install_data( + 'test_resowner.control', + 'test_resowner--1.0.sql', + kwargs: contrib_data_args, +) + +tests += { + 'name': 'test_resowner', + 'sd': meson.current_source_dir(), + 'bd': meson.current_build_dir(), + 'regress': { + 'sql': [ + 'test_resowner', + ], + }, +} diff --git a/src/test/modules/test_resowner/test_resowner--1.0.sql b/src/test/modules/test_resowner/test_resowner--1.0.sql new file mode 100644 index 00000000000..26fed7a010c --- /dev/null +++ b/src/test/modules/test_resowner/test_resowner--1.0.sql @@ -0,0 +1,30 @@ +/* src/test/modules/test_resowner/test_resowner--1.0.sql */ + +-- complain if script is sourced in psql, rather than via CREATE EXTENSION +\echo Use "CREATE EXTENSION test_resowner" to load this file. \quit + +CREATE FUNCTION test_resowner_priorities(nkinds pg_catalog.int4, nresources pg_catalog.int4) + RETURNS pg_catalog.void + AS 'MODULE_PATHNAME' LANGUAGE C; + +CREATE FUNCTION test_resowner_leak() + RETURNS pg_catalog.void + AS 'MODULE_PATHNAME' LANGUAGE C; + +CREATE FUNCTION test_resowner_remember_between_phases() + RETURNS pg_catalog.void + AS 'MODULE_PATHNAME' LANGUAGE C; + +CREATE FUNCTION test_resowner_forget_between_phases() + RETURNS pg_catalog.void + AS 'MODULE_PATHNAME' LANGUAGE C; + +CREATE FUNCTION test_resowner_many( + nkinds pg_catalog.int4, + nremember_bl pg_catalog.int4, + nforget_bl pg_catalog.int4, + nremember_al pg_catalog.int4, + nforget_al pg_catalog.int4 +) + RETURNS pg_catalog.void + AS 'MODULE_PATHNAME' LANGUAGE C; diff --git a/src/test/modules/test_resowner/test_resowner.control b/src/test/modules/test_resowner/test_resowner.control new file mode 100644 index 00000000000..b56c4ee92bd --- /dev/null +++ b/src/test/modules/test_resowner/test_resowner.control @@ -0,0 +1,4 @@ +comment = 'Test code for ResourceOwners' +default_version = '1.0' +module_pathname = '$libdir/test_resowner' +relocatable = true diff --git a/src/test/modules/test_resowner/test_resowner_basic.c b/src/test/modules/test_resowner/test_resowner_basic.c new file mode 100644 index 00000000000..4f4875b1a41 --- /dev/null +++ b/src/test/modules/test_resowner/test_resowner_basic.c @@ -0,0 +1,210 @@ +/*-------------------------------------------------------------------------- + * + * test_resowner_basic.c + * Test basic ResourceOwner functionality + * + * Copyright (c) 2022-2023, PostgreSQL Global Development Group + * + * IDENTIFICATION + * src/test/modules/test_resowner/test_resowner_basic.c + * + * ------------------------------------------------------------------------- + */ + +#include "postgres.h" + +#include "fmgr.h" +#include "lib/ilist.h" +#include "utils/memutils.h" +#include "utils/resowner.h" + +PG_MODULE_MAGIC; + +static void ReleaseString(Datum res); +static void PrintStringLeakWarning(Datum res); + +static ResourceOwnerFuncs string_funcs = { + .name = "test resource", + .release_phase = RESOURCE_RELEASE_AFTER_LOCKS, + .release_priority = RELEASE_PRIO_FIRST, + .ReleaseResource = ReleaseString, + .PrintLeakWarning = PrintStringLeakWarning +}; + +static void +ReleaseString(Datum res) +{ + elog(NOTICE, "releasing string: %s", DatumGetPointer(res)); +} + +static void +PrintStringLeakWarning(Datum res) +{ + elog(WARNING, "string leak: %s", DatumGetPointer(res)); +} + +/* demonstrates phases and priorities betwen a parent and child context */ +PG_FUNCTION_INFO_V1(test_resowner_priorities); +Datum +test_resowner_priorities(PG_FUNCTION_ARGS) +{ + int32 nkinds = PG_GETARG_INT32(0); + int32 nresources = PG_GETARG_INT32(1); + ResourceOwner parent, + child; + ResourceOwnerFuncs *before_funcs; + ResourceOwnerFuncs *after_funcs; + + if (nkinds <= 0) + elog(ERROR, "nkinds must be greater than zero"); + if (nresources <= 0) + elog(ERROR, "nresources must be greater than zero"); + + parent = ResourceOwnerCreate(CurrentResourceOwner, "test parent"); + child = ResourceOwnerCreate(parent, "test child"); + + /* FIXME Add a bunch of resources to child, with different priorities */ + before_funcs = palloc(nkinds * sizeof(ResourceOwnerFuncs)); + for (int i = 0; i < nkinds; i++) + { + before_funcs[i].name = psprintf("test resource before locks %d", i); + before_funcs[i].release_phase = RESOURCE_RELEASE_BEFORE_LOCKS; + before_funcs[i].release_priority = i; + before_funcs[i].ReleaseResource = ReleaseString; + before_funcs[i].PrintLeakWarning = PrintStringLeakWarning; + } + after_funcs = palloc(nkinds * sizeof(ResourceOwnerFuncs)); + for (int i = 0; i < nkinds; i++) + { + after_funcs[i].name = psprintf("test resource after locks %d", i); + after_funcs[i].release_phase = RESOURCE_RELEASE_AFTER_LOCKS; + after_funcs[i].release_priority = i; + after_funcs[i].ReleaseResource = ReleaseString; + after_funcs[i].PrintLeakWarning = PrintStringLeakWarning; + } + + ResourceOwnerEnlarge(child); + + for (int i = 0; i < nresources; i++) + { + ResourceOwnerFuncs *kind = &before_funcs[i % nkinds]; + + ResourceOwnerEnlarge(child); + ResourceOwnerRemember(child, + CStringGetDatum(psprintf("child before locks priority %d", kind->release_priority)), + kind); + } + + for (int i = 0; i < nresources; i++) + { + ResourceOwnerFuncs *kind = &after_funcs[i % nkinds]; + + ResourceOwnerEnlarge(child); + ResourceOwnerRemember(child, + CStringGetDatum(psprintf("child after locks priority %d", kind->release_priority)), + kind); + } + + for (int i = 0; i < nresources; i++) + { + ResourceOwnerFuncs *kind = &after_funcs[i % nkinds]; + + ResourceOwnerEnlarge(parent); + ResourceOwnerRemember(parent, + CStringGetDatum(psprintf("parent after locks priority %d", kind->release_priority)), + kind); + } + for (int i = 0; i < nresources; i++) + { + ResourceOwnerFuncs *kind = &before_funcs[i % nkinds]; + + ResourceOwnerEnlarge(parent); + ResourceOwnerRemember(parent, + CStringGetDatum(psprintf("parent before locks priority %d", kind->release_priority)), + kind); + } + + elog(NOTICE, "releasing resources before locks"); + ResourceOwnerRelease(parent, RESOURCE_RELEASE_BEFORE_LOCKS, false, false); + elog(NOTICE, "releasing locks"); + ResourceOwnerRelease(parent, RESOURCE_RELEASE_LOCKS, false, false); + elog(NOTICE, "releasing resources after locks"); + ResourceOwnerRelease(parent, RESOURCE_RELEASE_AFTER_LOCKS, false, false); + + ResourceOwnerDelete(parent); + + PG_RETURN_VOID(); +} + +PG_FUNCTION_INFO_V1(test_resowner_leak); +Datum +test_resowner_leak(PG_FUNCTION_ARGS) +{ + ResourceOwner resowner; + + resowner = ResourceOwnerCreate(CurrentResourceOwner, "TestOwner"); + + ResourceOwnerEnlarge(resowner); + + ResourceOwnerRemember(resowner, CStringGetDatum("my string"), &string_funcs); + + /* don't call ResourceOwnerForget, so that it is leaked */ + + ResourceOwnerRelease(resowner, RESOURCE_RELEASE_BEFORE_LOCKS, true, false); + ResourceOwnerRelease(resowner, RESOURCE_RELEASE_LOCKS, true, false); + ResourceOwnerRelease(resowner, RESOURCE_RELEASE_AFTER_LOCKS, true, false); + + ResourceOwnerDelete(resowner); + + PG_RETURN_VOID(); +} + +PG_FUNCTION_INFO_V1(test_resowner_remember_between_phases); +Datum +test_resowner_remember_between_phases(PG_FUNCTION_ARGS) +{ + ResourceOwner resowner; + + resowner = ResourceOwnerCreate(CurrentResourceOwner, "TestOwner"); + + ResourceOwnerRelease(resowner, RESOURCE_RELEASE_BEFORE_LOCKS, true, false); + + /* + * Try to remember a new resource. Fails because we already called + * ResourceOwnerRelease. + */ + ResourceOwnerEnlarge(resowner); + ResourceOwnerRemember(resowner, CStringGetDatum("my string"), &string_funcs); + + /* unreachable */ + elog(ERROR, "ResourceOwnerEnlarge should have errored out"); + + PG_RETURN_VOID(); +} + +PG_FUNCTION_INFO_V1(test_resowner_forget_between_phases); +Datum +test_resowner_forget_between_phases(PG_FUNCTION_ARGS) +{ + ResourceOwner resowner; + Datum str_resource; + + resowner = ResourceOwnerCreate(CurrentResourceOwner, "TestOwner"); + + ResourceOwnerEnlarge(resowner); + str_resource = CStringGetDatum("my string"); + ResourceOwnerRemember(resowner, str_resource, &string_funcs); + + ResourceOwnerRelease(resowner, RESOURCE_RELEASE_BEFORE_LOCKS, true, false); + + /* + * Try to forget the resource that was remembered earlier. Fails because + * we already called ResourceOwnerRelease. + */ + ResourceOwnerForget(resowner, str_resource, &string_funcs); + + /* unreachable */ + elog(ERROR, "ResourceOwnerForget should have errored out"); + + PG_RETURN_VOID(); +} diff --git a/src/test/modules/test_resowner/test_resowner_many.c b/src/test/modules/test_resowner/test_resowner_many.c new file mode 100644 index 00000000000..5b8a34b37ef --- /dev/null +++ b/src/test/modules/test_resowner/test_resowner_many.c @@ -0,0 +1,287 @@ +/*-------------------------------------------------------------------------- + * + * test_resowner_many.c + * Test ResourceOwner functionality with lots of resources + * + * Copyright (c) 2022-2023, PostgreSQL Global Development Group + * + * IDENTIFICATION + * src/test/modules/test_resowner/test_resowner_many.c + * + * ------------------------------------------------------------------------- + */ + +#include "postgres.h" + +#include "fmgr.h" +#include "lib/ilist.h" +#include "utils/memutils.h" +#include "utils/resowner.h" + +/* + * Define a custom resource type to use in the test. The resource being + * tracked is a palloc'd ManyTestResource struct. + * + * To cross-check that the ResourceOwner calls the callback functions + * correctly, we keep track of the remembered resources ourselves in a linked + * list, and also keep counters of how many times the callback functions have + * been called. + */ +typedef struct +{ + ResourceOwnerFuncs funcs; + int nremembered; + int nforgotten; + int nreleased; + int nleaked; + + dlist_head current_resources; +} ManyTestResourceKind; + +typedef struct +{ + ManyTestResourceKind *kind; + dlist_node node; +} ManyTestResource; + +/* + * Current release phase, and priority of last call to the release callback. + * This is used to check that the resources are released in correct order. + */ +static ResourceReleasePhase current_release_phase; +static uint32 last_release_priority = 0; + +/* prototypes for local functions */ +static void ReleaseManyTestResource(Datum res); +static void PrintManyTestLeakWarning(Datum res); +static void InitManyTestResourceKind(ManyTestResourceKind *kind, char *name, + ResourceReleasePhase phase, uint32 priority); +static void RememberManyTestResources(ResourceOwner owner, + ManyTestResourceKind *kinds, int nkinds, + int nresources); +static void ForgetManyTestResources(ResourceOwner owner, + ManyTestResourceKind *kinds, int nkinds, + int nresources); +static int GetTotalResourceCount(ManyTestResourceKind *kinds, int nkinds); + +/* ResourceOwner callback */ +static void +ReleaseManyTestResource(Datum res) +{ + ManyTestResource *mres = (ManyTestResource *) DatumGetPointer(res); + + elog(DEBUG1, "releasing resource %p from %s", mres, mres->kind->funcs.name); + Assert(last_release_priority <= mres->kind->funcs.release_priority); + + dlist_delete(&mres->node); + mres->kind->nreleased++; + last_release_priority = mres->kind->funcs.release_priority; + pfree(mres); +} + +/* ResourceOwner callback */ +static void +PrintManyTestLeakWarning(Datum res) +{ + ManyTestResource *mres = (ManyTestResource *) DatumGetPointer(res); + + mres->kind->nleaked++; + elog(DEBUG1, "leaked resource from %s", mres->kind->funcs.name); +} + +static void +InitManyTestResourceKind(ManyTestResourceKind *kind, char *name, + ResourceReleasePhase phase, uint32 priority) +{ + kind->funcs.name = name; + kind->funcs.release_phase = phase; + kind->funcs.release_priority = priority; + kind->funcs.ReleaseResource = ReleaseManyTestResource; + kind->funcs.PrintLeakWarning = PrintManyTestLeakWarning; + kind->nremembered = 0; + kind->nforgotten = 0; + kind->nreleased = 0; + kind->nleaked = 0; + dlist_init(&kind->current_resources); +} + +/* + * Remember 'nresources' resources. The resources are remembered in round + * robin fashion with the kinds from 'kinds' array. + */ +static void +RememberManyTestResources(ResourceOwner owner, + ManyTestResourceKind *kinds, int nkinds, + int nresources) +{ + int kind_idx = 0; + + for (int i = 0; i < nresources; i++) + { + ManyTestResource *mres = palloc(sizeof(ManyTestResource)); + + mres->kind = &kinds[kind_idx]; + dlist_node_init(&mres->node); + + ResourceOwnerEnlarge(owner); + ResourceOwnerRemember(owner, PointerGetDatum(mres), &kinds[kind_idx].funcs); + kinds[kind_idx].nremembered++; + dlist_push_tail(&kinds[kind_idx].current_resources, &mres->node); + + elog(DEBUG1, "remembered resource %p from %s", mres, mres->kind->funcs.name); + + kind_idx = (kind_idx + 1) % nkinds; + } +} + +/* + * Forget 'nresources' resources, in round robin fashion from 'kinds'. + */ +static void +ForgetManyTestResources(ResourceOwner owner, + ManyTestResourceKind *kinds, int nkinds, + int nresources) +{ + int kind_idx = 0; + + Assert(GetTotalResourceCount(kinds, nkinds) >= nresources); + + for (int i = 0; i < nresources; i++) + { + bool found = false; + + for (int j = 0; j < nkinds; j++) + { + kind_idx = (kind_idx + 1) % nkinds; + if (!dlist_is_empty(&kinds[kind_idx].current_resources)) + { + ManyTestResource *mres = dlist_head_element(ManyTestResource, node, &kinds[kind_idx].current_resources); + + ResourceOwnerForget(owner, PointerGetDatum(mres), &kinds[kind_idx].funcs); + kinds[kind_idx].nforgotten++; + dlist_delete(&mres->node); + pfree(mres); + + found = true; + break; + } + } + if (!found) + elog(ERROR, "could not find a test resource to forget"); + } +} + +/* + * Get total number of currently active resources among 'kinds'. + */ +static int +GetTotalResourceCount(ManyTestResourceKind *kinds, int nkinds) +{ + int ntotal = 0; + + for (int i = 0; i < nkinds; i++) + ntotal += kinds[i].nremembered - kinds[i].nforgotten - kinds[i].nreleased; + + return ntotal; +} + +/* + * Remember lots of resources, belonging to 'nkinds' different resource types + * with different priorities. Then forget some of them, and finally, release + * the resource owner. We use a custom resource type that performs various + * sanity checks to verify that the all the resources are released, and in the + * correct order. + */ +PG_FUNCTION_INFO_V1(test_resowner_many); +Datum +test_resowner_many(PG_FUNCTION_ARGS) +{ + int32 nkinds = PG_GETARG_INT32(0); + int32 nremember_bl = PG_GETARG_INT32(1); + int32 nforget_bl = PG_GETARG_INT32(2); + int32 nremember_al = PG_GETARG_INT32(3); + int32 nforget_al = PG_GETARG_INT32(4); + + ResourceOwner resowner; + + ManyTestResourceKind *before_kinds; + ManyTestResourceKind *after_kinds; + + /* Sanity check the arguments */ + if (nkinds < 0) + elog(ERROR, "nkinds must be >= 0"); + if (nremember_bl < 0) + elog(ERROR, "nremember_bl must be >= 0"); + if (nforget_bl < 0 || nforget_bl > nremember_bl) + elog(ERROR, "nforget_bl must between 0 and 'nremember_bl'"); + if (nremember_al < 0) + elog(ERROR, "nremember_al must be greater than zero"); + if (nforget_al < 0 || nforget_al > nremember_al) + elog(ERROR, "nforget_al must between 0 and 'nremember_al'"); + + /* Initialize all the different resource kinds to use */ + before_kinds = palloc(nkinds * sizeof(ManyTestResourceKind)); + for (int i = 0; i < nkinds; i++) + { + InitManyTestResourceKind(&before_kinds[i], + psprintf("resource before locks %d", i), + RESOURCE_RELEASE_BEFORE_LOCKS, i); + } + after_kinds = palloc(nkinds * sizeof(ManyTestResourceKind)); + for (int i = 0; i < nkinds; i++) + { + InitManyTestResourceKind(&after_kinds[i], + psprintf("resource after locks %d", i), + RESOURCE_RELEASE_AFTER_LOCKS, i); + } + + resowner = ResourceOwnerCreate(CurrentResourceOwner, "TestOwner"); + + /* Remember a bunch of resources */ + if (nremember_bl > 0) + { + elog(NOTICE, "remembering %d before-locks resources", nremember_bl); + RememberManyTestResources(resowner, before_kinds, nkinds, nremember_bl); + } + if (nremember_al > 0) + { + elog(NOTICE, "remembering %d after-locks resources", nremember_al); + RememberManyTestResources(resowner, after_kinds, nkinds, nremember_al); + } + + /* Forget what was remembered */ + if (nforget_bl > 0) + { + elog(NOTICE, "forgetting %d before-locks resources", nforget_bl); + ForgetManyTestResources(resowner, before_kinds, nkinds, nforget_bl); + } + + if (nforget_al > 0) + { + elog(NOTICE, "forgetting %d after-locks resources", nforget_al); + ForgetManyTestResources(resowner, after_kinds, nkinds, nforget_al); + } + + /* Start releasing */ + elog(NOTICE, "releasing resources before locks"); + current_release_phase = RESOURCE_RELEASE_BEFORE_LOCKS; + last_release_priority = 0; + ResourceOwnerRelease(resowner, RESOURCE_RELEASE_BEFORE_LOCKS, false, false); + Assert(GetTotalResourceCount(before_kinds, nkinds) == 0); + + elog(NOTICE, "releasing locks"); + current_release_phase = RESOURCE_RELEASE_LOCKS; + last_release_priority = 0; + ResourceOwnerRelease(resowner, RESOURCE_RELEASE_LOCKS, false, false); + + elog(NOTICE, "releasing resources after locks"); + current_release_phase = RESOURCE_RELEASE_AFTER_LOCKS; + last_release_priority = 0; + ResourceOwnerRelease(resowner, RESOURCE_RELEASE_AFTER_LOCKS, false, false); + Assert(GetTotalResourceCount(before_kinds, nkinds) == 0); + Assert(GetTotalResourceCount(after_kinds, nkinds) == 0); + + ResourceOwnerDelete(resowner); + + PG_RETURN_VOID(); +} -- 2.30.2