From bbb37408e5496fd011cf460279cf9ba2533c4fd6 Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Thu, 16 Oct 2025 13:27:25 +0200 Subject: [PATCH v2] Deduplicate entries in ResourceOwner We may track the same resource many times, but the current ResourceOwner does not handle that efficiently. Each reference is a separate entry, which leads to long linear searches in the simple hash table. This adds a simple opportunistic deduplication. Entries get a counter, tracking how many references they represent. It only applies to the "hash" phase, not to the initial array. And when adding an entry to the hash finds an empty slot first, it uses it (instead of continuing to look for a possible match). This means the deduplication is not perfect - there may be multiple entries for the same value. But in practice it seems like a good trade off. It won't affect cases with no duplicates, and it will help cases with many of them. The code also continues to grow the hash table as before. In some cases it might be enough to deduplicate the contents. But it's hard to say in advance, and it's not much cheaper as it still requires walking the current hash table. So there's a risk of attempting deduplication, only to find the hash still needs to be enlarged, doubling the cost. So we just grow the hash table. In the worst case the memory usage is not worse than without deduplication. If deduplication is effective, the hash table grows later or not at all. --- src/backend/utils/resowner/resowner.c | 215 ++++++++++++++++++++------ 1 file changed, 166 insertions(+), 49 deletions(-) diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.c index fca84ded6dd..055426c7761 100644 --- a/src/backend/utils/resowner/resowner.c +++ b/src/backend/utils/resowner/resowner.c @@ -15,6 +15,12 @@ * a particular reference, you need to search both the array and the hash * table. * + * The hash table deduplicates entries in an opportunistic way. If it finds + * an entry for the same resource (same kind), it increments a counter instead. + * But if it finds an empty slot first, it uses the slot. If the table gets + * enlarged, those entries will be merged, but it's possible to have multiple + * entries for the same resource. The fixed-size array does not deduplicate. + * * 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 @@ -67,6 +73,18 @@ typedef struct ResourceElem const ResourceOwnerDesc *kind; /* NULL indicates a free hash table slot */ } ResourceElem; +/* + * ResourceHashElem represents a reference associated with a resource owner, + * stored in the hash table. Similar to ResourceElem, but has a counter field + * tracking how many references it represents, to allow deduplication. + */ +typedef struct ResourceHashElem +{ + Datum item; + uint32 count; /* number of references */ + const ResourceOwnerDesc *kind; /* NULL indicates a free hash table slot */ +} ResourceHashElem; + /* * Size of the fixed-size array to hold most-recently remembered resources. */ @@ -151,7 +169,7 @@ struct ResourceOwnerData * release priority. The first 'nhash' elements are occupied, the rest * are empty. */ - ResourceElem *hash; + ResourceHashElem *hash; uint32 capacity; /* allocated length of hash[] */ uint32 grow_at; /* grow hash when reach this */ @@ -198,7 +216,8 @@ static ResourceReleaseCallbackItem *ResourceRelease_callbacks = NULL; /* Internal routines */ static inline uint32 hash_resource_elem(Datum value, const ResourceOwnerDesc *kind); static void ResourceOwnerAddToHash(ResourceOwner owner, Datum value, - const ResourceOwnerDesc *kind); + const ResourceOwnerDesc *kind, + uint32 count); static int resource_priority_cmp(const void *a, const void *b); static void ResourceOwnerSort(ResourceOwner owner); static void ResourceOwnerReleaseAll(ResourceOwner owner, @@ -239,7 +258,8 @@ hash_resource_elem(Datum value, const ResourceOwnerDesc *kind) * Adds 'value' of given 'kind' to the ResourceOwner's hash table */ static void -ResourceOwnerAddToHash(ResourceOwner owner, Datum value, const ResourceOwnerDesc *kind) +ResourceOwnerAddToHash(ResourceOwner owner, Datum value, const ResourceOwnerDesc *kind, + uint32 count) { uint32 mask = owner->capacity - 1; uint32 idx; @@ -250,12 +270,25 @@ ResourceOwnerAddToHash(ResourceOwner owner, Datum value, const ResourceOwnerDesc idx = hash_resource_elem(value, kind) & mask; for (;;) { + /* + * Increment counter for a matching entry or use an empty slot, + * whichever we find first. + */ + + if ((owner->hash[idx].kind == kind) && + (owner->hash[idx].item == value)) + { + owner->hash[idx].count += count; + return; + } + if (owner->hash[idx].kind == NULL) break; /* found a free slot */ idx = (idx + 1) & mask; } owner->hash[idx].item = value; owner->hash[idx].kind = kind; + owner->hash[idx].count = count; owner->nhash++; } @@ -277,6 +310,24 @@ resource_priority_cmp(const void *a, const void *b) return 1; } +/* + * Comparison function to sort by release phase and priority + */ +static int +resource_priority_hash_cmp(const void *a, const void *b) +{ + const ResourceHashElem *ra = (const ResourceHashElem *) a; + const ResourceHashElem *rb = (const ResourceHashElem *) b; + + /* Note: reverse order */ + if (ra->kind->release_phase == rb->kind->release_phase) + return pg_cmp_u32(rb->kind->release_priority, ra->kind->release_priority); + else if (ra->kind->release_phase > rb->kind->release_phase) + return -1; + else + return 1; +} + /* * Sort resources in reverse release priority. * @@ -288,16 +339,21 @@ resource_priority_cmp(const void *a, const void *b) static void ResourceOwnerSort(ResourceOwner owner) { - ResourceElem *items; - uint32 nitems; - if (owner->nhash == 0) { + ResourceElem *items; + uint32 nitems; + items = owner->arr; nitems = owner->narr; + + qsort(items, nitems, sizeof(ResourceElem), resource_priority_cmp); } else { + ResourceHashElem *items; + uint32 nitems; + /* * Compact the hash table, so that all the elements are in the * beginning of the 'hash' array, with no empty elements. @@ -324,7 +380,9 @@ ResourceOwnerSort(ResourceOwner owner) Assert(dst + owner->narr <= owner->capacity); for (int idx = 0; idx < owner->narr; idx++) { - owner->hash[dst] = owner->arr[idx]; + owner->hash[dst].item = owner->arr[idx].item; + owner->hash[dst].kind = owner->arr[idx].kind; + owner->hash[dst].count = 1; dst++; } Assert(dst == owner->nhash + owner->narr); @@ -333,9 +391,9 @@ ResourceOwnerSort(ResourceOwner owner) items = owner->hash; nitems = owner->nhash; - } - qsort(items, nitems, sizeof(ResourceElem), resource_priority_cmp); + qsort(items, nitems, sizeof(ResourceHashElem), resource_priority_hash_cmp); + } } /* @@ -345,9 +403,6 @@ static void ResourceOwnerReleaseAll(ResourceOwner owner, ResourceReleasePhase phase, bool printLeakWarnings) { - ResourceElem *items; - uint32 nitems; - /* * ResourceOwnerSort must've been called already. All the resources are * either in the array or the hash. @@ -356,49 +411,93 @@ ResourceOwnerReleaseAll(ResourceOwner owner, ResourceReleasePhase phase, Assert(owner->sorted); if (owner->nhash == 0) { + ResourceElem *items; + uint32 nitems; + items = owner->arr; nitems = owner->narr; + + /* + * 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. + */ + while (nitems > 0) + { + uint32 idx = nitems - 1; + Datum value = items[idx].item; + const ResourceOwnerDesc *kind = items[idx].kind; + + if (kind->release_phase > phase) + break; + Assert(kind->release_phase == phase); + + if (printLeakWarnings) + { + char *res_str; + + res_str = kind->DebugPrint ? + kind->DebugPrint(value) + : psprintf("%s %p", kind->name, DatumGetPointer(value)); + pfree(res_str); + } + + kind->ReleaseResource(value); + + nitems--; + } + + owner->narr = nitems; } else { + ResourceHashElem *items; + uint32 nitems; + Assert(owner->narr == 0); items = owner->hash; nitems = owner->nhash; - } - /* - * 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. - */ - while (nitems > 0) - { - uint32 idx = nitems - 1; - Datum value = items[idx].item; - const ResourceOwnerDesc *kind = items[idx].kind; + /* + * 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. + */ + while (nitems > 0) + { + uint32 idx = nitems - 1; + Datum value = items[idx].item; + const ResourceOwnerDesc *kind = items[idx].kind; + uint32 count = items[idx].count; - if (kind->release_phase > phase) - break; - Assert(kind->release_phase == phase); + if (kind->release_phase > phase) + break; + Assert(kind->release_phase == phase); - if (printLeakWarnings) - { - char *res_str; + if (printLeakWarnings) + { + char *res_str; + + res_str = kind->DebugPrint ? + kind->DebugPrint(value) + : psprintf("%s %p", kind->name, DatumGetPointer(value)); + pfree(res_str); + } + + /* invoke the release callback for each reference */ + while (count > 0) + { + kind->ReleaseResource(value); + count--; + } - res_str = kind->DebugPrint ? - kind->DebugPrint(value) - : psprintf("%s %p", kind->name, DatumGetPointer(value)); - elog(WARNING, "resource was not closed: %s", res_str); - pfree(res_str); + nitems--; } - kind->ReleaseResource(value); - nitems--; - } - if (owner->nhash == 0) - owner->narr = nitems; - else + owner->nhash = nitems; + } } @@ -466,16 +565,16 @@ ResourceOwnerEnlarge(ResourceOwner owner) uint32 i, oldcap, newcap; - ResourceElem *oldhash; - ResourceElem *newhash; + ResourceHashElem *oldhash; + ResourceHashElem *newhash; oldhash = owner->hash; oldcap = owner->capacity; /* Double the capacity (it must stay a power of 2!) */ newcap = (oldcap > 0) ? oldcap * 2 : RESOWNER_HASH_INIT_SIZE; - newhash = (ResourceElem *) MemoryContextAllocZero(TopMemoryContext, - newcap * sizeof(ResourceElem)); + newhash = (ResourceHashElem *) MemoryContextAllocZero(TopMemoryContext, + newcap * sizeof(ResourceHashElem)); /* * We assume we can't fail below this point, so OK to scribble on the @@ -496,7 +595,8 @@ ResourceOwnerEnlarge(ResourceOwner owner) for (i = 0; i < oldcap; i++) { if (oldhash[i].kind != NULL) - ResourceOwnerAddToHash(owner, oldhash[i].item, oldhash[i].kind); + ResourceOwnerAddToHash(owner, oldhash[i].item, oldhash[i].kind, + oldhash[i].count); } /* And release old hash table. */ @@ -506,7 +606,7 @@ ResourceOwnerEnlarge(ResourceOwner owner) /* Move items from the array to the hash */ for (int i = 0; i < owner->narr; i++) - ResourceOwnerAddToHash(owner, owner->arr[i].item, owner->arr[i].kind); + ResourceOwnerAddToHash(owner, owner->arr[i].item, owner->arr[i].kind, 1); owner->narr = 0; Assert(owner->nhash <= owner->grow_at); @@ -555,7 +655,8 @@ ResourceOwnerRemember(ResourceOwner owner, Datum value, const ResourceOwnerDesc * Note: Forgetting a resource does not guarantee that there is room to * remember a new resource. One exception is when you forget the most * recently remembered resource; that does make room for a new remember call. - * Some code callers rely on that exception. + * Some code callers rely on that exception. The deduplication does not + * change this, as it only applies to the hash table. */ void ResourceOwnerForget(ResourceOwner owner, Datum value, const ResourceOwnerDesc *kind) @@ -597,8 +698,17 @@ ResourceOwnerForget(ResourceOwner owner, Datum value, const ResourceOwnerDesc *k if (owner->hash[idx].item == value && owner->hash[idx].kind == kind) { + /* decrement the count for entry for multiple references */ + if (owner->hash[idx].count > 1) + { + owner->hash[idx].count--; + return; + } + + /* remove the entry entirely */ owner->hash[idx].item = (Datum) 0; owner->hash[idx].kind = NULL; + owner->hash[idx].count = 0; owner->nhash--; #ifdef RESOWNER_STATS @@ -847,12 +957,19 @@ ResourceOwnerReleaseAllOfKind(ResourceOwner owner, const ResourceOwnerDesc *kind if (owner->hash[i].kind == kind) { Datum value = owner->hash[i].item; + uint32 count = owner->hash[i].count; owner->hash[i].item = (Datum) 0; owner->hash[i].kind = NULL; + owner->hash[i].count = 0; owner->nhash--; - kind->ReleaseResource(value); + /* invoke the release callback once for each reference */ + while (count > 0) + { + kind->ReleaseResource(value); + count--; + } } } owner->releasing = false; -- 2.51.0