public inbox for [email protected]help / color / mirror / Atom feed
Re: type cache cleanup improvements 28+ messages / 10 participants [nested] [flat]
* Re: type cache cleanup improvements @ 2024-03-11 12:24 Aleksander Alekseev <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Aleksander Alekseev @ 2024-03-11 12:24 UTC (permalink / raw) To: pgsql-hackers; +Cc: Tom Lane <[email protected]>; Teodor Sigaev <[email protected]> Hi, > Yep, exacly. One time from 2^32 we reset whole cache instead of one (or several) > entry with hash value = 0. Got it. Here is an updated patch where I added a corresponding comment. Now the patch LGTM. I'm going to change its status to RfC unless anyone wants to review it too. -- Best regards, Aleksander Alekseev Attachments: [application/octet-stream] v4-0001-Improve-performance-of-type-cache-cleanup.patch (16.6K, ../../CAJ7c6TPyfqAEbyXcAx7zZKrh8Wyp2H3fkmhEeVnO8O97japYtQ@mail.gmail.com/2-v4-0001-Improve-performance-of-type-cache-cleanup.patch) download | inline diff: From 0d6171e0ad039af624be31f44eb60c3788e275b7 Mon Sep 17 00:00:00 2001 From: Aleksander Alekseev <[email protected]> Date: Tue, 5 Mar 2024 14:13:40 +0300 Subject: [PATCH v4] Improve performance of type cache cleanup This patch significantly improves the performance in cases when user creates several thousands of temporary tables. Firstly, the patch adds a local map between a relation and its type as it was previously suggested in the comments for TypeCacheRelCallback(). Unfortunately, syscache can't be used here because TypeCacheRelCallback() can be called outside of a transaction. Secondly, it modifies TypeCacheTypCallback() so that it finds an entry in the hash table by O(1). Linear scan was used previously because the hash algorithm differed from the one used by syscache. Additionally dynahash had no interface for finding entries with a given hash value. Author: Teodor Sigaev Reviewed-by: Aleksander Alekseev Discussion: https://postgr.es/m/[email protected] --- src/backend/utils/cache/typcache.c | 211 +++++++++++++++++++++-------- src/backend/utils/hash/dynahash.c | 106 +++++++++------ src/include/utils/hsearch.h | 4 + 3 files changed, 221 insertions(+), 100 deletions(-) diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c index 0d4d0b0a15..9145088f44 100644 --- a/src/backend/utils/cache/typcache.c +++ b/src/backend/utils/cache/typcache.c @@ -77,6 +77,15 @@ /* The main type cache hashtable searched by lookup_type_cache */ static HTAB *TypeCacheHash = NULL; +/* The map from relation's oid to its type oid */ +typedef struct mapRelTypeEntry +{ + Oid typrelid; + Oid type_id; +} mapRelTypeEntry; + +static HTAB *mapRelType = NULL; + /* List of type cache entries for domain types */ static TypeCacheEntry *firstDomainTypeEntry = NULL; @@ -330,6 +339,15 @@ static TupleDesc find_or_make_matching_shared_tupledesc(TupleDesc tupdesc); static dsa_pointer share_tupledesc(dsa_area *area, TupleDesc tupdesc, uint32 typmod); +/* + * Hash function compatible with one-arg system cache hash function. + */ +static uint32 +type_cache_syshash(const void *key, Size keysize) +{ + Assert(keysize == sizeof(Oid)); + return GetSysCacheHashValue1(TYPEOID, ObjectIdGetDatum(*(const Oid*)key)); +} /* * lookup_type_cache @@ -355,8 +373,20 @@ lookup_type_cache(Oid type_id, int flags) ctl.keysize = sizeof(Oid); ctl.entrysize = sizeof(TypeCacheEntry); + /* + * TypeEntry takes hash value from the system cache. For TypeCacheHash + * we use the same hash in order to speedup search by hash value. This + * is used by hash_seq_init_with_hash_value(). + */ + ctl.hash = type_cache_syshash; + TypeCacheHash = hash_create("Type information cache", 64, - &ctl, HASH_ELEM | HASH_BLOBS); + &ctl, HASH_ELEM | HASH_FUNCTION); + + ctl.keysize = sizeof(Oid); + ctl.entrysize = sizeof(mapRelTypeEntry); + mapRelType = hash_create("Map reloid to typeoid", 64, + &ctl, HASH_ELEM | HASH_BLOBS); /* Also set up callbacks for SI invalidations */ CacheRegisterRelcacheCallback(TypeCacheRelCallback, (Datum) 0); @@ -407,8 +437,7 @@ lookup_type_cache(Oid type_id, int flags) /* These fields can never change, by definition */ typentry->type_id = type_id; - typentry->type_id_hash = GetSysCacheHashValue1(TYPEOID, - ObjectIdGetDatum(type_id)); + typentry->type_id_hash = get_hash_value(TypeCacheHash, &type_id); /* Keep this part in sync with the code below */ typentry->typlen = typtup->typlen; @@ -470,6 +499,24 @@ lookup_type_cache(Oid type_id, int flags) ReleaseSysCache(tp); } + /* + * Add a record to the relation->type map. We don't bother if type will + * become disconnected from the relation. Although it seems to be impossible, + * storing old data is safe in any case. In the worst case scenario we will + * just do an extra cleanup of a cache entry. + */ + if (OidIsValid(typentry->typrelid) && typentry->typtype == TYPTYPE_COMPOSITE) + { + mapRelTypeEntry *relentry; + + relentry = (mapRelTypeEntry*) hash_search(mapRelType, + &typentry->typrelid, + HASH_ENTER, NULL); + + relentry->typrelid = typentry->typrelid; + relentry->type_id = typentry->type_id; + } + /* * Look up opclasses if we haven't already and any dependent info is * requested. @@ -2264,6 +2311,33 @@ SharedRecordTypmodRegistryAttach(SharedRecordTypmodRegistry *registry) CurrentSession->shared_typmod_table = typmod_table; } +static void +invalidateCompositeTypeCacheEntry(TypeCacheEntry *typentry) +{ + /* Delete tupdesc if we have it */ + if (typentry->tupDesc != NULL) + { + /* + * Release our refcount and free the tupdesc if none remain. We can't + * use DecrTupleDescRefCount here because this reference is not logged + * by the current resource owner. + */ + Assert(typentry->tupDesc->tdrefcount > 0); + if (--typentry->tupDesc->tdrefcount == 0) + FreeTupleDesc(typentry->tupDesc); + typentry->tupDesc = NULL; + + /* + * Also clear tupDesc_identifier, so that anyone watching + * it will realize that the tupdesc has changed. + */ + typentry->tupDesc_identifier = 0; + } + + /* Reset equality/comparison/hashing validity information */ + typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; +} + /* * TypeCacheRelCallback * Relcache inval callback function @@ -2273,63 +2347,46 @@ SharedRecordTypmodRegistryAttach(SharedRecordTypmodRegistry *registry) * whatever info we have cached about the composite type's comparability. * * This is called when a relcache invalidation event occurs for the given - * relid. We must scan the whole typcache hash since we don't know the - * type OID corresponding to the relid. We could do a direct search if this - * were a syscache-flush callback on pg_type, but then we would need all - * ALTER-TABLE-like commands that could modify a rowtype to issue syscache - * invals against the rel's pg_type OID. The extra SI signaling could very - * well cost more than we'd save, since in most usages there are not very - * many entries in a backend's typcache. The risk of bugs-of-omission seems - * high, too. - * - * Another possibility, with only localized impact, is to maintain a second - * hashtable that indexes composite-type typcache entries by their typrelid. - * But it's still not clear it's worth the trouble. + * relid. We can't use syscache to find a type corresponding to the given + * relation because the code can be called outside of transaction. Thus we use + * a dedicated relid->type map, mapRelType. */ static void TypeCacheRelCallback(Datum arg, Oid relid) { - HASH_SEQ_STATUS status; TypeCacheEntry *typentry; - /* TypeCacheHash must exist, else this callback wouldn't be registered */ - hash_seq_init(&status, TypeCacheHash); - while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) + /* + * mapRelType and TypeCacheHash should exist, otherwise this callback + * wouldn't be registered + */ + + if (OidIsValid(relid)) { - if (typentry->typtype == TYPTYPE_COMPOSITE) + mapRelTypeEntry *relentry; + + relentry = (mapRelTypeEntry *) hash_search(mapRelType, + &relid, + HASH_FIND, NULL); + + if (relentry != NULL) { - /* Skip if no match, unless we're zapping all composite types */ - if (relid != typentry->typrelid && relid != InvalidOid) - continue; + typentry = (TypeCacheEntry *) hash_search(TypeCacheHash, + &relentry->type_id, + HASH_FIND, NULL); - /* Delete tupdesc if we have it */ - if (typentry->tupDesc != NULL) + if (typentry != NULL) { - /* - * Release our refcount, and free the tupdesc if none remain. - * (Can't use DecrTupleDescRefCount because this reference is - * not logged in current resource owner.) - */ - Assert(typentry->tupDesc->tdrefcount > 0); - if (--typentry->tupDesc->tdrefcount == 0) - FreeTupleDesc(typentry->tupDesc); - typentry->tupDesc = NULL; + Assert(typentry->typtype == TYPTYPE_COMPOSITE); + Assert(relid == typentry->typrelid); - /* - * Also clear tupDesc_identifier, so that anything watching - * that will realize that the tupdesc has possibly changed. - * (Alternatively, we could specify that to detect possible - * tupdesc change, one must check for tupDesc != NULL as well - * as tupDesc_identifier being the same as what was previously - * seen. That seems error-prone.) - */ - typentry->tupDesc_identifier = 0; + invalidateCompositeTypeCacheEntry(typentry); } - - /* Reset equality/comparison/hashing validity information */ - typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; } - else if (typentry->typtype == TYPTYPE_DOMAIN) + + for (typentry = firstDomainTypeEntry; + typentry != NULL; + typentry = typentry->nextDomain) { /* * If it's domain over composite, reset flags. (We don't bother @@ -2341,6 +2398,35 @@ TypeCacheRelCallback(Datum arg, Oid relid) typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; } } + else + { + HASH_SEQ_STATUS status; + + /* + * Relid = 0, so we need to reset all composite types in cache. Also, we + * should reset flags for domain types, and we loop over all entries + * in hash, so, do it in a single scan. + */ + hash_seq_init(&status, TypeCacheHash); + while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) + { + if (typentry->typtype == TYPTYPE_COMPOSITE) + { + invalidateCompositeTypeCacheEntry(typentry); + } + else if (typentry->typtype == TYPTYPE_DOMAIN) + { + /* + * If it's domain over composite, reset flags. (We don't bother + * trying to determine whether the specific base type needs a + * reset.) Note that if we haven't determined whether the base + * type is composite, we don't need to reset anything. + */ + if (typentry->flags & TCFLAGS_DOMAIN_BASE_IS_COMPOSITE) + typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; + } + } + } } /* @@ -2358,20 +2444,27 @@ TypeCacheTypCallback(Datum arg, int cacheid, uint32 hashvalue) TypeCacheEntry *typentry; /* TypeCacheHash must exist, else this callback wouldn't be registered */ - hash_seq_init(&status, TypeCacheHash); + + /* + * By convection, zero hash value is passed to the callback as a sign + * that it's time to invalidate the cache. See sinval.c, inval.c and + * InvalidateSystemCachesExtended(). + */ + if (hashvalue == 0) + hash_seq_init(&status, TypeCacheHash); + else + hash_seq_init_with_hash_value(&status, TypeCacheHash, hashvalue); + while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) { - /* Is this the targeted type row (or it's a total cache flush)? */ - if (hashvalue == 0 || typentry->type_id_hash == hashvalue) - { - /* - * Mark the data obtained directly from pg_type as invalid. Also, - * if it's a domain, typnotnull might've changed, so we'll need to - * recalculate its constraints. - */ - typentry->flags &= ~(TCFLAGS_HAVE_PG_TYPE_DATA | - TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS); - } + Assert(hashvalue == 0 || typentry->type_id_hash == hashvalue); + /* + * Mark the data obtained directly from pg_type as invalid. Also, + * if it's a domain, typnotnull might've changed, so we'll need to + * recalculate its constraints. + */ + typentry->flags &= ~(TCFLAGS_HAVE_PG_TYPE_DATA | + TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS); } } diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c index a4152080b5..3fc8b7ff5e 100644 --- a/src/backend/utils/hash/dynahash.c +++ b/src/backend/utils/hash/dynahash.c @@ -962,6 +962,33 @@ hash_search(HTAB *hashp, foundPtr); } +/* + * Do initial lookup of a bucket by the given hash value. + */ +static HASHBUCKET* +hash_initial_lookup(HTAB *hashp, uint32 hashvalue, uint32 *bucket) +{ + HASHHDR *hctl = hashp->hctl; + HASHSEGMENT segp; + long segment_num; + long segment_ndx; + + /* + * Do the initial lookup + */ + *bucket = calc_bucket(hctl, hashvalue); + + segment_num = *bucket >> hashp->sshift; + segment_ndx = MOD(*bucket, hashp->ssize); + + segp = hashp->dir[segment_num]; + + if (segp == NULL) + hash_corrupted(hashp); + + return &segp[segment_ndx]; +} + void * hash_search_with_hash_value(HTAB *hashp, const void *keyPtr, @@ -973,9 +1000,6 @@ hash_search_with_hash_value(HTAB *hashp, int freelist_idx = FREELIST_IDX(hctl, hashvalue); Size keysize; uint32 bucket; - long segment_num; - long segment_ndx; - HASHSEGMENT segp; HASHBUCKET currBucket; HASHBUCKET *prevBucketPtr; HashCompareFunc match; @@ -1008,17 +1032,7 @@ hash_search_with_hash_value(HTAB *hashp, /* * Do the initial lookup */ - bucket = calc_bucket(hctl, hashvalue); - - segment_num = bucket >> hashp->sshift; - segment_ndx = MOD(bucket, hashp->ssize); - - segp = hashp->dir[segment_num]; - - if (segp == NULL) - hash_corrupted(hashp); - - prevBucketPtr = &segp[segment_ndx]; + prevBucketPtr = hash_initial_lookup(hashp, hashvalue, &bucket); currBucket = *prevBucketPtr; /* @@ -1159,14 +1173,10 @@ hash_update_hash_key(HTAB *hashp, const void *newKeyPtr) { HASHELEMENT *existingElement = ELEMENT_FROM_KEY(existingEntry); - HASHHDR *hctl = hashp->hctl; uint32 newhashvalue; Size keysize; uint32 bucket; uint32 newbucket; - long segment_num; - long segment_ndx; - HASHSEGMENT segp; HASHBUCKET currBucket; HASHBUCKET *prevBucketPtr; HASHBUCKET *oldPrevPtr; @@ -1187,17 +1197,7 @@ hash_update_hash_key(HTAB *hashp, * this to be able to unlink it from its hash chain, but as a side benefit * we can verify the validity of the passed existingEntry pointer. */ - bucket = calc_bucket(hctl, existingElement->hashvalue); - - segment_num = bucket >> hashp->sshift; - segment_ndx = MOD(bucket, hashp->ssize); - - segp = hashp->dir[segment_num]; - - if (segp == NULL) - hash_corrupted(hashp); - - prevBucketPtr = &segp[segment_ndx]; + prevBucketPtr = hash_initial_lookup(hashp, existingElement->hashvalue, &bucket); currBucket = *prevBucketPtr; while (currBucket != NULL) @@ -1219,18 +1219,7 @@ hash_update_hash_key(HTAB *hashp, * chain we want to put the entry into. */ newhashvalue = hashp->hash(newKeyPtr, hashp->keysize); - - newbucket = calc_bucket(hctl, newhashvalue); - - segment_num = newbucket >> hashp->sshift; - segment_ndx = MOD(newbucket, hashp->ssize); - - segp = hashp->dir[segment_num]; - - if (segp == NULL) - hash_corrupted(hashp); - - prevBucketPtr = &segp[segment_ndx]; + prevBucketPtr = hash_initial_lookup(hashp, newhashvalue, &newbucket); currBucket = *prevBucketPtr; /* @@ -1423,10 +1412,27 @@ hash_seq_init(HASH_SEQ_STATUS *status, HTAB *hashp) status->hashp = hashp; status->curBucket = 0; status->curEntry = NULL; + status->hasHashvalue = false; if (!hashp->frozen) register_seq_scan(hashp); } +/* + * Same as above but scan by the given hash value. + * See also hash_seq_search(). + */ +void +hash_seq_init_with_hash_value(HASH_SEQ_STATUS *status, HTAB *hashp, + uint32 hashvalue) +{ + hash_seq_init(status, hashp); + + status->hasHashvalue = true; + status->hashvalue = hashvalue; + + status->curEntry = *hash_initial_lookup(hashp, hashvalue, &status->curBucket); +} + void * hash_seq_search(HASH_SEQ_STATUS *status) { @@ -1440,6 +1446,24 @@ hash_seq_search(HASH_SEQ_STATUS *status) uint32 curBucket; HASHELEMENT *curElem; + if (status->hasHashvalue) + { + /* + * Scan entries only in the current bucket because only this bucket can + * contain entries with the given hash value. + */ + while ((curElem = status->curEntry) != NULL) + { + status->curEntry = curElem->link; + if (status->hashvalue != curElem->hashvalue) + continue; + return (void *) ELEMENTKEY(curElem); + } + + hash_seq_term(status); + return NULL; + } + if ((curElem = status->curEntry) != NULL) { /* Continuing scan of curBucket... */ diff --git a/src/include/utils/hsearch.h b/src/include/utils/hsearch.h index da26941f6d..c99d74625f 100644 --- a/src/include/utils/hsearch.h +++ b/src/include/utils/hsearch.h @@ -122,6 +122,8 @@ typedef struct HTAB *hashp; uint32 curBucket; /* index of current bucket */ HASHELEMENT *curEntry; /* current entry in bucket */ + bool hasHashvalue; /* true if hashvalue was provided */ + uint32 hashvalue; /* hashvalue to start seqscan over hash */ } HASH_SEQ_STATUS; /* @@ -141,6 +143,8 @@ extern bool hash_update_hash_key(HTAB *hashp, void *existingEntry, const void *newKeyPtr); extern long hash_get_num_entries(HTAB *hashp); extern void hash_seq_init(HASH_SEQ_STATUS *status, HTAB *hashp); +extern void hash_seq_init_with_hash_value(HASH_SEQ_STATUS *status, HTAB *hashp, + uint32 hashvalue); extern void *hash_seq_search(HASH_SEQ_STATUS *status); extern void hash_seq_term(HASH_SEQ_STATUS *status); extern void hash_freeze(HTAB *hashp); -- 2.44.0 ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-03-12 15:55 Teodor Sigaev <[email protected]> parent: Aleksander Alekseev <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Teodor Sigaev @ 2024-03-12 15:55 UTC (permalink / raw) To: Aleksander Alekseev <[email protected]>; pgsql-hackers; +Cc: Tom Lane <[email protected]> > Got it. Here is an updated patch where I added a corresponding comment. Thank you! Playing around I found one more place which could easily modified with hash_seq_init_with_hash_value() call. -- Teodor Sigaev E-mail: [email protected] WWW: http://www.sigaev.ru/ Attachments: [text/x-patch] attoptcache-v1.patch (2.4K, ../../[email protected]/2-attoptcache-v1.patch) download | inline diff: diff --git a/src/backend/utils/cache/attoptcache.c b/src/backend/utils/cache/attoptcache.c index af978ccd4b1..28980620662 100644 --- a/src/backend/utils/cache/attoptcache.c +++ b/src/backend/utils/cache/attoptcache.c @@ -44,12 +44,10 @@ typedef struct /* * InvalidateAttoptCacheCallback - * Flush all cache entries when pg_attribute is updated. + * Flush cache entry (or entries) when pg_attribute is updated. * * When pg_attribute is updated, we must flush the cache entry at least - * for that attribute. Currently, we just flush them all. Since attribute - * options are not currently used in performance-critical paths (such as - * query execution), this seems OK. + * for that attribute. */ static void InvalidateAttoptCacheCallback(Datum arg, int cacheid, uint32 hashvalue) @@ -57,7 +55,16 @@ InvalidateAttoptCacheCallback(Datum arg, int cacheid, uint32 hashvalue) HASH_SEQ_STATUS status; AttoptCacheEntry *attopt; - hash_seq_init(&status, AttoptCacheHash); + /* + * By convection, zero hash value is passed to the callback as a sign + * that it's time to invalidate the cache. See sinval.c, inval.c and + * InvalidateSystemCachesExtended(). + */ + if (hashvalue == 0) + hash_seq_init(&status, AttoptCacheHash); + else + hash_seq_init_with_hash_value(&status, AttoptCacheHash, hashvalue); + while ((attopt = (AttoptCacheEntry *) hash_seq_search(&status)) != NULL) { if (attopt->opts) @@ -70,6 +77,17 @@ InvalidateAttoptCacheCallback(Datum arg, int cacheid, uint32 hashvalue) } } +/* + * Hash function compatible with two-arg system cache hash function. + */ +static uint32 +relatt_cache_syshash(const void *key, Size keysize) +{ + const AttoptCacheKey* ckey = key; + + return GetSysCacheHashValue2(ATTNUM, ckey->attrelid, ckey->attnum); +} + /* * InitializeAttoptCache * Initialize the attribute options cache. @@ -82,9 +100,17 @@ InitializeAttoptCache(void) /* Initialize the hash table. */ ctl.keysize = sizeof(AttoptCacheKey); ctl.entrysize = sizeof(AttoptCacheEntry); + + /* + * AttoptCacheEntry takes hash value from the system cache. For + * AttoptCacheHash we use the same hash in order to speedup search by hash + * value. This is used by hash_seq_init_with_hash_value(). + */ + ctl.hash = relatt_cache_syshash; + AttoptCacheHash = hash_create("Attopt cache", 256, &ctl, - HASH_ELEM | HASH_BLOBS); + HASH_ELEM | HASH_FUNCTION); /* Make sure we've initialized CacheMemoryContext. */ if (!CacheMemoryContext) ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-03-13 05:47 Michael Paquier <[email protected]> parent: Teodor Sigaev <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Michael Paquier @ 2024-03-13 05:47 UTC (permalink / raw) To: Teodor Sigaev <[email protected]>; +Cc: Aleksander Alekseev <[email protected]>; pgsql-hackers; Tom Lane <[email protected]> On Tue, Mar 12, 2024 at 06:55:41PM +0300, Teodor Sigaev wrote: > Playing around I found one more place which could easily modified with > hash_seq_init_with_hash_value() call. I think that this patch should be split for clarity, as there are a few things that are independently useful. I guess something like that: - Introduction of hash_initial_lookup(), that simplifies 3 places of dynahash.c where the same code is used. The routine should be inlined. - The split in hash_seq_search to force a different type of search is weird, complicating the dynahash interface by hiding what seems like a search mode. Rather than hasHashvalue that's hidden in the middle of HASH_SEQ_STATUS, could it be better to have an entirely different API for the search? That should be a patch on its own, as well. - The typcache changes. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-03-13 13:40 Teodor Sigaev <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Teodor Sigaev @ 2024-03-13 13:40 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Aleksander Alekseev <[email protected]>; pgsql-hackers; Tom Lane <[email protected]> > I think that this patch should be split for clarity, as there are a > few things that are independently useful. I guess something like > that: Done, all patches should be applied consequentially. > - The typcache changes. 01-map_rel_to_type.v5.patch adds map relation to its type > - Introduction of hash_initial_lookup(), that simplifies 3 places of > dynahash.c where the same code is used. The routine should be > inlined. > - The split in hash_seq_search to force a different type of search is > weird, complicating the dynahash interface by hiding what seems like a > search mode. Rather than hasHashvalue that's hidden in the middle of > HASH_SEQ_STATUS, could it be better to have an entirely different API > for the search? That should be a patch on its own, as well. 02-hash_seq_init_with_hash_value.v5.patch - introduces a hash_seq_init_with_hash_value() method. hash_initial_lookup() is marked as inline, but I suppose, modern compilers are smart enough to inline it automatically. Using separate interface for scanning hash with hash value will make scan code more ugly in case when we need to use special value of hash value as it is done in cache's scans. Look, instead of this simplified code: if (hashvalue == 0) hash_seq_init(&status, TypeCacheHash); else hash_seq_init_with_hash_value(&status, TypeCacheHash, hashvalue); while ((typentry = hash_seq_search(&status)) != NULL) { ... } we will need to code something like that: if (hashvalue == 0) { hash_seq_init(&status, TypeCacheHash); while ((typentry = hash_seq_search(&status)) != NULL) { ... } } else { hash_seq_init_with_hash_value(&status, TypeCacheHash, hashvalue); while ((typentry = hash_seq_search_with_hash_value(&status)) != NULL) { ... } } Or I didn't understand you. I thought about integrate check inside existing loop in hash_seq_search() : + rerun: if ((curElem = status->curEntry) != NULL) { /* Continuing scan of curBucket... */ status->curEntry = curElem->link; if (status->curEntry == NULL) /* end of this bucket */ + { + if (status->hasHashvalue) + hash_seq_term(status); ++status->curBucket; + } + else if (status->hasHashvalue && status->hashvalue != + curElem->hashvalue) + goto rerun; return (void *) ELEMENTKEY(curElem); } But for me it looks weird and adds some checks which will takes some CPU time. 03-att_with_hash_value.v5.patch - adds usage of previous patch. -- Teodor Sigaev E-mail: [email protected] WWW: http://www.sigaev.ru/ Attachments: [text/x-patch] 03-att_with_hash_value.v5.patch (10.9K, ../../[email protected]/2-03-att_with_hash_value.v5.patch) download | inline diff: diff --git a/src/backend/utils/cache/attoptcache.c b/src/backend/utils/cache/attoptcache.c index af978ccd4b1..3a18b2e9a77 100644 --- a/src/backend/utils/cache/attoptcache.c +++ b/src/backend/utils/cache/attoptcache.c @@ -44,12 +44,10 @@ typedef struct /* * InvalidateAttoptCacheCallback - * Flush all cache entries when pg_attribute is updated. + * Flush cache entry (or entries) when pg_attribute is updated. * * When pg_attribute is updated, we must flush the cache entry at least - * for that attribute. Currently, we just flush them all. Since attribute - * options are not currently used in performance-critical paths (such as - * query execution), this seems OK. + * for that attribute. */ static void InvalidateAttoptCacheCallback(Datum arg, int cacheid, uint32 hashvalue) @@ -57,7 +55,16 @@ InvalidateAttoptCacheCallback(Datum arg, int cacheid, uint32 hashvalue) HASH_SEQ_STATUS status; AttoptCacheEntry *attopt; - hash_seq_init(&status, AttoptCacheHash); + /* + * By convection, zero hash value is passed to the callback as a sign + * that it's time to invalidate the cache. See sinval.c, inval.c and + * InvalidateSystemCachesExtended(). + */ + if (hashvalue == 0) + hash_seq_init(&status, AttoptCacheHash); + else + hash_seq_init_with_hash_value(&status, AttoptCacheHash, hashvalue); + while ((attopt = (AttoptCacheEntry *) hash_seq_search(&status)) != NULL) { if (attopt->opts) @@ -70,6 +77,18 @@ InvalidateAttoptCacheCallback(Datum arg, int cacheid, uint32 hashvalue) } } +/* + * Hash function compatible with two-arg system cache hash function. + */ +static uint32 +relatt_cache_syshash(const void *key, Size keysize) +{ + const AttoptCacheKey* ckey = key; + + Assert(keysize == sizeof(*ckey)); + return GetSysCacheHashValue2(ATTNUM, ckey->attrelid, ckey->attnum); +} + /* * InitializeAttoptCache * Initialize the attribute options cache. @@ -82,9 +101,17 @@ InitializeAttoptCache(void) /* Initialize the hash table. */ ctl.keysize = sizeof(AttoptCacheKey); ctl.entrysize = sizeof(AttoptCacheEntry); + + /* + * AttoptCacheEntry takes hash value from the system cache. For + * AttoptCacheHash we use the same hash in order to speedup search by hash + * value. This is used by hash_seq_init_with_hash_value(). + */ + ctl.hash = relatt_cache_syshash; + AttoptCacheHash = hash_create("Attopt cache", 256, &ctl, - HASH_ELEM | HASH_BLOBS); + HASH_ELEM | HASH_FUNCTION); /* Make sure we've initialized CacheMemoryContext. */ if (!CacheMemoryContext) diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c index 7936c3b46d0..9145088f44d 100644 --- a/src/backend/utils/cache/typcache.c +++ b/src/backend/utils/cache/typcache.c @@ -339,6 +339,15 @@ static TupleDesc find_or_make_matching_shared_tupledesc(TupleDesc tupdesc); static dsa_pointer share_tupledesc(dsa_area *area, TupleDesc tupdesc, uint32 typmod); +/* + * Hash function compatible with one-arg system cache hash function. + */ +static uint32 +type_cache_syshash(const void *key, Size keysize) +{ + Assert(keysize == sizeof(Oid)); + return GetSysCacheHashValue1(TYPEOID, ObjectIdGetDatum(*(const Oid*)key)); +} /* * lookup_type_cache @@ -364,8 +373,15 @@ lookup_type_cache(Oid type_id, int flags) ctl.keysize = sizeof(Oid); ctl.entrysize = sizeof(TypeCacheEntry); + /* + * TypeEntry takes hash value from the system cache. For TypeCacheHash + * we use the same hash in order to speedup search by hash value. This + * is used by hash_seq_init_with_hash_value(). + */ + ctl.hash = type_cache_syshash; + TypeCacheHash = hash_create("Type information cache", 64, - &ctl, HASH_ELEM | HASH_BLOBS); + &ctl, HASH_ELEM | HASH_FUNCTION); ctl.keysize = sizeof(Oid); ctl.entrysize = sizeof(mapRelTypeEntry); @@ -421,8 +437,7 @@ lookup_type_cache(Oid type_id, int flags) /* These fields can never change, by definition */ typentry->type_id = type_id; - typentry->type_id_hash = GetSysCacheHashValue1(TYPEOID, - ObjectIdGetDatum(type_id)); + typentry->type_id_hash = get_hash_value(TypeCacheHash, &type_id); /* Keep this part in sync with the code below */ typentry->typlen = typtup->typlen; @@ -2429,20 +2444,27 @@ TypeCacheTypCallback(Datum arg, int cacheid, uint32 hashvalue) TypeCacheEntry *typentry; /* TypeCacheHash must exist, else this callback wouldn't be registered */ - hash_seq_init(&status, TypeCacheHash); + + /* + * By convection, zero hash value is passed to the callback as a sign + * that it's time to invalidate the cache. See sinval.c, inval.c and + * InvalidateSystemCachesExtended(). + */ + if (hashvalue == 0) + hash_seq_init(&status, TypeCacheHash); + else + hash_seq_init_with_hash_value(&status, TypeCacheHash, hashvalue); + while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) { - /* Is this the targeted type row (or it's a total cache flush)? */ - if (hashvalue == 0 || typentry->type_id_hash == hashvalue) - { - /* - * Mark the data obtained directly from pg_type as invalid. Also, - * if it's a domain, typnotnull might've changed, so we'll need to - * recalculate its constraints. - */ - typentry->flags &= ~(TCFLAGS_HAVE_PG_TYPE_DATA | - TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS); - } + Assert(hashvalue == 0 || typentry->type_id_hash == hashvalue); + /* + * Mark the data obtained directly from pg_type as invalid. Also, + * if it's a domain, typnotnull might've changed, so we'll need to + * recalculate its constraints. + */ + typentry->flags &= ~(TCFLAGS_HAVE_PG_TYPE_DATA | + TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS); } } diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c index a4152080b5d..dfea7a904e2 100644 --- a/src/backend/utils/hash/dynahash.c +++ b/src/backend/utils/hash/dynahash.c @@ -962,6 +962,33 @@ hash_search(HTAB *hashp, foundPtr); } +/* + * Do initial lookup of a bucket by the given hash value. + */ +static inline HASHBUCKET* +hash_initial_lookup(HTAB *hashp, uint32 hashvalue, uint32 *bucket) +{ + HASHHDR *hctl = hashp->hctl; + HASHSEGMENT segp; + long segment_num; + long segment_ndx; + + /* + * Do the initial lookup + */ + *bucket = calc_bucket(hctl, hashvalue); + + segment_num = *bucket >> hashp->sshift; + segment_ndx = MOD(*bucket, hashp->ssize); + + segp = hashp->dir[segment_num]; + + if (segp == NULL) + hash_corrupted(hashp); + + return &segp[segment_ndx]; +} + void * hash_search_with_hash_value(HTAB *hashp, const void *keyPtr, @@ -973,9 +1000,6 @@ hash_search_with_hash_value(HTAB *hashp, int freelist_idx = FREELIST_IDX(hctl, hashvalue); Size keysize; uint32 bucket; - long segment_num; - long segment_ndx; - HASHSEGMENT segp; HASHBUCKET currBucket; HASHBUCKET *prevBucketPtr; HashCompareFunc match; @@ -1008,17 +1032,7 @@ hash_search_with_hash_value(HTAB *hashp, /* * Do the initial lookup */ - bucket = calc_bucket(hctl, hashvalue); - - segment_num = bucket >> hashp->sshift; - segment_ndx = MOD(bucket, hashp->ssize); - - segp = hashp->dir[segment_num]; - - if (segp == NULL) - hash_corrupted(hashp); - - prevBucketPtr = &segp[segment_ndx]; + prevBucketPtr = hash_initial_lookup(hashp, hashvalue, &bucket); currBucket = *prevBucketPtr; /* @@ -1159,14 +1173,10 @@ hash_update_hash_key(HTAB *hashp, const void *newKeyPtr) { HASHELEMENT *existingElement = ELEMENT_FROM_KEY(existingEntry); - HASHHDR *hctl = hashp->hctl; uint32 newhashvalue; Size keysize; uint32 bucket; uint32 newbucket; - long segment_num; - long segment_ndx; - HASHSEGMENT segp; HASHBUCKET currBucket; HASHBUCKET *prevBucketPtr; HASHBUCKET *oldPrevPtr; @@ -1187,17 +1197,7 @@ hash_update_hash_key(HTAB *hashp, * this to be able to unlink it from its hash chain, but as a side benefit * we can verify the validity of the passed existingEntry pointer. */ - bucket = calc_bucket(hctl, existingElement->hashvalue); - - segment_num = bucket >> hashp->sshift; - segment_ndx = MOD(bucket, hashp->ssize); - - segp = hashp->dir[segment_num]; - - if (segp == NULL) - hash_corrupted(hashp); - - prevBucketPtr = &segp[segment_ndx]; + prevBucketPtr = hash_initial_lookup(hashp, existingElement->hashvalue, &bucket); currBucket = *prevBucketPtr; while (currBucket != NULL) @@ -1219,18 +1219,7 @@ hash_update_hash_key(HTAB *hashp, * chain we want to put the entry into. */ newhashvalue = hashp->hash(newKeyPtr, hashp->keysize); - - newbucket = calc_bucket(hctl, newhashvalue); - - segment_num = newbucket >> hashp->sshift; - segment_ndx = MOD(newbucket, hashp->ssize); - - segp = hashp->dir[segment_num]; - - if (segp == NULL) - hash_corrupted(hashp); - - prevBucketPtr = &segp[segment_ndx]; + prevBucketPtr = hash_initial_lookup(hashp, newhashvalue, &newbucket); currBucket = *prevBucketPtr; /* @@ -1423,10 +1412,27 @@ hash_seq_init(HASH_SEQ_STATUS *status, HTAB *hashp) status->hashp = hashp; status->curBucket = 0; status->curEntry = NULL; + status->hasHashvalue = false; if (!hashp->frozen) register_seq_scan(hashp); } +/* + * Same as above but scan by the given hash value. + * See also hash_seq_search(). + */ +void +hash_seq_init_with_hash_value(HASH_SEQ_STATUS *status, HTAB *hashp, + uint32 hashvalue) +{ + hash_seq_init(status, hashp); + + status->hasHashvalue = true; + status->hashvalue = hashvalue; + + status->curEntry = *hash_initial_lookup(hashp, hashvalue, &status->curBucket); +} + void * hash_seq_search(HASH_SEQ_STATUS *status) { @@ -1440,6 +1446,24 @@ hash_seq_search(HASH_SEQ_STATUS *status) uint32 curBucket; HASHELEMENT *curElem; + if (status->hasHashvalue) + { + /* + * Scan entries only in the current bucket because only this bucket can + * contain entries with the given hash value. + */ + while ((curElem = status->curEntry) != NULL) + { + status->curEntry = curElem->link; + if (status->hashvalue != curElem->hashvalue) + continue; + return (void *) ELEMENTKEY(curElem); + } + + hash_seq_term(status); + return NULL; + } + if ((curElem = status->curEntry) != NULL) { /* Continuing scan of curBucket... */ diff --git a/src/include/utils/hsearch.h b/src/include/utils/hsearch.h index da26941f6db..c99d74625f7 100644 --- a/src/include/utils/hsearch.h +++ b/src/include/utils/hsearch.h @@ -122,6 +122,8 @@ typedef struct HTAB *hashp; uint32 curBucket; /* index of current bucket */ HASHELEMENT *curEntry; /* current entry in bucket */ + bool hasHashvalue; /* true if hashvalue was provided */ + uint32 hashvalue; /* hashvalue to start seqscan over hash */ } HASH_SEQ_STATUS; /* @@ -141,6 +143,8 @@ extern bool hash_update_hash_key(HTAB *hashp, void *existingEntry, const void *newKeyPtr); extern long hash_get_num_entries(HTAB *hashp); extern void hash_seq_init(HASH_SEQ_STATUS *status, HTAB *hashp); +extern void hash_seq_init_with_hash_value(HASH_SEQ_STATUS *status, HTAB *hashp, + uint32 hashvalue); extern void *hash_seq_search(HASH_SEQ_STATUS *status); extern void hash_seq_term(HASH_SEQ_STATUS *status); extern void hash_freeze(HTAB *hashp); [text/x-patch] 02-hash_seq_init_with_hash_value.v5.patch (5.4K, ../../[email protected]/3-02-hash_seq_init_with_hash_value.v5.patch) download | inline diff: diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c index a4152080b5d..dfea7a904e2 100644 --- a/src/backend/utils/hash/dynahash.c +++ b/src/backend/utils/hash/dynahash.c @@ -962,6 +962,33 @@ hash_search(HTAB *hashp, foundPtr); } +/* + * Do initial lookup of a bucket by the given hash value. + */ +static inline HASHBUCKET* +hash_initial_lookup(HTAB *hashp, uint32 hashvalue, uint32 *bucket) +{ + HASHHDR *hctl = hashp->hctl; + HASHSEGMENT segp; + long segment_num; + long segment_ndx; + + /* + * Do the initial lookup + */ + *bucket = calc_bucket(hctl, hashvalue); + + segment_num = *bucket >> hashp->sshift; + segment_ndx = MOD(*bucket, hashp->ssize); + + segp = hashp->dir[segment_num]; + + if (segp == NULL) + hash_corrupted(hashp); + + return &segp[segment_ndx]; +} + void * hash_search_with_hash_value(HTAB *hashp, const void *keyPtr, @@ -973,9 +1000,6 @@ hash_search_with_hash_value(HTAB *hashp, int freelist_idx = FREELIST_IDX(hctl, hashvalue); Size keysize; uint32 bucket; - long segment_num; - long segment_ndx; - HASHSEGMENT segp; HASHBUCKET currBucket; HASHBUCKET *prevBucketPtr; HashCompareFunc match; @@ -1008,17 +1032,7 @@ hash_search_with_hash_value(HTAB *hashp, /* * Do the initial lookup */ - bucket = calc_bucket(hctl, hashvalue); - - segment_num = bucket >> hashp->sshift; - segment_ndx = MOD(bucket, hashp->ssize); - - segp = hashp->dir[segment_num]; - - if (segp == NULL) - hash_corrupted(hashp); - - prevBucketPtr = &segp[segment_ndx]; + prevBucketPtr = hash_initial_lookup(hashp, hashvalue, &bucket); currBucket = *prevBucketPtr; /* @@ -1159,14 +1173,10 @@ hash_update_hash_key(HTAB *hashp, const void *newKeyPtr) { HASHELEMENT *existingElement = ELEMENT_FROM_KEY(existingEntry); - HASHHDR *hctl = hashp->hctl; uint32 newhashvalue; Size keysize; uint32 bucket; uint32 newbucket; - long segment_num; - long segment_ndx; - HASHSEGMENT segp; HASHBUCKET currBucket; HASHBUCKET *prevBucketPtr; HASHBUCKET *oldPrevPtr; @@ -1187,17 +1197,7 @@ hash_update_hash_key(HTAB *hashp, * this to be able to unlink it from its hash chain, but as a side benefit * we can verify the validity of the passed existingEntry pointer. */ - bucket = calc_bucket(hctl, existingElement->hashvalue); - - segment_num = bucket >> hashp->sshift; - segment_ndx = MOD(bucket, hashp->ssize); - - segp = hashp->dir[segment_num]; - - if (segp == NULL) - hash_corrupted(hashp); - - prevBucketPtr = &segp[segment_ndx]; + prevBucketPtr = hash_initial_lookup(hashp, existingElement->hashvalue, &bucket); currBucket = *prevBucketPtr; while (currBucket != NULL) @@ -1219,18 +1219,7 @@ hash_update_hash_key(HTAB *hashp, * chain we want to put the entry into. */ newhashvalue = hashp->hash(newKeyPtr, hashp->keysize); - - newbucket = calc_bucket(hctl, newhashvalue); - - segment_num = newbucket >> hashp->sshift; - segment_ndx = MOD(newbucket, hashp->ssize); - - segp = hashp->dir[segment_num]; - - if (segp == NULL) - hash_corrupted(hashp); - - prevBucketPtr = &segp[segment_ndx]; + prevBucketPtr = hash_initial_lookup(hashp, newhashvalue, &newbucket); currBucket = *prevBucketPtr; /* @@ -1423,10 +1412,27 @@ hash_seq_init(HASH_SEQ_STATUS *status, HTAB *hashp) status->hashp = hashp; status->curBucket = 0; status->curEntry = NULL; + status->hasHashvalue = false; if (!hashp->frozen) register_seq_scan(hashp); } +/* + * Same as above but scan by the given hash value. + * See also hash_seq_search(). + */ +void +hash_seq_init_with_hash_value(HASH_SEQ_STATUS *status, HTAB *hashp, + uint32 hashvalue) +{ + hash_seq_init(status, hashp); + + status->hasHashvalue = true; + status->hashvalue = hashvalue; + + status->curEntry = *hash_initial_lookup(hashp, hashvalue, &status->curBucket); +} + void * hash_seq_search(HASH_SEQ_STATUS *status) { @@ -1440,6 +1446,24 @@ hash_seq_search(HASH_SEQ_STATUS *status) uint32 curBucket; HASHELEMENT *curElem; + if (status->hasHashvalue) + { + /* + * Scan entries only in the current bucket because only this bucket can + * contain entries with the given hash value. + */ + while ((curElem = status->curEntry) != NULL) + { + status->curEntry = curElem->link; + if (status->hashvalue != curElem->hashvalue) + continue; + return (void *) ELEMENTKEY(curElem); + } + + hash_seq_term(status); + return NULL; + } + if ((curElem = status->curEntry) != NULL) { /* Continuing scan of curBucket... */ diff --git a/src/include/utils/hsearch.h b/src/include/utils/hsearch.h index da26941f6db..c99d74625f7 100644 --- a/src/include/utils/hsearch.h +++ b/src/include/utils/hsearch.h @@ -122,6 +122,8 @@ typedef struct HTAB *hashp; uint32 curBucket; /* index of current bucket */ HASHELEMENT *curEntry; /* current entry in bucket */ + bool hasHashvalue; /* true if hashvalue was provided */ + uint32 hashvalue; /* hashvalue to start seqscan over hash */ } HASH_SEQ_STATUS; /* @@ -141,6 +143,8 @@ extern bool hash_update_hash_key(HTAB *hashp, void *existingEntry, const void *newKeyPtr); extern long hash_get_num_entries(HTAB *hashp); extern void hash_seq_init(HASH_SEQ_STATUS *status, HTAB *hashp); +extern void hash_seq_init_with_hash_value(HASH_SEQ_STATUS *status, HTAB *hashp, + uint32 hashvalue); extern void *hash_seq_search(HASH_SEQ_STATUS *status); extern void hash_seq_term(HASH_SEQ_STATUS *status); extern void hash_freeze(HTAB *hashp); [text/x-patch] 01-map_rel_to_type.v5.patch (7.4K, ../../[email protected]/4-01-map_rel_to_type.v5.patch) download | inline diff: diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c index 0d4d0b0a154..7936c3b46d0 100644 --- a/src/backend/utils/cache/typcache.c +++ b/src/backend/utils/cache/typcache.c @@ -77,6 +77,15 @@ /* The main type cache hashtable searched by lookup_type_cache */ static HTAB *TypeCacheHash = NULL; +/* The map from relation's oid to its type oid */ +typedef struct mapRelTypeEntry +{ + Oid typrelid; + Oid type_id; +} mapRelTypeEntry; + +static HTAB *mapRelType = NULL; + /* List of type cache entries for domain types */ static TypeCacheEntry *firstDomainTypeEntry = NULL; @@ -358,6 +367,11 @@ lookup_type_cache(Oid type_id, int flags) TypeCacheHash = hash_create("Type information cache", 64, &ctl, HASH_ELEM | HASH_BLOBS); + ctl.keysize = sizeof(Oid); + ctl.entrysize = sizeof(mapRelTypeEntry); + mapRelType = hash_create("Map reloid to typeoid", 64, + &ctl, HASH_ELEM | HASH_BLOBS); + /* Also set up callbacks for SI invalidations */ CacheRegisterRelcacheCallback(TypeCacheRelCallback, (Datum) 0); CacheRegisterSyscacheCallback(TYPEOID, TypeCacheTypCallback, (Datum) 0); @@ -470,6 +484,24 @@ lookup_type_cache(Oid type_id, int flags) ReleaseSysCache(tp); } + /* + * Add a record to the relation->type map. We don't bother if type will + * become disconnected from the relation. Although it seems to be impossible, + * storing old data is safe in any case. In the worst case scenario we will + * just do an extra cleanup of a cache entry. + */ + if (OidIsValid(typentry->typrelid) && typentry->typtype == TYPTYPE_COMPOSITE) + { + mapRelTypeEntry *relentry; + + relentry = (mapRelTypeEntry*) hash_search(mapRelType, + &typentry->typrelid, + HASH_ENTER, NULL); + + relentry->typrelid = typentry->typrelid; + relentry->type_id = typentry->type_id; + } + /* * Look up opclasses if we haven't already and any dependent info is * requested. @@ -2264,6 +2296,33 @@ SharedRecordTypmodRegistryAttach(SharedRecordTypmodRegistry *registry) CurrentSession->shared_typmod_table = typmod_table; } +static void +invalidateCompositeTypeCacheEntry(TypeCacheEntry *typentry) +{ + /* Delete tupdesc if we have it */ + if (typentry->tupDesc != NULL) + { + /* + * Release our refcount and free the tupdesc if none remain. We can't + * use DecrTupleDescRefCount here because this reference is not logged + * by the current resource owner. + */ + Assert(typentry->tupDesc->tdrefcount > 0); + if (--typentry->tupDesc->tdrefcount == 0) + FreeTupleDesc(typentry->tupDesc); + typentry->tupDesc = NULL; + + /* + * Also clear tupDesc_identifier, so that anyone watching + * it will realize that the tupdesc has changed. + */ + typentry->tupDesc_identifier = 0; + } + + /* Reset equality/comparison/hashing validity information */ + typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; +} + /* * TypeCacheRelCallback * Relcache inval callback function @@ -2273,63 +2332,46 @@ SharedRecordTypmodRegistryAttach(SharedRecordTypmodRegistry *registry) * whatever info we have cached about the composite type's comparability. * * This is called when a relcache invalidation event occurs for the given - * relid. We must scan the whole typcache hash since we don't know the - * type OID corresponding to the relid. We could do a direct search if this - * were a syscache-flush callback on pg_type, but then we would need all - * ALTER-TABLE-like commands that could modify a rowtype to issue syscache - * invals against the rel's pg_type OID. The extra SI signaling could very - * well cost more than we'd save, since in most usages there are not very - * many entries in a backend's typcache. The risk of bugs-of-omission seems - * high, too. - * - * Another possibility, with only localized impact, is to maintain a second - * hashtable that indexes composite-type typcache entries by their typrelid. - * But it's still not clear it's worth the trouble. + * relid. We can't use syscache to find a type corresponding to the given + * relation because the code can be called outside of transaction. Thus we use + * a dedicated relid->type map, mapRelType. */ static void TypeCacheRelCallback(Datum arg, Oid relid) { - HASH_SEQ_STATUS status; TypeCacheEntry *typentry; - /* TypeCacheHash must exist, else this callback wouldn't be registered */ - hash_seq_init(&status, TypeCacheHash); - while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) + /* + * mapRelType and TypeCacheHash should exist, otherwise this callback + * wouldn't be registered + */ + + if (OidIsValid(relid)) { - if (typentry->typtype == TYPTYPE_COMPOSITE) + mapRelTypeEntry *relentry; + + relentry = (mapRelTypeEntry *) hash_search(mapRelType, + &relid, + HASH_FIND, NULL); + + if (relentry != NULL) { - /* Skip if no match, unless we're zapping all composite types */ - if (relid != typentry->typrelid && relid != InvalidOid) - continue; + typentry = (TypeCacheEntry *) hash_search(TypeCacheHash, + &relentry->type_id, + HASH_FIND, NULL); - /* Delete tupdesc if we have it */ - if (typentry->tupDesc != NULL) + if (typentry != NULL) { - /* - * Release our refcount, and free the tupdesc if none remain. - * (Can't use DecrTupleDescRefCount because this reference is - * not logged in current resource owner.) - */ - Assert(typentry->tupDesc->tdrefcount > 0); - if (--typentry->tupDesc->tdrefcount == 0) - FreeTupleDesc(typentry->tupDesc); - typentry->tupDesc = NULL; + Assert(typentry->typtype == TYPTYPE_COMPOSITE); + Assert(relid == typentry->typrelid); - /* - * Also clear tupDesc_identifier, so that anything watching - * that will realize that the tupdesc has possibly changed. - * (Alternatively, we could specify that to detect possible - * tupdesc change, one must check for tupDesc != NULL as well - * as tupDesc_identifier being the same as what was previously - * seen. That seems error-prone.) - */ - typentry->tupDesc_identifier = 0; + invalidateCompositeTypeCacheEntry(typentry); } - - /* Reset equality/comparison/hashing validity information */ - typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; } - else if (typentry->typtype == TYPTYPE_DOMAIN) + + for (typentry = firstDomainTypeEntry; + typentry != NULL; + typentry = typentry->nextDomain) { /* * If it's domain over composite, reset flags. (We don't bother @@ -2341,6 +2383,35 @@ TypeCacheRelCallback(Datum arg, Oid relid) typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; } } + else + { + HASH_SEQ_STATUS status; + + /* + * Relid = 0, so we need to reset all composite types in cache. Also, we + * should reset flags for domain types, and we loop over all entries + * in hash, so, do it in a single scan. + */ + hash_seq_init(&status, TypeCacheHash); + while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) + { + if (typentry->typtype == TYPTYPE_COMPOSITE) + { + invalidateCompositeTypeCacheEntry(typentry); + } + else if (typentry->typtype == TYPTYPE_DOMAIN) + { + /* + * If it's domain over composite, reset flags. (We don't bother + * trying to determine whether the specific base type needs a + * reset.) Note that if we haven't determined whether the base + * type is composite, we don't need to reset anything. + */ + if (typentry->flags & TCFLAGS_DOMAIN_BASE_IS_COMPOSITE) + typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; + } + } + } } /* ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-03-14 07:42 Michael Paquier <[email protected]> parent: Teodor Sigaev <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Michael Paquier @ 2024-03-14 07:42 UTC (permalink / raw) To: Teodor Sigaev <[email protected]>; +Cc: Aleksander Alekseev <[email protected]>; pgsql-hackers; Tom Lane <[email protected]> On Wed, Mar 13, 2024 at 04:40:38PM +0300, Teodor Sigaev wrote: > Done, all patches should be applied consequentially. One thing that first pops out to me is that we can do the refactor of hash_initial_lookup() as an independent piece, without the extra paths introduced. But rather than returning the bucket hash and have the bucket number as an in/out argument of hash_initial_lookup(), there is an argument for reversing them: hash_search_with_hash_value() does not care about the bucket number. > 02-hash_seq_init_with_hash_value.v5.patch - introduces a > hash_seq_init_with_hash_value() method. hash_initial_lookup() is marked as > inline, but I suppose, modern compilers are smart enough to inline it > automatically. Likely so, though that does not hurt to show the intention to the reader. So I would like to suggest the attached patch for this first piece. What do you think? It may also be an idea to use `git format-patch` when generating a series of patches. That makes for easier reviews. -- Michael Attachments: [text/x-diff] 0001-Refactor-initial-hash-lookup-in-dynahash.c.patch (4.0K, ../../[email protected]/2-0001-Refactor-initial-hash-lookup-in-dynahash.c.patch) download | inline diff: From 6b1fe126b9f72ff27aca08128948f4e617ba70dd Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Thu, 14 Mar 2024 16:35:40 +0900 Subject: [PATCH] Refactor initial hash lookup in dynahash.c --- src/backend/utils/hash/dynahash.c | 75 ++++++++++++++----------------- 1 file changed, 33 insertions(+), 42 deletions(-) diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c index a4152080b5..e1bd92a01c 100644 --- a/src/backend/utils/hash/dynahash.c +++ b/src/backend/utils/hash/dynahash.c @@ -273,6 +273,8 @@ static void hdefault(HTAB *hashp); static int choose_nelem_alloc(Size entrysize); static bool init_htab(HTAB *hashp, long nelem); static void hash_corrupted(HTAB *hashp); +static uint32 hash_initial_lookup(HTAB *hashp, uint32 hashvalue, + HASHBUCKET **bucketptr); static long next_pow2_long(long num); static int next_pow2_int(long num); static void register_seq_scan(HTAB *hashp); @@ -972,10 +974,6 @@ hash_search_with_hash_value(HTAB *hashp, HASHHDR *hctl = hashp->hctl; int freelist_idx = FREELIST_IDX(hctl, hashvalue); Size keysize; - uint32 bucket; - long segment_num; - long segment_ndx; - HASHSEGMENT segp; HASHBUCKET currBucket; HASHBUCKET *prevBucketPtr; HashCompareFunc match; @@ -1008,17 +1006,7 @@ hash_search_with_hash_value(HTAB *hashp, /* * Do the initial lookup */ - bucket = calc_bucket(hctl, hashvalue); - - segment_num = bucket >> hashp->sshift; - segment_ndx = MOD(bucket, hashp->ssize); - - segp = hashp->dir[segment_num]; - - if (segp == NULL) - hash_corrupted(hashp); - - prevBucketPtr = &segp[segment_ndx]; + (void) hash_initial_lookup(hashp, hashvalue, &prevBucketPtr); currBucket = *prevBucketPtr; /* @@ -1159,14 +1147,10 @@ hash_update_hash_key(HTAB *hashp, const void *newKeyPtr) { HASHELEMENT *existingElement = ELEMENT_FROM_KEY(existingEntry); - HASHHDR *hctl = hashp->hctl; uint32 newhashvalue; Size keysize; uint32 bucket; uint32 newbucket; - long segment_num; - long segment_ndx; - HASHSEGMENT segp; HASHBUCKET currBucket; HASHBUCKET *prevBucketPtr; HASHBUCKET *oldPrevPtr; @@ -1187,17 +1171,8 @@ hash_update_hash_key(HTAB *hashp, * this to be able to unlink it from its hash chain, but as a side benefit * we can verify the validity of the passed existingEntry pointer. */ - bucket = calc_bucket(hctl, existingElement->hashvalue); - - segment_num = bucket >> hashp->sshift; - segment_ndx = MOD(bucket, hashp->ssize); - - segp = hashp->dir[segment_num]; - - if (segp == NULL) - hash_corrupted(hashp); - - prevBucketPtr = &segp[segment_ndx]; + bucket = hash_initial_lookup(hashp, existingElement->hashvalue, + &prevBucketPtr); currBucket = *prevBucketPtr; while (currBucket != NULL) @@ -1219,18 +1194,7 @@ hash_update_hash_key(HTAB *hashp, * chain we want to put the entry into. */ newhashvalue = hashp->hash(newKeyPtr, hashp->keysize); - - newbucket = calc_bucket(hctl, newhashvalue); - - segment_num = newbucket >> hashp->sshift; - segment_ndx = MOD(newbucket, hashp->ssize); - - segp = hashp->dir[segment_num]; - - if (segp == NULL) - hash_corrupted(hashp); - - prevBucketPtr = &segp[segment_ndx]; + newbucket = hash_initial_lookup(hashp, newhashvalue, &prevBucketPtr); currBucket = *prevBucketPtr; /* @@ -1741,6 +1705,33 @@ element_alloc(HTAB *hashp, int nelem, int freelist_idx) return true; } +/* + * Do initial lookup of a bucket for the given hash value, retrieving its + * bucket number and its hash bucket. + */ +static inline uint32 +hash_initial_lookup(HTAB *hashp, uint32 hashvalue, HASHBUCKET **bucketptr) +{ + HASHHDR *hctl = hashp->hctl; + HASHSEGMENT segp; + long segment_num; + long segment_ndx; + uint32 bucket; + + bucket = calc_bucket(hctl, hashvalue); + + segment_num = bucket >> hashp->sshift; + segment_ndx = MOD(bucket, hashp->ssize); + + segp = hashp->dir[segment_num]; + + if (segp == NULL) + hash_corrupted(hashp); + + *bucketptr = &segp[segment_ndx]; + return bucket; +} + /* complain when we have detected a corrupted hashtable */ static void hash_corrupted(HTAB *hashp) -- 2.43.0 [application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc) download ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-03-14 13:27 Teodor Sigaev <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Teodor Sigaev @ 2024-03-14 13:27 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Aleksander Alekseev <[email protected]>; pgsql-hackers; Tom Lane <[email protected]> > One thing that first pops out to me is that we can do the refactor of > hash_initial_lookup() as an independent piece, without the extra paths > introduced. But rather than returning the bucket hash and have the > bucket number as an in/out argument of hash_initial_lookup(), there is > an argument for reversing them: hash_search_with_hash_value() does not > care about the bucket number. Ok, no problem > >> 02-hash_seq_init_with_hash_value.v5.patch - introduces a >> hash_seq_init_with_hash_value() method. hash_initial_lookup() is marked as >> inline, but I suppose, modern compilers are smart enough to inline it >> automatically. > > Likely so, though that does not hurt to show the intention to the > reader. Agree > > So I would like to suggest the attached patch for this first piece. > What do you think? I have not any objections > > It may also be an idea to use `git format-patch` when generating a > series of patches. That makes for easier reviews. Thanks, will try -- Teodor Sigaev E-mail: [email protected] WWW: http://www.sigaev.ru/ ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-03-14 22:58 Michael Paquier <[email protected]> parent: Teodor Sigaev <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Michael Paquier @ 2024-03-14 22:58 UTC (permalink / raw) To: Teodor Sigaev <[email protected]>; +Cc: Aleksander Alekseev <[email protected]>; pgsql-hackers; Tom Lane <[email protected]> On Thu, Mar 14, 2024 at 04:27:43PM +0300, Teodor Sigaev wrote: >> So I would like to suggest the attached patch for this first piece. >> What do you think? > > I have not any objections Okay, I've applied this piece for now. Not sure I'll have much room to look at the rest. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-03-15 10:57 Teodor Sigaev <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 2 replies; 28+ messages in thread From: Teodor Sigaev @ 2024-03-15 10:57 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Aleksander Alekseev <[email protected]>; pgsql-hackers; Tom Lane <[email protected]> > Okay, I've applied this piece for now. Not sure I'll have much room > to look at the rest. Thank you very much! Rest of patches, rebased. -- Teodor Sigaev E-mail: [email protected] WWW: http://www.sigaev.ru/ Attachments: [text/x-patch] 0003-usage-of-hash_search_with_hash_value.patch (6.0K, ../../[email protected]/2-0003-usage-of-hash_search_with_hash_value.patch) download | inline diff: From b30af080d7768c2fdb6198e2e40ef93419f60732 Mon Sep 17 00:00:00 2001 From: Teodor Sigaev <[email protected]> Date: Fri, 15 Mar 2024 13:55:10 +0300 Subject: [PATCH 3/3] usage of hash_search_with_hash_value --- src/backend/utils/cache/attoptcache.c | 39 ++++++++++++++++---- src/backend/utils/cache/typcache.c | 52 +++++++++++++++++++-------- 2 files changed, 70 insertions(+), 21 deletions(-) diff --git a/src/backend/utils/cache/attoptcache.c b/src/backend/utils/cache/attoptcache.c index af978ccd4b1..3a18b2e9a77 100644 --- a/src/backend/utils/cache/attoptcache.c +++ b/src/backend/utils/cache/attoptcache.c @@ -44,12 +44,10 @@ typedef struct /* * InvalidateAttoptCacheCallback - * Flush all cache entries when pg_attribute is updated. + * Flush cache entry (or entries) when pg_attribute is updated. * * When pg_attribute is updated, we must flush the cache entry at least - * for that attribute. Currently, we just flush them all. Since attribute - * options are not currently used in performance-critical paths (such as - * query execution), this seems OK. + * for that attribute. */ static void InvalidateAttoptCacheCallback(Datum arg, int cacheid, uint32 hashvalue) @@ -57,7 +55,16 @@ InvalidateAttoptCacheCallback(Datum arg, int cacheid, uint32 hashvalue) HASH_SEQ_STATUS status; AttoptCacheEntry *attopt; - hash_seq_init(&status, AttoptCacheHash); + /* + * By convection, zero hash value is passed to the callback as a sign + * that it's time to invalidate the cache. See sinval.c, inval.c and + * InvalidateSystemCachesExtended(). + */ + if (hashvalue == 0) + hash_seq_init(&status, AttoptCacheHash); + else + hash_seq_init_with_hash_value(&status, AttoptCacheHash, hashvalue); + while ((attopt = (AttoptCacheEntry *) hash_seq_search(&status)) != NULL) { if (attopt->opts) @@ -70,6 +77,18 @@ InvalidateAttoptCacheCallback(Datum arg, int cacheid, uint32 hashvalue) } } +/* + * Hash function compatible with two-arg system cache hash function. + */ +static uint32 +relatt_cache_syshash(const void *key, Size keysize) +{ + const AttoptCacheKey* ckey = key; + + Assert(keysize == sizeof(*ckey)); + return GetSysCacheHashValue2(ATTNUM, ckey->attrelid, ckey->attnum); +} + /* * InitializeAttoptCache * Initialize the attribute options cache. @@ -82,9 +101,17 @@ InitializeAttoptCache(void) /* Initialize the hash table. */ ctl.keysize = sizeof(AttoptCacheKey); ctl.entrysize = sizeof(AttoptCacheEntry); + + /* + * AttoptCacheEntry takes hash value from the system cache. For + * AttoptCacheHash we use the same hash in order to speedup search by hash + * value. This is used by hash_seq_init_with_hash_value(). + */ + ctl.hash = relatt_cache_syshash; + AttoptCacheHash = hash_create("Attopt cache", 256, &ctl, - HASH_ELEM | HASH_BLOBS); + HASH_ELEM | HASH_FUNCTION); /* Make sure we've initialized CacheMemoryContext. */ if (!CacheMemoryContext) diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c index 7936c3b46d0..9145088f44d 100644 --- a/src/backend/utils/cache/typcache.c +++ b/src/backend/utils/cache/typcache.c @@ -339,6 +339,15 @@ static TupleDesc find_or_make_matching_shared_tupledesc(TupleDesc tupdesc); static dsa_pointer share_tupledesc(dsa_area *area, TupleDesc tupdesc, uint32 typmod); +/* + * Hash function compatible with one-arg system cache hash function. + */ +static uint32 +type_cache_syshash(const void *key, Size keysize) +{ + Assert(keysize == sizeof(Oid)); + return GetSysCacheHashValue1(TYPEOID, ObjectIdGetDatum(*(const Oid*)key)); +} /* * lookup_type_cache @@ -364,8 +373,15 @@ lookup_type_cache(Oid type_id, int flags) ctl.keysize = sizeof(Oid); ctl.entrysize = sizeof(TypeCacheEntry); + /* + * TypeEntry takes hash value from the system cache. For TypeCacheHash + * we use the same hash in order to speedup search by hash value. This + * is used by hash_seq_init_with_hash_value(). + */ + ctl.hash = type_cache_syshash; + TypeCacheHash = hash_create("Type information cache", 64, - &ctl, HASH_ELEM | HASH_BLOBS); + &ctl, HASH_ELEM | HASH_FUNCTION); ctl.keysize = sizeof(Oid); ctl.entrysize = sizeof(mapRelTypeEntry); @@ -421,8 +437,7 @@ lookup_type_cache(Oid type_id, int flags) /* These fields can never change, by definition */ typentry->type_id = type_id; - typentry->type_id_hash = GetSysCacheHashValue1(TYPEOID, - ObjectIdGetDatum(type_id)); + typentry->type_id_hash = get_hash_value(TypeCacheHash, &type_id); /* Keep this part in sync with the code below */ typentry->typlen = typtup->typlen; @@ -2429,20 +2444,27 @@ TypeCacheTypCallback(Datum arg, int cacheid, uint32 hashvalue) TypeCacheEntry *typentry; /* TypeCacheHash must exist, else this callback wouldn't be registered */ - hash_seq_init(&status, TypeCacheHash); + + /* + * By convection, zero hash value is passed to the callback as a sign + * that it's time to invalidate the cache. See sinval.c, inval.c and + * InvalidateSystemCachesExtended(). + */ + if (hashvalue == 0) + hash_seq_init(&status, TypeCacheHash); + else + hash_seq_init_with_hash_value(&status, TypeCacheHash, hashvalue); + while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) { - /* Is this the targeted type row (or it's a total cache flush)? */ - if (hashvalue == 0 || typentry->type_id_hash == hashvalue) - { - /* - * Mark the data obtained directly from pg_type as invalid. Also, - * if it's a domain, typnotnull might've changed, so we'll need to - * recalculate its constraints. - */ - typentry->flags &= ~(TCFLAGS_HAVE_PG_TYPE_DATA | - TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS); - } + Assert(hashvalue == 0 || typentry->type_id_hash == hashvalue); + /* + * Mark the data obtained directly from pg_type as invalid. Also, + * if it's a domain, typnotnull might've changed, so we'll need to + * recalculate its constraints. + */ + typentry->flags &= ~(TCFLAGS_HAVE_PG_TYPE_DATA | + TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS); } } -- 2.43.2 [text/x-patch] 0002-hash_search_with_hash_value.patch (2.8K, ../../[email protected]/3-0002-hash_search_with_hash_value.patch) download | inline diff: From 917d70a81ad37d366d73a4e3a9ed92212b4698ad Mon Sep 17 00:00:00 2001 From: Teodor Sigaev <[email protected]> Date: Fri, 15 Mar 2024 13:52:50 +0300 Subject: [PATCH 2/3] hash_search_with_hash_value --- src/backend/utils/hash/dynahash.c | 38 +++++++++++++++++++++++++++++++ src/include/utils/hsearch.h | 4 ++++ 2 files changed, 42 insertions(+) diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c index 4080833df0f..e981298ea47 100644 --- a/src/backend/utils/hash/dynahash.c +++ b/src/backend/utils/hash/dynahash.c @@ -1387,10 +1387,30 @@ hash_seq_init(HASH_SEQ_STATUS *status, HTAB *hashp) status->hashp = hashp; status->curBucket = 0; status->curEntry = NULL; + status->hasHashvalue = false; if (!hashp->frozen) register_seq_scan(hashp); } +/* + * Same as above but scan by the given hash value. + * See also hash_seq_search(). + */ +void +hash_seq_init_with_hash_value(HASH_SEQ_STATUS *status, HTAB *hashp, + uint32 hashvalue) +{ + HASHBUCKET *bucketPtr; + + hash_seq_init(status, hashp); + + status->hasHashvalue = true; + status->hashvalue = hashvalue; + + status->curBucket = hash_initial_lookup(hashp, hashvalue, &bucketPtr); + status->curEntry = *bucketPtr; +} + void * hash_seq_search(HASH_SEQ_STATUS *status) { @@ -1404,6 +1424,24 @@ hash_seq_search(HASH_SEQ_STATUS *status) uint32 curBucket; HASHELEMENT *curElem; + if (status->hasHashvalue) + { + /* + * Scan entries only in the current bucket because only this bucket can + * contain entries with the given hash value. + */ + while ((curElem = status->curEntry) != NULL) + { + status->curEntry = curElem->link; + if (status->hashvalue != curElem->hashvalue) + continue; + return (void *) ELEMENTKEY(curElem); + } + + hash_seq_term(status); + return NULL; + } + if ((curElem = status->curEntry) != NULL) { /* Continuing scan of curBucket... */ diff --git a/src/include/utils/hsearch.h b/src/include/utils/hsearch.h index da26941f6db..c99d74625f7 100644 --- a/src/include/utils/hsearch.h +++ b/src/include/utils/hsearch.h @@ -122,6 +122,8 @@ typedef struct HTAB *hashp; uint32 curBucket; /* index of current bucket */ HASHELEMENT *curEntry; /* current entry in bucket */ + bool hasHashvalue; /* true if hashvalue was provided */ + uint32 hashvalue; /* hashvalue to start seqscan over hash */ } HASH_SEQ_STATUS; /* @@ -141,6 +143,8 @@ extern bool hash_update_hash_key(HTAB *hashp, void *existingEntry, const void *newKeyPtr); extern long hash_get_num_entries(HTAB *hashp); extern void hash_seq_init(HASH_SEQ_STATUS *status, HTAB *hashp); +extern void hash_seq_init_with_hash_value(HASH_SEQ_STATUS *status, HTAB *hashp, + uint32 hashvalue); extern void *hash_seq_search(HASH_SEQ_STATUS *status); extern void hash_seq_term(HASH_SEQ_STATUS *status); extern void hash_freeze(HTAB *hashp); -- 2.43.2 [text/x-patch] 0001-type-cache.patch (7.7K, ../../[email protected]/4-0001-type-cache.patch) download | inline diff: From 93d3ff32c7c09ec96e7649f831d508c2921b5b5b Mon Sep 17 00:00:00 2001 From: Teodor Sigaev <[email protected]> Date: Fri, 15 Mar 2024 13:52:34 +0300 Subject: [PATCH 1/3] type cache --- src/backend/utils/cache/typcache.c | 159 +++++++++++++++++++++-------- 1 file changed, 115 insertions(+), 44 deletions(-) diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c index 0d4d0b0a154..7936c3b46d0 100644 --- a/src/backend/utils/cache/typcache.c +++ b/src/backend/utils/cache/typcache.c @@ -77,6 +77,15 @@ /* The main type cache hashtable searched by lookup_type_cache */ static HTAB *TypeCacheHash = NULL; +/* The map from relation's oid to its type oid */ +typedef struct mapRelTypeEntry +{ + Oid typrelid; + Oid type_id; +} mapRelTypeEntry; + +static HTAB *mapRelType = NULL; + /* List of type cache entries for domain types */ static TypeCacheEntry *firstDomainTypeEntry = NULL; @@ -358,6 +367,11 @@ lookup_type_cache(Oid type_id, int flags) TypeCacheHash = hash_create("Type information cache", 64, &ctl, HASH_ELEM | HASH_BLOBS); + ctl.keysize = sizeof(Oid); + ctl.entrysize = sizeof(mapRelTypeEntry); + mapRelType = hash_create("Map reloid to typeoid", 64, + &ctl, HASH_ELEM | HASH_BLOBS); + /* Also set up callbacks for SI invalidations */ CacheRegisterRelcacheCallback(TypeCacheRelCallback, (Datum) 0); CacheRegisterSyscacheCallback(TYPEOID, TypeCacheTypCallback, (Datum) 0); @@ -470,6 +484,24 @@ lookup_type_cache(Oid type_id, int flags) ReleaseSysCache(tp); } + /* + * Add a record to the relation->type map. We don't bother if type will + * become disconnected from the relation. Although it seems to be impossible, + * storing old data is safe in any case. In the worst case scenario we will + * just do an extra cleanup of a cache entry. + */ + if (OidIsValid(typentry->typrelid) && typentry->typtype == TYPTYPE_COMPOSITE) + { + mapRelTypeEntry *relentry; + + relentry = (mapRelTypeEntry*) hash_search(mapRelType, + &typentry->typrelid, + HASH_ENTER, NULL); + + relentry->typrelid = typentry->typrelid; + relentry->type_id = typentry->type_id; + } + /* * Look up opclasses if we haven't already and any dependent info is * requested. @@ -2264,6 +2296,33 @@ SharedRecordTypmodRegistryAttach(SharedRecordTypmodRegistry *registry) CurrentSession->shared_typmod_table = typmod_table; } +static void +invalidateCompositeTypeCacheEntry(TypeCacheEntry *typentry) +{ + /* Delete tupdesc if we have it */ + if (typentry->tupDesc != NULL) + { + /* + * Release our refcount and free the tupdesc if none remain. We can't + * use DecrTupleDescRefCount here because this reference is not logged + * by the current resource owner. + */ + Assert(typentry->tupDesc->tdrefcount > 0); + if (--typentry->tupDesc->tdrefcount == 0) + FreeTupleDesc(typentry->tupDesc); + typentry->tupDesc = NULL; + + /* + * Also clear tupDesc_identifier, so that anyone watching + * it will realize that the tupdesc has changed. + */ + typentry->tupDesc_identifier = 0; + } + + /* Reset equality/comparison/hashing validity information */ + typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; +} + /* * TypeCacheRelCallback * Relcache inval callback function @@ -2273,63 +2332,46 @@ SharedRecordTypmodRegistryAttach(SharedRecordTypmodRegistry *registry) * whatever info we have cached about the composite type's comparability. * * This is called when a relcache invalidation event occurs for the given - * relid. We must scan the whole typcache hash since we don't know the - * type OID corresponding to the relid. We could do a direct search if this - * were a syscache-flush callback on pg_type, but then we would need all - * ALTER-TABLE-like commands that could modify a rowtype to issue syscache - * invals against the rel's pg_type OID. The extra SI signaling could very - * well cost more than we'd save, since in most usages there are not very - * many entries in a backend's typcache. The risk of bugs-of-omission seems - * high, too. - * - * Another possibility, with only localized impact, is to maintain a second - * hashtable that indexes composite-type typcache entries by their typrelid. - * But it's still not clear it's worth the trouble. + * relid. We can't use syscache to find a type corresponding to the given + * relation because the code can be called outside of transaction. Thus we use + * a dedicated relid->type map, mapRelType. */ static void TypeCacheRelCallback(Datum arg, Oid relid) { - HASH_SEQ_STATUS status; TypeCacheEntry *typentry; - /* TypeCacheHash must exist, else this callback wouldn't be registered */ - hash_seq_init(&status, TypeCacheHash); - while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) + /* + * mapRelType and TypeCacheHash should exist, otherwise this callback + * wouldn't be registered + */ + + if (OidIsValid(relid)) { - if (typentry->typtype == TYPTYPE_COMPOSITE) + mapRelTypeEntry *relentry; + + relentry = (mapRelTypeEntry *) hash_search(mapRelType, + &relid, + HASH_FIND, NULL); + + if (relentry != NULL) { - /* Skip if no match, unless we're zapping all composite types */ - if (relid != typentry->typrelid && relid != InvalidOid) - continue; + typentry = (TypeCacheEntry *) hash_search(TypeCacheHash, + &relentry->type_id, + HASH_FIND, NULL); - /* Delete tupdesc if we have it */ - if (typentry->tupDesc != NULL) + if (typentry != NULL) { - /* - * Release our refcount, and free the tupdesc if none remain. - * (Can't use DecrTupleDescRefCount because this reference is - * not logged in current resource owner.) - */ - Assert(typentry->tupDesc->tdrefcount > 0); - if (--typentry->tupDesc->tdrefcount == 0) - FreeTupleDesc(typentry->tupDesc); - typentry->tupDesc = NULL; + Assert(typentry->typtype == TYPTYPE_COMPOSITE); + Assert(relid == typentry->typrelid); - /* - * Also clear tupDesc_identifier, so that anything watching - * that will realize that the tupdesc has possibly changed. - * (Alternatively, we could specify that to detect possible - * tupdesc change, one must check for tupDesc != NULL as well - * as tupDesc_identifier being the same as what was previously - * seen. That seems error-prone.) - */ - typentry->tupDesc_identifier = 0; + invalidateCompositeTypeCacheEntry(typentry); } - - /* Reset equality/comparison/hashing validity information */ - typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; } - else if (typentry->typtype == TYPTYPE_DOMAIN) + + for (typentry = firstDomainTypeEntry; + typentry != NULL; + typentry = typentry->nextDomain) { /* * If it's domain over composite, reset flags. (We don't bother @@ -2341,6 +2383,35 @@ TypeCacheRelCallback(Datum arg, Oid relid) typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; } } + else + { + HASH_SEQ_STATUS status; + + /* + * Relid = 0, so we need to reset all composite types in cache. Also, we + * should reset flags for domain types, and we loop over all entries + * in hash, so, do it in a single scan. + */ + hash_seq_init(&status, TypeCacheHash); + while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) + { + if (typentry->typtype == TYPTYPE_COMPOSITE) + { + invalidateCompositeTypeCacheEntry(typentry); + } + else if (typentry->typtype == TYPTYPE_DOMAIN) + { + /* + * If it's domain over composite, reset flags. (We don't bother + * trying to determine whether the specific base type needs a + * reset.) Note that if we haven't determined whether the base + * type is composite, we don't need to reset anything. + */ + if (typentry->flags & TCFLAGS_DOMAIN_BASE_IS_COMPOSITE) + typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; + } + } + } } /* -- 2.43.2 ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-03-29 04:49 Жарков Роман <[email protected]> parent: Teodor Sigaev <[email protected]> 1 sibling, 0 replies; 28+ messages in thread From: Жарков Роман @ 2024-03-29 04:49 UTC (permalink / raw) To: Teodor Sigaev <[email protected]>; +Cc: Michael Paquier <[email protected]>; Aleksander Alekseev <[email protected]>; pgsql-hackers; Tom Lane <[email protected]> > Rest of patches, rebased. Hello, I read and tested only the first patch so far. Creation of temp tables and rollback in your script work 3-4 times faster with 0001-type-cache.patch on my windows laptop. In the patch I found a copy of the comment "If it's domain over composite, reset flags...". Can we move the reset flags operation and its comment into the invalidateCompositeTypeCacheEntry() function? This simplify the TypeCacheRelCallback() func, but adds two more IF statements when we need to clean up a cache entry for a specific relation. (diff attached). -- Roman Zharkov Attachments: [application/octet-stream] mapRelType-v2.patch (7.7K, ../../1d01a3-66064880-1-7cb74a00@198342865/3-mapRelType-v2.patch) download | inline diff: diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c index aa4720cb598..0b97eea9136 100644 --- a/src/backend/utils/cache/typcache.c +++ b/src/backend/utils/cache/typcache.c @@ -77,6 +77,15 @@ /* The main type cache hashtable searched by lookup_type_cache */ static HTAB *TypeCacheHash = NULL; +/* The map from relation's oid to its type oid */ +typedef struct mapRelTypeEntry +{ + Oid typrelid; + Oid type_id; +} mapRelTypeEntry; + +static HTAB *mapRelType = NULL; + /* List of type cache entries for domain types */ static TypeCacheEntry *firstDomainTypeEntry = NULL; @@ -358,6 +367,11 @@ lookup_type_cache(Oid type_id, int flags) TypeCacheHash = hash_create("Type information cache", 64, &ctl, HASH_ELEM | HASH_BLOBS); + ctl.keysize = sizeof(Oid); + ctl.entrysize = sizeof(mapRelTypeEntry); + mapRelType = hash_create("Map reloid to typeoid", 64, + &ctl, HASH_ELEM | HASH_BLOBS); + /* Also set up callbacks for SI invalidations */ CacheRegisterRelcacheCallback(TypeCacheRelCallback, (Datum) 0); CacheRegisterSyscacheCallback(TYPEOID, TypeCacheTypCallback, (Datum) 0); @@ -470,6 +484,24 @@ lookup_type_cache(Oid type_id, int flags) ReleaseSysCache(tp); } + /* + * Add a record to the relation->type map. We don't bother if type will + * become disconnected from the relation. Although it seems to be impossible, + * storing old data is safe in any case. In the worst case scenario we will + * just do an extra cleanup of a cache entry. + */ + if (OidIsValid(typentry->typrelid) && typentry->typtype == TYPTYPE_COMPOSITE) + { + mapRelTypeEntry *relentry; + + relentry = (mapRelTypeEntry*) hash_search(mapRelType, + &typentry->typrelid, + HASH_ENTER, NULL); + + relentry->typrelid = typentry->typrelid; + relentry->type_id = typentry->type_id; + } + /* * Look up opclasses if we haven't already and any dependent info is * requested. @@ -2264,6 +2296,47 @@ SharedRecordTypmodRegistryAttach(SharedRecordTypmodRegistry *registry) CurrentSession->shared_typmod_table = typmod_table; } +static void +invalidateCompositeTypeCacheEntry(TypeCacheEntry *typentry) +{ + if (typentry->typtype == TYPTYPE_COMPOSITE) + { + /* Delete tupdesc if we have it */ + if (typentry->tupDesc != NULL) + { + /* + * Release our refcount and free the tupdesc if none remain. We can't + * use DecrTupleDescRefCount here because this reference is not logged + * by the current resource owner. + */ + Assert(typentry->tupDesc->tdrefcount > 0); + if (--typentry->tupDesc->tdrefcount == 0) + FreeTupleDesc(typentry->tupDesc); + typentry->tupDesc = NULL; + + /* + * Also clear tupDesc_identifier, so that anyone watching + * it will realize that the tupdesc has changed. + */ + typentry->tupDesc_identifier = 0; + } + + /* Reset equality/comparison/hashing validity information */ + typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; + } + else if (typentry->typtype == TYPTYPE_DOMAIN) + { + /* + * If it's domain over composite, reset flags. (We don't bother + * trying to determine whether the specific base type needs a + * reset.) Note that if we haven't determined whether the base + * type is composite, we don't need to reset anything. + */ + if (typentry->flags & TCFLAGS_DOMAIN_BASE_IS_COMPOSITE) + typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; + } +} + /* * TypeCacheRelCallback * Relcache inval callback function @@ -2273,72 +2346,63 @@ SharedRecordTypmodRegistryAttach(SharedRecordTypmodRegistry *registry) * whatever info we have cached about the composite type's comparability. * * This is called when a relcache invalidation event occurs for the given - * relid. We must scan the whole typcache hash since we don't know the - * type OID corresponding to the relid. We could do a direct search if this - * were a syscache-flush callback on pg_type, but then we would need all - * ALTER-TABLE-like commands that could modify a rowtype to issue syscache - * invals against the rel's pg_type OID. The extra SI signaling could very - * well cost more than we'd save, since in most usages there are not very - * many entries in a backend's typcache. The risk of bugs-of-omission seems - * high, too. - * - * Another possibility, with only localized impact, is to maintain a second - * hashtable that indexes composite-type typcache entries by their typrelid. - * But it's still not clear it's worth the trouble. + * relid. We can't use syscache to find a type corresponding to the given + * relation because the code can be called outside of transaction. Thus we use + * a dedicated relid->type map, mapRelType. */ static void TypeCacheRelCallback(Datum arg, Oid relid) { - HASH_SEQ_STATUS status; TypeCacheEntry *typentry; - /* TypeCacheHash must exist, else this callback wouldn't be registered */ - hash_seq_init(&status, TypeCacheHash); - while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) + /* + * mapRelType and TypeCacheHash should exist, otherwise this callback + * wouldn't be registered + */ + + if (OidIsValid(relid)) { - if (typentry->typtype == TYPTYPE_COMPOSITE) + mapRelTypeEntry *relentry; + + relentry = (mapRelTypeEntry *) hash_search(mapRelType, + &relid, + HASH_FIND, NULL); + + if (relentry != NULL) { - /* Skip if no match, unless we're zapping all composite types */ - if (relid != typentry->typrelid && relid != InvalidOid) - continue; + typentry = (TypeCacheEntry *) hash_search(TypeCacheHash, + &relentry->type_id, + HASH_FIND, NULL); - /* Delete tupdesc if we have it */ - if (typentry->tupDesc != NULL) + if (typentry != NULL) { - /* - * Release our refcount, and free the tupdesc if none remain. - * (Can't use DecrTupleDescRefCount because this reference is - * not logged in current resource owner.) - */ - Assert(typentry->tupDesc->tdrefcount > 0); - if (--typentry->tupDesc->tdrefcount == 0) - FreeTupleDesc(typentry->tupDesc); - typentry->tupDesc = NULL; - - /* - * Also clear tupDesc_identifier, so that anything watching - * that will realize that the tupdesc has possibly changed. - * (Alternatively, we could specify that to detect possible - * tupdesc change, one must check for tupDesc != NULL as well - * as tupDesc_identifier being the same as what was previously - * seen. That seems error-prone.) - */ - typentry->tupDesc_identifier = 0; + Assert(typentry->typtype == TYPTYPE_COMPOSITE); + Assert(relid == typentry->typrelid); + + invalidateCompositeTypeCacheEntry(typentry); } + } - /* Reset equality/comparison/hashing validity information */ - typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; + for (typentry = firstDomainTypeEntry; + typentry != NULL; + typentry = typentry->nextDomain) + { + invalidateCompositeTypeCacheEntry(typentry); } - else if (typentry->typtype == TYPTYPE_DOMAIN) + } + else + { + HASH_SEQ_STATUS status; + + /* + * Relid = 0, so we need to reset all composite types in cache. Also, we + * should reset flags for domain types, and we loop over all entries + * in hash, so, do it in a single scan. + */ + hash_seq_init(&status, TypeCacheHash); + while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) { - /* - * If it's domain over composite, reset flags. (We don't bother - * trying to determine whether the specific base type needs a - * reset.) Note that if we haven't determined whether the base - * type is composite, we don't need to reset anything. - */ - if (typentry->flags & TCFLAGS_DOMAIN_BASE_IS_COMPOSITE) - typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; + invalidateCompositeTypeCacheEntry(typentry); } } } ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-04-03 06:07 Andrei Lepikhov <[email protected]> parent: Teodor Sigaev <[email protected]> 1 sibling, 1 reply; 28+ messages in thread From: Andrei Lepikhov @ 2024-04-03 06:07 UTC (permalink / raw) To: Teodor Sigaev <[email protected]>; pgsql-hackers; +Cc: Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]> On 3/15/24 17:57, Teodor Sigaev wrote: >> Okay, I've applied this piece for now. Not sure I'll have much room >> to look at the rest. > > Thank you very much! I have spent some time reviewing this feature. I think we can discuss and apply it step-by-step. So, the 0001-* patch is at this moment. The feature addresses the issue of TypCache being bloated by intensive usage of non-standard types and domains. It adds significant overhead during relcache invalidation by thoroughly scanning this hash table. IMO, this feature will be handy soon, as we already see some patches where TypCache is intensively used for storing composite types—for example, look into solutions proposed in [1]. One of my main concerns with this feature is the possibility of lost entries, which could be mistakenly used by relations with the same oid in the future. This seems particularly possible in cases with multiple temporary tables. The author has attempted to address this by replacing the typrelid and type_id fields in the mapRelType on each call of lookup_type_cache. However, I believe we could further improve this by removing the entry from mapRelType on invalidation, thus avoiding this potential issue. While reviewing the patch, I made some minor changes (see attachment) that you're free to adopt or reject. However, it's crucial that the patch includes a detailed explanation, not just a single sentence, to ensure everyone understands the changes. Upon closer inspection, I noticed that the current implementation only invalidates the cache entry. While this is acceptable for standard types, it may not be sufficient to maintain numerous custom types (as in the example in the initial letter) or in cases where whole-row vars are heavily used. In such scenarios, removing the entry and reducing the hash table's size might be more efficient. In toto, the 0001-* patch looks good, and I would be glad to see it in the core. [1] https://www.postgresql.org/message-id/flat/CAKcux6ktu-8tefLWtQuuZBYFaZA83vUzuRd7c1YHC-yEWyYFpg%40mai... -- regards, Andrei Lepikhov Postgres Professional Attachments: [text/x-patch] 0001-minor_improvements.diff (3.3K, ../../[email protected]/2-0001-minor_improvements.diff) download | inline diff: diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c index e3c32c7848..ed321603d5 100644 --- a/src/backend/utils/cache/typcache.c +++ b/src/backend/utils/cache/typcache.c @@ -74,16 +74,17 @@ #include "utils/typcache.h" -/* The main type cache hashtable searched by lookup_type_cache */ -static HTAB *TypeCacheHash = NULL; - /* The map from relation's oid to its type oid */ -typedef struct mapRelTypeEntry +typedef struct RelTypeMapEntry { Oid typrelid; Oid type_id; -} mapRelTypeEntry; +} RelTypeMapEntry; + +/* The main type cache hashtable searched by lookup_type_cache */ +static HTAB *TypeCacheHash = NULL; +/* Utility hash table to speed up processing of invalidation relcache events. */ static HTAB *mapRelType = NULL; /* List of type cache entries for domain types */ @@ -368,7 +369,7 @@ lookup_type_cache(Oid type_id, int flags) &ctl, HASH_ELEM | HASH_BLOBS); ctl.keysize = sizeof(Oid); - ctl.entrysize = sizeof(mapRelTypeEntry); + ctl.entrysize = sizeof(RelTypeMapEntry); mapRelType = hash_create("Map reloid to typeoid", 64, &ctl, HASH_ELEM | HASH_BLOBS); @@ -492,11 +493,11 @@ lookup_type_cache(Oid type_id, int flags) */ if (OidIsValid(typentry->typrelid) && typentry->typtype == TYPTYPE_COMPOSITE) { - mapRelTypeEntry *relentry; + RelTypeMapEntry *relentry; - relentry = (mapRelTypeEntry*) hash_search(mapRelType, - &typentry->typrelid, - HASH_ENTER, NULL); + relentry = (RelTypeMapEntry *) hash_search(mapRelType, + &typentry->typrelid, + HASH_ENTER, NULL); relentry->typrelid = typentry->typrelid; relentry->type_id = typentry->type_id; @@ -2297,7 +2298,7 @@ SharedRecordTypmodRegistryAttach(SharedRecordTypmodRegistry *registry) } static void -invalidateCompositeTypeCacheEntry(TypeCacheEntry *typentry) +invalidateTypeCacheEntry(TypeCacheEntry *typentry) { /* Delete tupdesc if we have it */ if (typentry->tupDesc != NULL) @@ -2348,11 +2349,11 @@ TypeCacheRelCallback(Datum arg, Oid relid) if (OidIsValid(relid)) { - mapRelTypeEntry *relentry; + RelTypeMapEntry *relentry; - relentry = (mapRelTypeEntry *) hash_search(mapRelType, - &relid, - HASH_FIND, NULL); + relentry = (RelTypeMapEntry *) hash_search(mapRelType, + &relid, + HASH_FIND, NULL); if (relentry != NULL) { @@ -2365,7 +2366,7 @@ TypeCacheRelCallback(Datum arg, Oid relid) Assert(typentry->typtype == TYPTYPE_COMPOSITE); Assert(relid == typentry->typrelid); - invalidateCompositeTypeCacheEntry(typentry); + invalidateTypeCacheEntry(typentry); } } @@ -2397,7 +2398,7 @@ TypeCacheRelCallback(Datum arg, Oid relid) { if (typentry->typtype == TYPTYPE_COMPOSITE) { - invalidateCompositeTypeCacheEntry(typentry); + invalidateTypeCacheEntry(typentry); } else if (typentry->typtype == TYPTYPE_DOMAIN) { diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index cfa9d5aaea..8f24690306 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2347,6 +2347,7 @@ RelocationBufferInfo RelptrFreePageBtree RelptrFreePageManager RelptrFreePageSpanLeader +RelTypeMapEntry RemoteSlot RenameStmt ReopenPtrType ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-08-05 01:16 Alexander Korotkov <[email protected]> parent: Andrei Lepikhov <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Alexander Korotkov @ 2024-08-05 01:16 UTC (permalink / raw) To: Andrei Lepikhov <[email protected]>; +Cc: Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]> Hi! On Wed, Apr 3, 2024 at 9:07 AM Andrei Lepikhov <[email protected]> wrote: > On 3/15/24 17:57, Teodor Sigaev wrote: > >> Okay, I've applied this piece for now. Not sure I'll have much room > >> to look at the rest. > > > > Thank you very much! > I have spent some time reviewing this feature. I think we can discuss > and apply it step-by-step. So, the 0001-* patch is at this moment. > The feature addresses the issue of TypCache being bloated by intensive > usage of non-standard types and domains. It adds significant overhead > during relcache invalidation by thoroughly scanning this hash table. > IMO, this feature will be handy soon, as we already see some patches > where TypCache is intensively used for storing composite types—for > example, look into solutions proposed in [1]. > One of my main concerns with this feature is the possibility of lost > entries, which could be mistakenly used by relations with the same oid > in the future. This seems particularly possible in cases with multiple > temporary tables. The author has attempted to address this by replacing > the typrelid and type_id fields in the mapRelType on each call of > lookup_type_cache. However, I believe we could further improve this by > removing the entry from mapRelType on invalidation, thus avoiding this > potential issue. > While reviewing the patch, I made some minor changes (see attachment) > that you're free to adopt or reject. However, it's crucial that the > patch includes a detailed explanation, not just a single sentence, to > ensure everyone understands the changes. > Upon closer inspection, I noticed that the current implementation only > invalidates the cache entry. While this is acceptable for standard > types, it may not be sufficient to maintain numerous custom types (as in > the example in the initial letter) or in cases where whole-row vars are > heavily used. In such scenarios, removing the entry and reducing the > hash table's size might be more efficient. > In toto, the 0001-* patch looks good, and I would be glad to see it in > the core. I've revised the patchset. First of all, I've re-ordered the patches. 0001-0002 (former 0002-0003) Comprises hash_search_with_hash_value() function and its application to avoid full hash iteration in InvalidateAttoptCacheCallback() and TypeCacheTypCallback(). I think this is quite straightforward optimization without negative side effects. I've revised comments, commit message and did some code beautification. I'm going to push this if no objections. 0003 (former 0001) I've revised this patch. I think main concerns expressed in the thread about this path is that we don't have invalidation mechanism for relid => typid map. Finally due to oid wraparound same relids could get reused. That could lead to invalid entries in the map about existing relids and typeids. This is rather messy, but I don't think this could cause a material bug. The maps items are used only for cache invalidation. Extra invalidation doesn't cause a bug. If type with same relid will be cached, then correspoding map item will be overridden, so no missing invalidation. However, I see the following reasons for keeping consistent state of relid => typid map. 1) As the main use-case for this optimization is flood of temporary tables, it would be nice not let relid => typid map bloat in this case. I see that TypeCacheHash would get bloated, because its entries are never deleted. However, I would prefer to not get this situation even worse. 2) In future we may find some more use-cases for relid => typid map besides cache invalidation. Keeping that in consistent state could be advantage then. In the attached patch, I'm keeping relid => typid map when corresponding typentry have either TCFLAGS_HAVE_PG_TYPE_DATA, or TCFLAGS_OPERATOR_FLAGS, or tupdesc. Thus, when temporary table gets deleted, we would invalidate the map item. It will be also nice to get rid of iteration over all the cached domain types in TypeCacheRelCallback(). However, this typically shouldn't be a problem since domain types are less tended to bloat. Domain types are created manually, unlike composite types which are automatically created for every temporary table. We will probably need to optimize this in future, but I don't feel this to be necessary in present patch. I think the revised 0003 requires review. ------ Regards, Alexander Korotkov Supabase Attachments: [application/octet-stream] v7-0001-Introduce-hash_search_with_hash_value-function.patch (3.3K, ../../CAPpHfdtNU6Lz21_ZtNDg_d+WoUvK3Togukp+cMu0cQp7o1w_OA@mail.gmail.com/2-v7-0001-Introduce-hash_search_with_hash_value-function.patch) download | inline diff: From 54ead85554d3bb571b74f2ac85aea2cd330ddc2f Mon Sep 17 00:00:00 2001 From: Alexander Korotkov <[email protected]> Date: Mon, 5 Aug 2024 00:34:08 +0300 Subject: [PATCH v7 1/3] Introduce hash_search_with_hash_value() function This new function iterates hash entries with given hash values. This function is designed to avoid full sequential hash search in the syscache invalidation callbacks. Discussion: https://postgr.es/m/5812a6e5-68ae-4d84-9d85-b443176966a1%40sigaev.ru Author: Teodor Sigaev Reviewed-by: Aleksander Alekseev, Tom Lane, Michael Paquier, Roman Zharkov Reviewed-by: Andrei Lepikhov --- src/backend/utils/hash/dynahash.c | 38 +++++++++++++++++++++++++++++++ src/include/utils/hsearch.h | 4 ++++ 2 files changed, 42 insertions(+) diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c index 145e058fe67..8040416a13c 100644 --- a/src/backend/utils/hash/dynahash.c +++ b/src/backend/utils/hash/dynahash.c @@ -1387,10 +1387,30 @@ hash_seq_init(HASH_SEQ_STATUS *status, HTAB *hashp) status->hashp = hashp; status->curBucket = 0; status->curEntry = NULL; + status->hasHashvalue = false; if (!hashp->frozen) register_seq_scan(hashp); } +/* + * Same as above but scan by the given hash value. + * See also hash_seq_search(). + */ +void +hash_seq_init_with_hash_value(HASH_SEQ_STATUS *status, HTAB *hashp, + uint32 hashvalue) +{ + HASHBUCKET *bucketPtr; + + hash_seq_init(status, hashp); + + status->hasHashvalue = true; + status->hashvalue = hashvalue; + + status->curBucket = hash_initial_lookup(hashp, hashvalue, &bucketPtr); + status->curEntry = *bucketPtr; +} + void * hash_seq_search(HASH_SEQ_STATUS *status) { @@ -1404,6 +1424,24 @@ hash_seq_search(HASH_SEQ_STATUS *status) uint32 curBucket; HASHELEMENT *curElem; + if (status->hasHashvalue) + { + /* + * Scan entries only in the current bucket because only this bucket + * can contain entries with the given hash value. + */ + while ((curElem = status->curEntry) != NULL) + { + status->curEntry = curElem->link; + if (status->hashvalue != curElem->hashvalue) + continue; + return (void *) ELEMENTKEY(curElem); + } + + hash_seq_term(status); + return NULL; + } + if ((curElem = status->curEntry) != NULL) { /* Continuing scan of curBucket... */ diff --git a/src/include/utils/hsearch.h b/src/include/utils/hsearch.h index da26941f6db..c99d74625f7 100644 --- a/src/include/utils/hsearch.h +++ b/src/include/utils/hsearch.h @@ -122,6 +122,8 @@ typedef struct HTAB *hashp; uint32 curBucket; /* index of current bucket */ HASHELEMENT *curEntry; /* current entry in bucket */ + bool hasHashvalue; /* true if hashvalue was provided */ + uint32 hashvalue; /* hashvalue to start seqscan over hash */ } HASH_SEQ_STATUS; /* @@ -141,6 +143,8 @@ extern bool hash_update_hash_key(HTAB *hashp, void *existingEntry, const void *newKeyPtr); extern long hash_get_num_entries(HTAB *hashp); extern void hash_seq_init(HASH_SEQ_STATUS *status, HTAB *hashp); +extern void hash_seq_init_with_hash_value(HASH_SEQ_STATUS *status, HTAB *hashp, + uint32 hashvalue); extern void *hash_seq_search(HASH_SEQ_STATUS *status); extern void hash_seq_term(HASH_SEQ_STATUS *status); extern void hash_freeze(HTAB *hashp); -- 2.39.3 (Apple Git-145) [application/octet-stream] v7-0002-Optimize-InvalidateAttoptCacheCallback-and-TypeCa.patch (6.7K, ../../CAPpHfdtNU6Lz21_ZtNDg_d+WoUvK3Togukp+cMu0cQp7o1w_OA@mail.gmail.com/3-v7-0002-Optimize-InvalidateAttoptCacheCallback-and-TypeCa.patch) download | inline diff: From b7db567611f6ac7f9a6e550f2e7863bbef5bc361 Mon Sep 17 00:00:00 2001 From: Alexander Korotkov <[email protected]> Date: Mon, 5 Aug 2024 00:44:49 +0300 Subject: [PATCH v7 2/3] Optimize InvalidateAttoptCacheCallback() and TypeCacheTypCallback() These callbacks are receiving hash values as arguments, which doesn't allow direct lookups for AttoptCacheHash and TypeCacheHash. This is why subject callbacks currently use full iteration over corresponding hashes. This commit avoids full hash interation in InvalidateAttoptCacheCallback(), and TypeCacheTypCallback(). At first, we switch AttoptCacheHash and TypeCacheHash to use same hash function as syscache. As second, we use hash_seq_init_with_hash_value() to iterate only hash entries with matching hash value. Discussion: https://postgr.es/m/5812a6e5-68ae-4d84-9d85-b443176966a1%40sigaev.ru Author: Teodor Sigaev Reviewed-by: Aleksander Alekseev, Tom Lane, Michael Paquier, Roman Zharkov Reviewed-by: Andrei Lepikhov --- src/backend/utils/cache/attoptcache.c | 39 ++++++++++++++++--- src/backend/utils/cache/typcache.c | 55 +++++++++++++++++++-------- 2 files changed, 73 insertions(+), 21 deletions(-) diff --git a/src/backend/utils/cache/attoptcache.c b/src/backend/utils/cache/attoptcache.c index af978ccd4b1..bd2e07080bd 100644 --- a/src/backend/utils/cache/attoptcache.c +++ b/src/backend/utils/cache/attoptcache.c @@ -44,12 +44,10 @@ typedef struct /* * InvalidateAttoptCacheCallback - * Flush all cache entries when pg_attribute is updated. + * Flush cache entry (or entries) when pg_attribute is updated. * * When pg_attribute is updated, we must flush the cache entry at least - * for that attribute. Currently, we just flush them all. Since attribute - * options are not currently used in performance-critical paths (such as - * query execution), this seems OK. + * for that attribute. */ static void InvalidateAttoptCacheCallback(Datum arg, int cacheid, uint32 hashvalue) @@ -57,7 +55,16 @@ InvalidateAttoptCacheCallback(Datum arg, int cacheid, uint32 hashvalue) HASH_SEQ_STATUS status; AttoptCacheEntry *attopt; - hash_seq_init(&status, AttoptCacheHash); + /* + * By convection, zero hash value is passed to the callback as a sign that + * it's time to invalidate the cache. See sinval.c, inval.c and + * InvalidateSystemCachesExtended(). + */ + if (hashvalue == 0) + hash_seq_init(&status, AttoptCacheHash); + else + hash_seq_init_with_hash_value(&status, AttoptCacheHash, hashvalue); + while ((attopt = (AttoptCacheEntry *) hash_seq_search(&status)) != NULL) { if (attopt->opts) @@ -70,6 +77,18 @@ InvalidateAttoptCacheCallback(Datum arg, int cacheid, uint32 hashvalue) } } +/* + * Hash function compatible with two-arg system cache hash function. + */ +static uint32 +relatt_cache_syshash(const void *key, Size keysize) +{ + const AttoptCacheKey *ckey = key; + + Assert(keysize == sizeof(*ckey)); + return GetSysCacheHashValue2(ATTNUM, ckey->attrelid, ckey->attnum); +} + /* * InitializeAttoptCache * Initialize the attribute options cache. @@ -82,9 +101,17 @@ InitializeAttoptCache(void) /* Initialize the hash table. */ ctl.keysize = sizeof(AttoptCacheKey); ctl.entrysize = sizeof(AttoptCacheEntry); + + /* + * AttoptCacheEntry takes hash value from the system cache. For + * AttoptCacheHash we use the same hash in order to speedup search by hash + * value. This is used by hash_seq_init_with_hash_value(). + */ + ctl.hash = relatt_cache_syshash; + AttoptCacheHash = hash_create("Attopt cache", 256, &ctl, - HASH_ELEM | HASH_BLOBS); + HASH_ELEM | HASH_FUNCTION); /* Make sure we've initialized CacheMemoryContext. */ if (!CacheMemoryContext) diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c index aa4720cb598..a6d9ce0c513 100644 --- a/src/backend/utils/cache/typcache.c +++ b/src/backend/utils/cache/typcache.c @@ -331,6 +331,16 @@ static dsa_pointer share_tupledesc(dsa_area *area, TupleDesc tupdesc, uint32 typmod); +/* + * Hash function compatible with one-arg system cache hash function. + */ +static uint32 +type_cache_syshash(const void *key, Size keysize) +{ + Assert(keysize == sizeof(Oid)); + return GetSysCacheHashValue1(TYPEOID, ObjectIdGetDatum(*(const Oid *) key)); +} + /* * lookup_type_cache * @@ -355,8 +365,16 @@ lookup_type_cache(Oid type_id, int flags) ctl.keysize = sizeof(Oid); ctl.entrysize = sizeof(TypeCacheEntry); + + /* + * TypeEntry takes hash value from the system cache. For TypeCacheHash + * we use the same hash in order to speedup search by hash value. This + * is used by hash_seq_init_with_hash_value(). + */ + ctl.hash = type_cache_syshash; + TypeCacheHash = hash_create("Type information cache", 64, - &ctl, HASH_ELEM | HASH_BLOBS); + &ctl, HASH_ELEM | HASH_FUNCTION); /* Also set up callbacks for SI invalidations */ CacheRegisterRelcacheCallback(TypeCacheRelCallback, (Datum) 0); @@ -407,8 +425,7 @@ lookup_type_cache(Oid type_id, int flags) /* These fields can never change, by definition */ typentry->type_id = type_id; - typentry->type_id_hash = GetSysCacheHashValue1(TYPEOID, - ObjectIdGetDatum(type_id)); + typentry->type_id_hash = get_hash_value(TypeCacheHash, &type_id); /* Keep this part in sync with the code below */ typentry->typlen = typtup->typlen; @@ -2358,20 +2375,28 @@ TypeCacheTypCallback(Datum arg, int cacheid, uint32 hashvalue) TypeCacheEntry *typentry; /* TypeCacheHash must exist, else this callback wouldn't be registered */ - hash_seq_init(&status, TypeCacheHash); + + /* + * By convection, zero hash value is passed to the callback as a sign that + * it's time to invalidate the cache. See sinval.c, inval.c and + * InvalidateSystemCachesExtended(). + */ + if (hashvalue == 0) + hash_seq_init(&status, TypeCacheHash); + else + hash_seq_init_with_hash_value(&status, TypeCacheHash, hashvalue); + while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) { - /* Is this the targeted type row (or it's a total cache flush)? */ - if (hashvalue == 0 || typentry->type_id_hash == hashvalue) - { - /* - * Mark the data obtained directly from pg_type as invalid. Also, - * if it's a domain, typnotnull might've changed, so we'll need to - * recalculate its constraints. - */ - typentry->flags &= ~(TCFLAGS_HAVE_PG_TYPE_DATA | - TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS); - } + Assert(hashvalue == 0 || typentry->type_id_hash == hashvalue); + + /* + * Mark the data obtained directly from pg_type as invalid. Also, if + * it's a domain, typnotnull might've changed, so we'll need to + * recalculate its constraints. + */ + typentry->flags &= ~(TCFLAGS_HAVE_PG_TYPE_DATA | + TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS); } } -- 2.39.3 (Apple Git-145) [application/octet-stream] v7-0003-Avoid-looping-over-all-type-cache-entries-in-Type.patch (14.7K, ../../CAPpHfdtNU6Lz21_ZtNDg_d+WoUvK3Togukp+cMu0cQp7o1w_OA@mail.gmail.com/4-v7-0003-Avoid-looping-over-all-type-cache-entries-in-Type.patch) download | inline diff: From 6a97ad5f4d9822edafe3f6f3aec939d7bdc2ef4b Mon Sep 17 00:00:00 2001 From: Alexander Korotkov <[email protected]> Date: Mon, 5 Aug 2024 00:20:24 +0300 Subject: [PATCH v7 3/3] Avoid looping over all type cache entries in TypeCacheRelCallback() Currently when a single relcache entry gets invalidated, TypeCacheRelCallback() has to loop over all type cache entries to find appropriate typentry to invalidate. Unfortunately, using the syscache here is impossible, because this callback could be called outside a transaction and this makes impossible catalog lookups. This is why present commit introduces RelIdToTypeIdCacheHash to map relation oid to its composite type oid. We are keeping RelIdToTypeIdCacheHash entry while corresponding type cache entry have something to clean. Therefore, RelIdToTypeIdCacheHash shouldn't get bloat in the case of temporary tables flood. Discussion: https://postgr.es/m/5812a6e5-68ae-4d84-9d85-b443176966a1%40sigaev.ru Author: Teodor Sigaev Reviewed-by: Aleksander Alekseev, Tom Lane, Michael Paquier, Roman Zharkov Reviewed-by: Andrei Lepikhov --- src/backend/utils/cache/typcache.c | 295 ++++++++++++++++++++++++----- src/tools/pgindent/typedefs.list | 1 + 2 files changed, 251 insertions(+), 45 deletions(-) diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c index a6d9ce0c513..8a93e060ac2 100644 --- a/src/backend/utils/cache/typcache.c +++ b/src/backend/utils/cache/typcache.c @@ -77,6 +77,20 @@ /* The main type cache hashtable searched by lookup_type_cache */ static HTAB *TypeCacheHash = NULL; +/* + * The map from relation's oid to the corresponding composite type oid. We're + * keeping the map entry when corresponding typentry have either + * TCFLAGS_HAVE_PG_TYPE_DATA, or TCFLAGS_OPERATOR_FLAGS, or tupdesc. That is + * we're keeping map entry if the entry has something to clear. + */ +static HTAB *RelIdToTypeIdCacheHash = NULL; + +typedef struct RelIdToTypeIdCacheEntry +{ + Oid relid; /* OID of the relation */ + Oid composite_typid; /* OID of the relation's composite type */ +} RelIdToTypeIdCacheEntry; + /* List of type cache entries for domain types */ static TypeCacheEntry *firstDomainTypeEntry = NULL; @@ -329,7 +343,8 @@ static void shared_record_typmod_registry_detach(dsm_segment *segment, static TupleDesc find_or_make_matching_shared_tupledesc(TupleDesc tupdesc); static dsa_pointer share_tupledesc(dsa_area *area, TupleDesc tupdesc, uint32 typmod); - +static void check_insert_rel_type_cache(TypeCacheEntry *typentry); +static void check_delete_rel_type_cache(TypeCacheEntry *typentry); /* * Hash function compatible with one-arg system cache hash function. @@ -376,6 +391,13 @@ lookup_type_cache(Oid type_id, int flags) TypeCacheHash = hash_create("Type information cache", 64, &ctl, HASH_ELEM | HASH_FUNCTION); + Assert(RelIdToTypeIdCacheHash == NULL); + + ctl.keysize = sizeof(Oid); + ctl.entrysize = sizeof(RelIdToTypeIdCacheEntry); + RelIdToTypeIdCacheHash = hash_create("Map from relid to oid of cached composite type", 64, + &ctl, HASH_ELEM | HASH_BLOBS); + /* Also set up callbacks for SI invalidations */ CacheRegisterRelcacheCallback(TypeCacheRelCallback, (Datum) 0); CacheRegisterSyscacheCallback(TYPEOID, TypeCacheTypCallback, (Datum) 0); @@ -387,6 +409,8 @@ lookup_type_cache(Oid type_id, int flags) CreateCacheMemoryContext(); } + Assert(TypeCacheHash != NULL && RelIdToTypeIdCacheHash != NULL); + /* Try to look up an existing entry */ typentry = (TypeCacheEntry *) hash_search(TypeCacheHash, &type_id, @@ -438,6 +462,7 @@ lookup_type_cache(Oid type_id, int flags) typentry->typelem = typtup->typelem; typentry->typcollation = typtup->typcollation; typentry->flags |= TCFLAGS_HAVE_PG_TYPE_DATA; + check_insert_rel_type_cache(typentry); /* If it's a domain, immediately thread it into the domain cache list */ if (typentry->typtype == TYPTYPE_DOMAIN) @@ -483,6 +508,7 @@ lookup_type_cache(Oid type_id, int flags) typentry->typelem = typtup->typelem; typentry->typcollation = typtup->typcollation; typentry->flags |= TCFLAGS_HAVE_PG_TYPE_DATA; + check_insert_rel_type_cache(typentry); ReleaseSysCache(tp); } @@ -2281,6 +2307,53 @@ SharedRecordTypmodRegistryAttach(SharedRecordTypmodRegistry *registry) CurrentSession->shared_typmod_table = typmod_table; } +/* + * InvalidateCompositeTypeCacheEntry + * Invalidate particular TypeCacheEntry on Relcache inval callback + * + * Delete the cached tuple descriptor (if any) for the given composite + * type, and reset whatever info we have cached about the composite type's + * comparability. + */ +static void +InvalidateCompositeTypeCacheEntry(TypeCacheEntry *typentry) +{ + bool hadTupDescOrOpclass; + + Assert(typentry->typtype == TYPTYPE_COMPOSITE && + OidIsValid(typentry->typrelid)); + + hadTupDescOrOpclass = (typentry->tupDesc != NULL) || + (typentry->flags & TCFLAGS_OPERATOR_FLAGS); + + /* Delete tupdesc if we have it */ + if (typentry->tupDesc != NULL) + { + /* + * Release our refcount and free the tupdesc if none remain. We can't + * use DecrTupleDescRefCount here because this reference is not logged + * by the current resource owner. + */ + Assert(typentry->tupDesc->tdrefcount > 0); + if (--typentry->tupDesc->tdrefcount == 0) + FreeTupleDesc(typentry->tupDesc); + typentry->tupDesc = NULL; + + /* + * Also clear tupDesc_identifier, so that anyone watching it will + * realize that the tupdesc has changed. + */ + typentry->tupDesc_identifier = 0; + } + + /* Reset equality/comparison/hashing validity information */ + typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; + + /* Call check_delete_rel_type_cache() if we actually cleared something */ + if (hadTupDescOrOpclass) + check_delete_rel_type_cache(typentry); +} + /* * TypeCacheRelCallback * Relcache inval callback function @@ -2290,63 +2363,55 @@ SharedRecordTypmodRegistryAttach(SharedRecordTypmodRegistry *registry) * whatever info we have cached about the composite type's comparability. * * This is called when a relcache invalidation event occurs for the given - * relid. We must scan the whole typcache hash since we don't know the - * type OID corresponding to the relid. We could do a direct search if this - * were a syscache-flush callback on pg_type, but then we would need all - * ALTER-TABLE-like commands that could modify a rowtype to issue syscache - * invals against the rel's pg_type OID. The extra SI signaling could very - * well cost more than we'd save, since in most usages there are not very - * many entries in a backend's typcache. The risk of bugs-of-omission seems - * high, too. - * - * Another possibility, with only localized impact, is to maintain a second - * hashtable that indexes composite-type typcache entries by their typrelid. - * But it's still not clear it's worth the trouble. + * relid. We can't use syscache to find a type corresponding to the given + * relation because the code can be called outside of transaction. Thus we use + * the RelIdToTypeIdCacheHash map to locate appropriate typcache entry. */ static void TypeCacheRelCallback(Datum arg, Oid relid) { - HASH_SEQ_STATUS status; TypeCacheEntry *typentry; - /* TypeCacheHash must exist, else this callback wouldn't be registered */ - hash_seq_init(&status, TypeCacheHash); - while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) + /* + * RelIdToTypeIdCacheHash and TypeCacheHash should exist, otherwise this + * callback wouldn't be registered + */ + if (OidIsValid(relid)) { - if (typentry->typtype == TYPTYPE_COMPOSITE) + RelIdToTypeIdCacheEntry *relentry; + + /* + * Find an RelIdToTypeIdCacheHash entry, which should exist as soon as + * corresponding typcache entry has something to clean. + */ + relentry = (RelIdToTypeIdCacheEntry *) hash_search(RelIdToTypeIdCacheHash, + &relid, + HASH_FIND, NULL); + + if (relentry != NULL) { - /* Skip if no match, unless we're zapping all composite types */ - if (relid != typentry->typrelid && relid != InvalidOid) - continue; + typentry = (TypeCacheEntry *) hash_search(TypeCacheHash, + &relentry->composite_typid, + HASH_FIND, NULL); - /* Delete tupdesc if we have it */ - if (typentry->tupDesc != NULL) + if (typentry != NULL) { - /* - * Release our refcount, and free the tupdesc if none remain. - * (Can't use DecrTupleDescRefCount because this reference is - * not logged in current resource owner.) - */ - Assert(typentry->tupDesc->tdrefcount > 0); - if (--typentry->tupDesc->tdrefcount == 0) - FreeTupleDesc(typentry->tupDesc); - typentry->tupDesc = NULL; + Assert(typentry->typtype == TYPTYPE_COMPOSITE); + Assert(relid == typentry->typrelid); - /* - * Also clear tupDesc_identifier, so that anything watching - * that will realize that the tupdesc has possibly changed. - * (Alternatively, we could specify that to detect possible - * tupdesc change, one must check for tupDesc != NULL as well - * as tupDesc_identifier being the same as what was previously - * seen. That seems error-prone.) - */ - typentry->tupDesc_identifier = 0; + InvalidateCompositeTypeCacheEntry(typentry); } - - /* Reset equality/comparison/hashing validity information */ - typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; } - else if (typentry->typtype == TYPTYPE_DOMAIN) + + /* + * Visit all the domain types sequentially. Typically shouldn't be a + * problem since domain types are less tended to bloat. Domain types + * are created manually, unlike composite types which are + * automatically created for every temporary table. + */ + for (typentry = firstDomainTypeEntry; + typentry != NULL; + typentry = typentry->nextDomain) { /* * If it's domain over composite, reset flags. (We don't bother @@ -2358,6 +2423,36 @@ TypeCacheRelCallback(Datum arg, Oid relid) typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; } } + else + { + HASH_SEQ_STATUS status; + + /* + * Relid = 0, so we need to reset all composite types in cache. Also, + * we should reset flags for domain types, and we loop over all + * entries in hash, so, do it in a single scan. + */ + hash_seq_init(&status, TypeCacheHash); + while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) + { + if (typentry->typtype == TYPTYPE_COMPOSITE) + { + InvalidateCompositeTypeCacheEntry(typentry); + } + else if (typentry->typtype == TYPTYPE_DOMAIN) + { + /* + * If it's domain over composite, reset flags. (We don't + * bother trying to determine whether the specific base type + * needs a reset.) Note that if we haven't determined whether + * the base type is composite, we don't need to reset + * anything. + */ + if (typentry->flags & TCFLAGS_DOMAIN_BASE_IS_COMPOSITE) + typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; + } + } + } } /* @@ -2388,6 +2483,8 @@ TypeCacheTypCallback(Datum arg, int cacheid, uint32 hashvalue) while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) { + bool hadPgTypeData = (typentry->flags & TCFLAGS_HAVE_PG_TYPE_DATA); + Assert(hashvalue == 0 || typentry->type_id_hash == hashvalue); /* @@ -2397,6 +2494,10 @@ TypeCacheTypCallback(Datum arg, int cacheid, uint32 hashvalue) */ typentry->flags &= ~(TCFLAGS_HAVE_PG_TYPE_DATA | TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS); + + /* Call check_delete_rel_type_cache() if cleaned TCFLAGS_HAVE_PG_TYPE_DATA flag. */ + if (hadPgTypeData) + check_delete_rel_type_cache(typentry); } } @@ -2905,3 +3006,107 @@ shared_record_typmod_registry_detach(dsm_segment *segment, Datum datum) } CurrentSession->shared_typmod_registry = NULL; } + +/* + * Insert RelIdToTypeIdCacheHash entry after setting of the + * TCFLAGS_HAVE_PG_TYPE_DATA flag if needed. + */ +static void +check_insert_rel_type_cache(TypeCacheEntry *typentry) +{ + Assert(typentry->flags & TCFLAGS_HAVE_PG_TYPE_DATA); + + /* Immediately quit for non-composite types */ + if (typentry->typtype != TYPTYPE_COMPOSITE) + return; + + /* typrelid should be given for composite types */ + Assert(OidIsValid(typentry->typrelid)); + + /* + * Insert a RelIdToTypeIdCacheHash entry if the typentry doesn't have any + * information indicating there should be such an entry already. + */ + if (!(typentry->flags & TCFLAGS_OPERATOR_FLAGS) && + typentry->tupDesc == NULL) + { + RelIdToTypeIdCacheEntry *relentry; + bool found; + + relentry = (RelIdToTypeIdCacheEntry *) hash_search(RelIdToTypeIdCacheHash, + &typentry->typrelid, + HASH_ENTER, &found); + Assert(!found); + relentry->relid = typentry->typrelid; + relentry->composite_typid = typentry->type_id; + } + +#ifdef USE_ASSERT_CHECKING + + /* + * In assert-enabled builds otherwise check for RelIdToTypeIdCacheHash + * entry if it should exist. + */ + if (!(typentry->flags & TCFLAGS_OPERATOR_FLAGS) && + typentry->tupDesc == NULL) + { + bool found; + + (void) hash_search(RelIdToTypeIdCacheHash, + &typentry->typrelid, + HASH_FIND, &found); + Assert(found); + } +#endif +} + +/* + * Delete entry RelIdToTypeIdCacheHash if needed after resetting of the + * TCFLAGS_HAVE_PG_TYPE_DATA flag, or any of TCFLAGS_OPERATOR_FLAGS flags, + * or tupDesc if needed. + */ +static void +check_delete_rel_type_cache(TypeCacheEntry *typentry) +{ + /* Immediately quit for non-composite types */ + if (typentry->typtype != TYPTYPE_COMPOSITE) + return; + + /* typrelid should be given for composite types */ + Assert(OidIsValid(typentry->typrelid)); + + /* + * Delete a RelIdToTypeIdCacheHash entry if the typentry doesn't have any + * information indicating entry should be still there. + */ + if (!(typentry->flags & TCFLAGS_HAVE_PG_TYPE_DATA) && + !(typentry->flags & TCFLAGS_OPERATOR_FLAGS) && + typentry->tupDesc == NULL) + { + bool found; + + (void) hash_search(RelIdToTypeIdCacheHash, + &typentry->typrelid, + HASH_REMOVE, &found); + Assert(found); + } + +#ifdef USE_ASSERT_CHECKING + + /* + * In assert-enabled builds otherwise check for RelIdToTypeIdCacheHash + * entry if it should exist. + */ + if ((typentry->flags & TCFLAGS_HAVE_PG_TYPE_DATA) || + (typentry->flags & TCFLAGS_OPERATOR_FLAGS) || + typentry->tupDesc != NULL) + { + bool found; + + (void) hash_search(RelIdToTypeIdCacheHash, + &typentry->typrelid, + HASH_FIND, &found); + Assert(found); + } +#endif +} diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 75fc05093cc..2ac7199a6ce 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2371,6 +2371,7 @@ RelFileLocator RelFileLocatorBackend RelFileNumber RelIdCacheEnt +RelIdToTypeIdCacheEntry RelInfo RelInfoArr RelMapFile -- 2.39.3 (Apple Git-145) ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-08-20 19:00 Alexander Korotkov <[email protected]> parent: Alexander Korotkov <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Alexander Korotkov @ 2024-08-20 19:00 UTC (permalink / raw) To: Andrei Lepikhov <[email protected]>; +Cc: Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]> On Mon, Aug 5, 2024 at 4:16 AM Alexander Korotkov <[email protected]> wrote: > I've revised the patchset. First of all, I've re-ordered the patches. > > 0001-0002 (former 0002-0003) > Comprises hash_search_with_hash_value() function and its application > to avoid full hash iteration in InvalidateAttoptCacheCallback() and > TypeCacheTypCallback(). I think this is quite straightforward > optimization without negative side effects. I've revised comments, > commit message and did some code beautification. I'm going to push > this if no objections. > > 0003 (former 0001) > I've revised this patch. I think main concerns expressed in the > thread about this path is that we don't have invalidation mechanism > for relid => typid map. Finally due to oid wraparound same relids > could get reused. That could lead to invalid entries in the map about > existing relids and typeids. This is rather messy, but I don't think > this could cause a material bug. The maps items are used only for > cache invalidation. Extra invalidation doesn't cause a bug. If type > with same relid will be cached, then correspoding map item will be > overridden, so no missing invalidation. However, I see the following > reasons for keeping consistent state of relid => typid map. > > 1) As the main use-case for this optimization is flood of temporary > tables, it would be nice not let relid => typid map bloat in this > case. I see that TypeCacheHash would get bloated, because its entries > are never deleted. However, I would prefer to not get this situation > even worse. > 2) In future we may find some more use-cases for relid => typid map > besides cache invalidation. Keeping that in consistent state could be > advantage then. > > In the attached patch, I'm keeping relid => typid map when > corresponding typentry have either TCFLAGS_HAVE_PG_TYPE_DATA, or > TCFLAGS_OPERATOR_FLAGS, or tupdesc. Thus, when temporary table gets > deleted, we would invalidate the map item. > > It will be also nice to get rid of iteration over all the cached > domain types in TypeCacheRelCallback(). However, this typically > shouldn't be a problem since domain types are less tended to bloat. > Domain types are created manually, unlike composite types which are > automatically created for every temporary table. We will probably > need to optimize this in future, but I don't feel this to be necessary > in present patch. > > I think the revised 0003 requires review. The rebased remaining patch is attached. ------ Regards, Alexander Korotkov Supabase Attachments: [application/octet-stream] v8-0001-Avoid-looping-over-all-type-cache-entries-in-Type.patch (14.7K, ../../CAPpHfdtaGKWoOtf7xAegjFbeu0We_eUkrh0Sg6BrCe0UUm3REw@mail.gmail.com/2-v8-0001-Avoid-looping-over-all-type-cache-entries-in-Type.patch) download | inline diff: From c2be3e3d32cd784841e3ff4355176bd7572220d1 Mon Sep 17 00:00:00 2001 From: Alexander Korotkov <[email protected]> Date: Mon, 5 Aug 2024 00:20:24 +0300 Subject: [PATCH v8] Avoid looping over all type cache entries in TypeCacheRelCallback() Currently when a single relcache entry gets invalidated, TypeCacheRelCallback() has to loop over all type cache entries to find appropriate typentry to invalidate. Unfortunately, using the syscache here is impossible, because this callback could be called outside a transaction and this makes impossible catalog lookups. This is why present commit introduces RelIdToTypeIdCacheHash to map relation oid to its composite type oid. We are keeping RelIdToTypeIdCacheHash entry while corresponding type cache entry have something to clean. Therefore, RelIdToTypeIdCacheHash shouldn't get bloat in the case of temporary tables flood. Discussion: https://postgr.es/m/5812a6e5-68ae-4d84-9d85-b443176966a1%40sigaev.ru Author: Teodor Sigaev Reviewed-by: Aleksander Alekseev, Tom Lane, Michael Paquier, Roman Zharkov Reviewed-by: Andrei Lepikhov --- src/backend/utils/cache/typcache.c | 295 ++++++++++++++++++++++++----- src/tools/pgindent/typedefs.list | 1 + 2 files changed, 251 insertions(+), 45 deletions(-) diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c index 0b9e60845b2..472df866512 100644 --- a/src/backend/utils/cache/typcache.c +++ b/src/backend/utils/cache/typcache.c @@ -77,6 +77,20 @@ /* The main type cache hashtable searched by lookup_type_cache */ static HTAB *TypeCacheHash = NULL; +/* + * The map from relation's oid to the corresponding composite type oid. We're + * keeping the map entry when corresponding typentry have either + * TCFLAGS_HAVE_PG_TYPE_DATA, or TCFLAGS_OPERATOR_FLAGS, or tupdesc. That is + * we're keeping map entry if the entry has something to clear. + */ +static HTAB *RelIdToTypeIdCacheHash = NULL; + +typedef struct RelIdToTypeIdCacheEntry +{ + Oid relid; /* OID of the relation */ + Oid composite_typid; /* OID of the relation's composite type */ +} RelIdToTypeIdCacheEntry; + /* List of type cache entries for domain types */ static TypeCacheEntry *firstDomainTypeEntry = NULL; @@ -329,7 +343,8 @@ static void shared_record_typmod_registry_detach(dsm_segment *segment, static TupleDesc find_or_make_matching_shared_tupledesc(TupleDesc tupdesc); static dsa_pointer share_tupledesc(dsa_area *area, TupleDesc tupdesc, uint32 typmod); - +static void check_insert_rel_type_cache(TypeCacheEntry *typentry); +static void check_delete_rel_type_cache(TypeCacheEntry *typentry); /* * Hash function compatible with one-arg system cache hash function. @@ -376,6 +391,13 @@ lookup_type_cache(Oid type_id, int flags) TypeCacheHash = hash_create("Type information cache", 64, &ctl, HASH_ELEM | HASH_FUNCTION); + Assert(RelIdToTypeIdCacheHash == NULL); + + ctl.keysize = sizeof(Oid); + ctl.entrysize = sizeof(RelIdToTypeIdCacheEntry); + RelIdToTypeIdCacheHash = hash_create("Map from relid to oid of cached composite type", 64, + &ctl, HASH_ELEM | HASH_BLOBS); + /* Also set up callbacks for SI invalidations */ CacheRegisterRelcacheCallback(TypeCacheRelCallback, (Datum) 0); CacheRegisterSyscacheCallback(TYPEOID, TypeCacheTypCallback, (Datum) 0); @@ -387,6 +409,8 @@ lookup_type_cache(Oid type_id, int flags) CreateCacheMemoryContext(); } + Assert(TypeCacheHash != NULL && RelIdToTypeIdCacheHash != NULL); + /* Try to look up an existing entry */ typentry = (TypeCacheEntry *) hash_search(TypeCacheHash, &type_id, @@ -438,6 +462,7 @@ lookup_type_cache(Oid type_id, int flags) typentry->typelem = typtup->typelem; typentry->typcollation = typtup->typcollation; typentry->flags |= TCFLAGS_HAVE_PG_TYPE_DATA; + check_insert_rel_type_cache(typentry); /* If it's a domain, immediately thread it into the domain cache list */ if (typentry->typtype == TYPTYPE_DOMAIN) @@ -483,6 +508,7 @@ lookup_type_cache(Oid type_id, int flags) typentry->typelem = typtup->typelem; typentry->typcollation = typtup->typcollation; typentry->flags |= TCFLAGS_HAVE_PG_TYPE_DATA; + check_insert_rel_type_cache(typentry); ReleaseSysCache(tp); } @@ -2281,6 +2307,53 @@ SharedRecordTypmodRegistryAttach(SharedRecordTypmodRegistry *registry) CurrentSession->shared_typmod_table = typmod_table; } +/* + * InvalidateCompositeTypeCacheEntry + * Invalidate particular TypeCacheEntry on Relcache inval callback + * + * Delete the cached tuple descriptor (if any) for the given composite + * type, and reset whatever info we have cached about the composite type's + * comparability. + */ +static void +InvalidateCompositeTypeCacheEntry(TypeCacheEntry *typentry) +{ + bool hadTupDescOrOpclass; + + Assert(typentry->typtype == TYPTYPE_COMPOSITE && + OidIsValid(typentry->typrelid)); + + hadTupDescOrOpclass = (typentry->tupDesc != NULL) || + (typentry->flags & TCFLAGS_OPERATOR_FLAGS); + + /* Delete tupdesc if we have it */ + if (typentry->tupDesc != NULL) + { + /* + * Release our refcount and free the tupdesc if none remain. We can't + * use DecrTupleDescRefCount here because this reference is not logged + * by the current resource owner. + */ + Assert(typentry->tupDesc->tdrefcount > 0); + if (--typentry->tupDesc->tdrefcount == 0) + FreeTupleDesc(typentry->tupDesc); + typentry->tupDesc = NULL; + + /* + * Also clear tupDesc_identifier, so that anyone watching it will + * realize that the tupdesc has changed. + */ + typentry->tupDesc_identifier = 0; + } + + /* Reset equality/comparison/hashing validity information */ + typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; + + /* Call check_delete_rel_type_cache() if we actually cleared something */ + if (hadTupDescOrOpclass) + check_delete_rel_type_cache(typentry); +} + /* * TypeCacheRelCallback * Relcache inval callback function @@ -2290,63 +2363,55 @@ SharedRecordTypmodRegistryAttach(SharedRecordTypmodRegistry *registry) * whatever info we have cached about the composite type's comparability. * * This is called when a relcache invalidation event occurs for the given - * relid. We must scan the whole typcache hash since we don't know the - * type OID corresponding to the relid. We could do a direct search if this - * were a syscache-flush callback on pg_type, but then we would need all - * ALTER-TABLE-like commands that could modify a rowtype to issue syscache - * invals against the rel's pg_type OID. The extra SI signaling could very - * well cost more than we'd save, since in most usages there are not very - * many entries in a backend's typcache. The risk of bugs-of-omission seems - * high, too. - * - * Another possibility, with only localized impact, is to maintain a second - * hashtable that indexes composite-type typcache entries by their typrelid. - * But it's still not clear it's worth the trouble. + * relid. We can't use syscache to find a type corresponding to the given + * relation because the code can be called outside of transaction. Thus we use + * the RelIdToTypeIdCacheHash map to locate appropriate typcache entry. */ static void TypeCacheRelCallback(Datum arg, Oid relid) { - HASH_SEQ_STATUS status; TypeCacheEntry *typentry; - /* TypeCacheHash must exist, else this callback wouldn't be registered */ - hash_seq_init(&status, TypeCacheHash); - while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) + /* + * RelIdToTypeIdCacheHash and TypeCacheHash should exist, otherwise this + * callback wouldn't be registered + */ + if (OidIsValid(relid)) { - if (typentry->typtype == TYPTYPE_COMPOSITE) + RelIdToTypeIdCacheEntry *relentry; + + /* + * Find an RelIdToTypeIdCacheHash entry, which should exist as soon as + * corresponding typcache entry has something to clean. + */ + relentry = (RelIdToTypeIdCacheEntry *) hash_search(RelIdToTypeIdCacheHash, + &relid, + HASH_FIND, NULL); + + if (relentry != NULL) { - /* Skip if no match, unless we're zapping all composite types */ - if (relid != typentry->typrelid && relid != InvalidOid) - continue; + typentry = (TypeCacheEntry *) hash_search(TypeCacheHash, + &relentry->composite_typid, + HASH_FIND, NULL); - /* Delete tupdesc if we have it */ - if (typentry->tupDesc != NULL) + if (typentry != NULL) { - /* - * Release our refcount, and free the tupdesc if none remain. - * (Can't use DecrTupleDescRefCount because this reference is - * not logged in current resource owner.) - */ - Assert(typentry->tupDesc->tdrefcount > 0); - if (--typentry->tupDesc->tdrefcount == 0) - FreeTupleDesc(typentry->tupDesc); - typentry->tupDesc = NULL; + Assert(typentry->typtype == TYPTYPE_COMPOSITE); + Assert(relid == typentry->typrelid); - /* - * Also clear tupDesc_identifier, so that anything watching - * that will realize that the tupdesc has possibly changed. - * (Alternatively, we could specify that to detect possible - * tupdesc change, one must check for tupDesc != NULL as well - * as tupDesc_identifier being the same as what was previously - * seen. That seems error-prone.) - */ - typentry->tupDesc_identifier = 0; + InvalidateCompositeTypeCacheEntry(typentry); } - - /* Reset equality/comparison/hashing validity information */ - typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; } - else if (typentry->typtype == TYPTYPE_DOMAIN) + + /* + * Visit all the domain types sequentially. Typically shouldn't be a + * problem since domain types are less tended to bloat. Domain types + * are created manually, unlike composite types which are + * automatically created for every temporary table. + */ + for (typentry = firstDomainTypeEntry; + typentry != NULL; + typentry = typentry->nextDomain) { /* * If it's domain over composite, reset flags. (We don't bother @@ -2358,6 +2423,36 @@ TypeCacheRelCallback(Datum arg, Oid relid) typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; } } + else + { + HASH_SEQ_STATUS status; + + /* + * Relid = 0, so we need to reset all composite types in cache. Also, + * we should reset flags for domain types, and we loop over all + * entries in hash, so, do it in a single scan. + */ + hash_seq_init(&status, TypeCacheHash); + while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) + { + if (typentry->typtype == TYPTYPE_COMPOSITE) + { + InvalidateCompositeTypeCacheEntry(typentry); + } + else if (typentry->typtype == TYPTYPE_DOMAIN) + { + /* + * If it's domain over composite, reset flags. (We don't + * bother trying to determine whether the specific base type + * needs a reset.) Note that if we haven't determined whether + * the base type is composite, we don't need to reset + * anything. + */ + if (typentry->flags & TCFLAGS_DOMAIN_BASE_IS_COMPOSITE) + typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; + } + } + } } /* @@ -2388,6 +2483,8 @@ TypeCacheTypCallback(Datum arg, int cacheid, uint32 hashvalue) while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) { + bool hadPgTypeData = (typentry->flags & TCFLAGS_HAVE_PG_TYPE_DATA); + Assert(hashvalue == 0 || typentry->type_id_hash == hashvalue); /* @@ -2397,6 +2494,10 @@ TypeCacheTypCallback(Datum arg, int cacheid, uint32 hashvalue) */ typentry->flags &= ~(TCFLAGS_HAVE_PG_TYPE_DATA | TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS); + + /* Call check_delete_rel_type_cache() if cleaned TCFLAGS_HAVE_PG_TYPE_DATA flag. */ + if (hadPgTypeData) + check_delete_rel_type_cache(typentry); } } @@ -2905,3 +3006,107 @@ shared_record_typmod_registry_detach(dsm_segment *segment, Datum datum) } CurrentSession->shared_typmod_registry = NULL; } + +/* + * Insert RelIdToTypeIdCacheHash entry after setting of the + * TCFLAGS_HAVE_PG_TYPE_DATA flag if needed. + */ +static void +check_insert_rel_type_cache(TypeCacheEntry *typentry) +{ + Assert(typentry->flags & TCFLAGS_HAVE_PG_TYPE_DATA); + + /* Immediately quit for non-composite types */ + if (typentry->typtype != TYPTYPE_COMPOSITE) + return; + + /* typrelid should be given for composite types */ + Assert(OidIsValid(typentry->typrelid)); + + /* + * Insert a RelIdToTypeIdCacheHash entry if the typentry doesn't have any + * information indicating there should be such an entry already. + */ + if (!(typentry->flags & TCFLAGS_OPERATOR_FLAGS) && + typentry->tupDesc == NULL) + { + RelIdToTypeIdCacheEntry *relentry; + bool found; + + relentry = (RelIdToTypeIdCacheEntry *) hash_search(RelIdToTypeIdCacheHash, + &typentry->typrelid, + HASH_ENTER, &found); + Assert(!found); + relentry->relid = typentry->typrelid; + relentry->composite_typid = typentry->type_id; + } + +#ifdef USE_ASSERT_CHECKING + + /* + * In assert-enabled builds otherwise check for RelIdToTypeIdCacheHash + * entry if it should exist. + */ + if (!(typentry->flags & TCFLAGS_OPERATOR_FLAGS) && + typentry->tupDesc == NULL) + { + bool found; + + (void) hash_search(RelIdToTypeIdCacheHash, + &typentry->typrelid, + HASH_FIND, &found); + Assert(found); + } +#endif +} + +/* + * Delete entry RelIdToTypeIdCacheHash if needed after resetting of the + * TCFLAGS_HAVE_PG_TYPE_DATA flag, or any of TCFLAGS_OPERATOR_FLAGS flags, + * or tupDesc if needed. + */ +static void +check_delete_rel_type_cache(TypeCacheEntry *typentry) +{ + /* Immediately quit for non-composite types */ + if (typentry->typtype != TYPTYPE_COMPOSITE) + return; + + /* typrelid should be given for composite types */ + Assert(OidIsValid(typentry->typrelid)); + + /* + * Delete a RelIdToTypeIdCacheHash entry if the typentry doesn't have any + * information indicating entry should be still there. + */ + if (!(typentry->flags & TCFLAGS_HAVE_PG_TYPE_DATA) && + !(typentry->flags & TCFLAGS_OPERATOR_FLAGS) && + typentry->tupDesc == NULL) + { + bool found; + + (void) hash_search(RelIdToTypeIdCacheHash, + &typentry->typrelid, + HASH_REMOVE, &found); + Assert(found); + } + +#ifdef USE_ASSERT_CHECKING + + /* + * In assert-enabled builds otherwise check for RelIdToTypeIdCacheHash + * entry if it should exist. + */ + if ((typentry->flags & TCFLAGS_HAVE_PG_TYPE_DATA) || + (typentry->flags & TCFLAGS_OPERATOR_FLAGS) || + typentry->tupDesc != NULL) + { + bool found; + + (void) hash_search(RelIdToTypeIdCacheHash, + &typentry->typrelid, + HASH_FIND, &found); + Assert(found); + } +#endif +} diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 547d14b3e7c..09ad8f69223 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2375,6 +2375,7 @@ RelFileLocator RelFileLocatorBackend RelFileNumber RelIdCacheEnt +RelIdToTypeIdCacheEntry RelInfo RelInfoArr RelMapFile -- 2.39.3 (Apple Git-146) ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-08-21 13:28 Pavel Borisov <[email protected]> parent: Alexander Korotkov <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Pavel Borisov @ 2024-08-21 13:28 UTC (permalink / raw) To: Alexander Korotkov <[email protected]>; +Cc: Andrei Lepikhov <[email protected]>; Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]> Hi, Alexander! On Tue, 20 Aug 2024 at 23:01, Alexander Korotkov <[email protected]> wrote: > On Mon, Aug 5, 2024 at 4:16 AM Alexander Korotkov <[email protected]> > wrote: > > I've revised the patchset. First of all, I've re-ordered the patches. > > > > 0001-0002 (former 0002-0003) > > Comprises hash_search_with_hash_value() function and its application > > to avoid full hash iteration in InvalidateAttoptCacheCallback() and > > TypeCacheTypCallback(). I think this is quite straightforward > > optimization without negative side effects. I've revised comments, > > commit message and did some code beautification. I'm going to push > > this if no objections. > > > > 0003 (former 0001) > > I've revised this patch. I think main concerns expressed in the > > thread about this path is that we don't have invalidation mechanism > > for relid => typid map. Finally due to oid wraparound same relids > > could get reused. That could lead to invalid entries in the map about > > existing relids and typeids. This is rather messy, but I don't think > > this could cause a material bug. The maps items are used only for > > cache invalidation. Extra invalidation doesn't cause a bug. If type > > with same relid will be cached, then correspoding map item will be > > overridden, so no missing invalidation. However, I see the following > > reasons for keeping consistent state of relid => typid map. > > > > 1) As the main use-case for this optimization is flood of temporary > > tables, it would be nice not let relid => typid map bloat in this > > case. I see that TypeCacheHash would get bloated, because its entries > > are never deleted. However, I would prefer to not get this situation > > even worse. > > 2) In future we may find some more use-cases for relid => typid map > > besides cache invalidation. Keeping that in consistent state could be > > advantage then. > > > > In the attached patch, I'm keeping relid => typid map when > > corresponding typentry have either TCFLAGS_HAVE_PG_TYPE_DATA, or > > TCFLAGS_OPERATOR_FLAGS, or tupdesc. Thus, when temporary table gets > > deleted, we would invalidate the map item. > > > > It will be also nice to get rid of iteration over all the cached > > domain types in TypeCacheRelCallback(). However, this typically > > shouldn't be a problem since domain types are less tended to bloat. > > Domain types are created manually, unlike composite types which are > > automatically created for every temporary table. We will probably > > need to optimize this in future, but I don't feel this to be necessary > > in present patch. > > > > I think the revised 0003 requires review. > > The rebased remaining patch is attached. > I've looked at patch v8. 1. In function check_insert_rel_type_cache() the block: +#ifdef USE_ASSERT_CHECKING + + /* + * In assert-enabled builds otherwise check for RelIdToTypeIdCacheHash + * entry if it should exist. + */ + if (!(typentry->flags & TCFLAGS_OPERATOR_FLAGS) && + typentry->tupDesc == NULL) + { + bool found; + + (void) hash_search(RelIdToTypeIdCacheHash, + &typentry->typrelid, + HASH_FIND, &found); + Assert(found); + } +#endif As I understand it does HASH_FIND after the same value just inserted by HASH_ENT ER above under the same if condition: if (!(typentry->flags & TCFLAGS_OPERATOR_FLAGS) && + typentry->tupDesc == NULL) Why do we need to do this re-check HASH_ENTER? Also I see "otherwise" in comment in a quoted block, but if condition is the same. 2. For function check_delete_rel_type_cache(): I'd modify the block: +#ifdef USE_ASSERT_CHECKING + + /* + * In assert-enabled builds otherwise check for RelIdToTypeIdCacheHash + * entry if it should exist. + */ + if ((typentry->flags & TCFLAGS_HAVE_PG_TYPE_DATA) || + (typentry->flags & TCFLAGS_OPERATOR_FLAGS) || + typentry->tupDesc != NULL) + { + bool found; + + (void) hash_search(RelIdToTypeIdCacheHash, + &typentry->typrelid, + HASH_FIND, &found); + Assert(found); + } +#endif as: + + /* + * In assert-enabled builds otherwise check for RelIdToTypeIdCacheHash + * entry if it should exist. + */ + else +{ + #ifdef USE_ASSERT_CHECKING + bool found; + + (void) hash_search(RelIdToTypeIdCacheHash, + &typentry->typrelid, + HASH_FIND, &found); + Assert(found); +#endif +} 3. I think check_delete_rel_type_cache and check_insert_rel_type_cache are better to be renamed to be more clear, though I don't have exact proposals yet, 4. I haven't looked into comments, though I'd recommend oid -> OID replacement in the comments. Thank you for working on this patchset! Regards, Pavel Borisov Supabase ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-08-21 15:28 Alexander Korotkov <[email protected]> parent: Pavel Borisov <[email protected]> 0 siblings, 2 replies; 28+ messages in thread From: Alexander Korotkov @ 2024-08-21 15:28 UTC (permalink / raw) To: Pavel Borisov <[email protected]>; +Cc: Andrei Lepikhov <[email protected]>; Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]> Hi, Pavel! On Wed, Aug 21, 2024 at 4:28 PM Pavel Borisov <[email protected]> wrote: > I've looked at patch v8. > > 1. > In function check_insert_rel_type_cache() the block: > > +#ifdef USE_ASSERT_CHECKING > + > + /* > + * In assert-enabled builds otherwise check for RelIdToTypeIdCacheHash > + * entry if it should exist. > + */ > + if (!(typentry->flags & TCFLAGS_OPERATOR_FLAGS) && > + typentry->tupDesc == NULL) > + { > + bool found; > + > + (void) hash_search(RelIdToTypeIdCacheHash, > + &typentry->typrelid, > + HASH_FIND, &found); > + Assert(found); > + } > +#endif > > As I understand it does HASH_FIND after the same value just inserted by HASH_ENT > ER above under the same if condition: > > if (!(typentry->flags & TCFLAGS_OPERATOR_FLAGS) && > + typentry->tupDesc == NULL) > > Why do we need to do this re-check HASH_ENTER? Also I see "otherwise" in comment in a quoted block, but if condition is the same. Yep, these are remains from one of my previous attempt. No sense to check for HASH_FIND right after HASH_ENTER. Removed. > 2. > For function check_delete_rel_type_cache(): > I'd modify the block: > +#ifdef USE_ASSERT_CHECKING > + > + /* > + * In assert-enabled builds otherwise check for RelIdToTypeIdCacheHash > + * entry if it should exist. > + */ > + if ((typentry->flags & TCFLAGS_HAVE_PG_TYPE_DATA) || > + (typentry->flags & TCFLAGS_OPERATOR_FLAGS) || > + typentry->tupDesc != NULL) > + { > + bool found; > + > + (void) hash_search(RelIdToTypeIdCacheHash, > + &typentry->typrelid, > + HASH_FIND, &found); > + Assert(found); > + } > +#endif > > as: > + > + /* > + * In assert-enabled builds otherwise check for RelIdToTypeIdCacheHash > + * entry if it should exist. > + */ > + else > +{ > + #ifdef USE_ASSERT_CHECKING > + bool found; > + > + (void) hash_search(RelIdToTypeIdCacheHash, > + &typentry->typrelid, > + HASH_FIND, &found); > + Assert(found); > +#endif > +} Changed in the way you proposed, except I put the comment inside the #ifdef. I this it's easier to understand this way. > 3. I think check_delete_rel_type_cache and check_insert_rel_type_cache are better to be renamed to be more clear, though I don't have exact proposals yet, Renamed to delete_rel_type_cache_if_needed and insert_rel_type_cache_if_needed. I've checked that > 4. I haven't looked into comments, though I'd recommend oid -> OID replacement in the comments. I've changed oid -> OID in the comments and in the commit message. > Thank you for working on this patchset! Thank you for review! ------ Regards, Alexander Korotkov Supabase Attachments: [application/octet-stream] v9-0001-Avoid-looping-over-all-type-cache-entries-in-Type.patch (14.2K, ../../CAPpHfduEvD3ebSsQs80j10F_PS4FbukqXbPrhda=9KTk7X=LgA@mail.gmail.com/2-v9-0001-Avoid-looping-over-all-type-cache-entries-in-Type.patch) download | inline diff: From dc58a31f59a9c5b7a6752e5633f252a650343e2b Mon Sep 17 00:00:00 2001 From: Alexander Korotkov <[email protected]> Date: Mon, 5 Aug 2024 00:20:24 +0300 Subject: [PATCH v9] Avoid looping over all type cache entries in TypeCacheRelCallback() Currently when a single relcache entry gets invalidated, TypeCacheRelCallback() has to loop over all type cache entries to find appropriate typentry to invalidate. Unfortunately, using the syscache here is impossible, because this callback could be called outside a transaction and this makes impossible catalog lookups. This is why present commit introduces RelIdToTypeIdCacheHash to map relation OID to its composite type OID. We are keeping RelIdToTypeIdCacheHash entry while corresponding type cache entry have something to clean. Therefore, RelIdToTypeIdCacheHash shouldn't get bloat in the case of temporary tables flood. Discussion: https://postgr.es/m/5812a6e5-68ae-4d84-9d85-b443176966a1%40sigaev.ru Author: Teodor Sigaev Reviewed-by: Aleksander Alekseev, Tom Lane, Michael Paquier, Roman Zharkov Reviewed-by: Andrei Lepikhov --- src/backend/utils/cache/typcache.c | 273 ++++++++++++++++++++++++----- src/tools/pgindent/typedefs.list | 1 + 2 files changed, 229 insertions(+), 45 deletions(-) diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c index 0b9e60845b2..c883851e66c 100644 --- a/src/backend/utils/cache/typcache.c +++ b/src/backend/utils/cache/typcache.c @@ -77,6 +77,20 @@ /* The main type cache hashtable searched by lookup_type_cache */ static HTAB *TypeCacheHash = NULL; +/* + * The map from relation's OID to the corresponding composite type OID. We're + * keeping the map entry when corresponding typentry have either + * TCFLAGS_HAVE_PG_TYPE_DATA, or TCFLAGS_OPERATOR_FLAGS, or tupdesc. That is + * we're keeping map entry if the entry has something to clear. + */ +static HTAB *RelIdToTypeIdCacheHash = NULL; + +typedef struct RelIdToTypeIdCacheEntry +{ + Oid relid; /* OID of the relation */ + Oid composite_typid; /* OID of the relation's composite type */ +} RelIdToTypeIdCacheEntry; + /* List of type cache entries for domain types */ static TypeCacheEntry *firstDomainTypeEntry = NULL; @@ -329,7 +343,8 @@ static void shared_record_typmod_registry_detach(dsm_segment *segment, static TupleDesc find_or_make_matching_shared_tupledesc(TupleDesc tupdesc); static dsa_pointer share_tupledesc(dsa_area *area, TupleDesc tupdesc, uint32 typmod); - +static void insert_rel_type_cache_if_needed(TypeCacheEntry *typentry); +static void delete_rel_type_cache_if_needed(TypeCacheEntry *typentry); /* * Hash function compatible with one-arg system cache hash function. @@ -376,6 +391,13 @@ lookup_type_cache(Oid type_id, int flags) TypeCacheHash = hash_create("Type information cache", 64, &ctl, HASH_ELEM | HASH_FUNCTION); + Assert(RelIdToTypeIdCacheHash == NULL); + + ctl.keysize = sizeof(Oid); + ctl.entrysize = sizeof(RelIdToTypeIdCacheEntry); + RelIdToTypeIdCacheHash = hash_create("Map from relid to OID of cached composite type", 64, + &ctl, HASH_ELEM | HASH_BLOBS); + /* Also set up callbacks for SI invalidations */ CacheRegisterRelcacheCallback(TypeCacheRelCallback, (Datum) 0); CacheRegisterSyscacheCallback(TYPEOID, TypeCacheTypCallback, (Datum) 0); @@ -387,6 +409,8 @@ lookup_type_cache(Oid type_id, int flags) CreateCacheMemoryContext(); } + Assert(TypeCacheHash != NULL && RelIdToTypeIdCacheHash != NULL); + /* Try to look up an existing entry */ typentry = (TypeCacheEntry *) hash_search(TypeCacheHash, &type_id, @@ -438,6 +462,7 @@ lookup_type_cache(Oid type_id, int flags) typentry->typelem = typtup->typelem; typentry->typcollation = typtup->typcollation; typentry->flags |= TCFLAGS_HAVE_PG_TYPE_DATA; + insert_rel_type_cache_if_needed(typentry); /* If it's a domain, immediately thread it into the domain cache list */ if (typentry->typtype == TYPTYPE_DOMAIN) @@ -483,6 +508,7 @@ lookup_type_cache(Oid type_id, int flags) typentry->typelem = typtup->typelem; typentry->typcollation = typtup->typcollation; typentry->flags |= TCFLAGS_HAVE_PG_TYPE_DATA; + insert_rel_type_cache_if_needed(typentry); ReleaseSysCache(tp); } @@ -2281,6 +2307,53 @@ SharedRecordTypmodRegistryAttach(SharedRecordTypmodRegistry *registry) CurrentSession->shared_typmod_table = typmod_table; } +/* + * InvalidateCompositeTypeCacheEntry + * Invalidate particular TypeCacheEntry on Relcache inval callback + * + * Delete the cached tuple descriptor (if any) for the given composite + * type, and reset whatever info we have cached about the composite type's + * comparability. + */ +static void +InvalidateCompositeTypeCacheEntry(TypeCacheEntry *typentry) +{ + bool hadTupDescOrOpclass; + + Assert(typentry->typtype == TYPTYPE_COMPOSITE && + OidIsValid(typentry->typrelid)); + + hadTupDescOrOpclass = (typentry->tupDesc != NULL) || + (typentry->flags & TCFLAGS_OPERATOR_FLAGS); + + /* Delete tupdesc if we have it */ + if (typentry->tupDesc != NULL) + { + /* + * Release our refcount and free the tupdesc if none remain. We can't + * use DecrTupleDescRefCount here because this reference is not logged + * by the current resource owner. + */ + Assert(typentry->tupDesc->tdrefcount > 0); + if (--typentry->tupDesc->tdrefcount == 0) + FreeTupleDesc(typentry->tupDesc); + typentry->tupDesc = NULL; + + /* + * Also clear tupDesc_identifier, so that anyone watching it will + * realize that the tupdesc has changed. + */ + typentry->tupDesc_identifier = 0; + } + + /* Reset equality/comparison/hashing validity information */ + typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; + + /* Call check_delete_rel_type_cache() if we actually cleared something */ + if (hadTupDescOrOpclass) + delete_rel_type_cache_if_needed(typentry); +} + /* * TypeCacheRelCallback * Relcache inval callback function @@ -2290,63 +2363,55 @@ SharedRecordTypmodRegistryAttach(SharedRecordTypmodRegistry *registry) * whatever info we have cached about the composite type's comparability. * * This is called when a relcache invalidation event occurs for the given - * relid. We must scan the whole typcache hash since we don't know the - * type OID corresponding to the relid. We could do a direct search if this - * were a syscache-flush callback on pg_type, but then we would need all - * ALTER-TABLE-like commands that could modify a rowtype to issue syscache - * invals against the rel's pg_type OID. The extra SI signaling could very - * well cost more than we'd save, since in most usages there are not very - * many entries in a backend's typcache. The risk of bugs-of-omission seems - * high, too. - * - * Another possibility, with only localized impact, is to maintain a second - * hashtable that indexes composite-type typcache entries by their typrelid. - * But it's still not clear it's worth the trouble. + * relid. We can't use syscache to find a type corresponding to the given + * relation because the code can be called outside of transaction. Thus we use + * the RelIdToTypeIdCacheHash map to locate appropriate typcache entry. */ static void TypeCacheRelCallback(Datum arg, Oid relid) { - HASH_SEQ_STATUS status; TypeCacheEntry *typentry; - /* TypeCacheHash must exist, else this callback wouldn't be registered */ - hash_seq_init(&status, TypeCacheHash); - while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) + /* + * RelIdToTypeIdCacheHash and TypeCacheHash should exist, otherwise this + * callback wouldn't be registered + */ + if (OidIsValid(relid)) { - if (typentry->typtype == TYPTYPE_COMPOSITE) + RelIdToTypeIdCacheEntry *relentry; + + /* + * Find an RelIdToTypeIdCacheHash entry, which should exist as soon as + * corresponding typcache entry has something to clean. + */ + relentry = (RelIdToTypeIdCacheEntry *) hash_search(RelIdToTypeIdCacheHash, + &relid, + HASH_FIND, NULL); + + if (relentry != NULL) { - /* Skip if no match, unless we're zapping all composite types */ - if (relid != typentry->typrelid && relid != InvalidOid) - continue; + typentry = (TypeCacheEntry *) hash_search(TypeCacheHash, + &relentry->composite_typid, + HASH_FIND, NULL); - /* Delete tupdesc if we have it */ - if (typentry->tupDesc != NULL) + if (typentry != NULL) { - /* - * Release our refcount, and free the tupdesc if none remain. - * (Can't use DecrTupleDescRefCount because this reference is - * not logged in current resource owner.) - */ - Assert(typentry->tupDesc->tdrefcount > 0); - if (--typentry->tupDesc->tdrefcount == 0) - FreeTupleDesc(typentry->tupDesc); - typentry->tupDesc = NULL; + Assert(typentry->typtype == TYPTYPE_COMPOSITE); + Assert(relid == typentry->typrelid); - /* - * Also clear tupDesc_identifier, so that anything watching - * that will realize that the tupdesc has possibly changed. - * (Alternatively, we could specify that to detect possible - * tupdesc change, one must check for tupDesc != NULL as well - * as tupDesc_identifier being the same as what was previously - * seen. That seems error-prone.) - */ - typentry->tupDesc_identifier = 0; + InvalidateCompositeTypeCacheEntry(typentry); } - - /* Reset equality/comparison/hashing validity information */ - typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; } - else if (typentry->typtype == TYPTYPE_DOMAIN) + + /* + * Visit all the domain types sequentially. Typically shouldn't be a + * problem since domain types are less tended to bloat. Domain types + * are created manually, unlike composite types which are + * automatically created for every temporary table. + */ + for (typentry = firstDomainTypeEntry; + typentry != NULL; + typentry = typentry->nextDomain) { /* * If it's domain over composite, reset flags. (We don't bother @@ -2358,6 +2423,36 @@ TypeCacheRelCallback(Datum arg, Oid relid) typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; } } + else + { + HASH_SEQ_STATUS status; + + /* + * Relid = 0, so we need to reset all composite types in cache. Also, + * we should reset flags for domain types, and we loop over all + * entries in hash, so, do it in a single scan. + */ + hash_seq_init(&status, TypeCacheHash); + while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) + { + if (typentry->typtype == TYPTYPE_COMPOSITE) + { + InvalidateCompositeTypeCacheEntry(typentry); + } + else if (typentry->typtype == TYPTYPE_DOMAIN) + { + /* + * If it's domain over composite, reset flags. (We don't + * bother trying to determine whether the specific base type + * needs a reset.) Note that if we haven't determined whether + * the base type is composite, we don't need to reset + * anything. + */ + if (typentry->flags & TCFLAGS_DOMAIN_BASE_IS_COMPOSITE) + typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; + } + } + } } /* @@ -2388,6 +2483,8 @@ TypeCacheTypCallback(Datum arg, int cacheid, uint32 hashvalue) while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) { + bool hadPgTypeData = (typentry->flags & TCFLAGS_HAVE_PG_TYPE_DATA); + Assert(hashvalue == 0 || typentry->type_id_hash == hashvalue); /* @@ -2397,6 +2494,10 @@ TypeCacheTypCallback(Datum arg, int cacheid, uint32 hashvalue) */ typentry->flags &= ~(TCFLAGS_HAVE_PG_TYPE_DATA | TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS); + + /* Call check_delete_rel_type_cache() if cleaned TCFLAGS_HAVE_PG_TYPE_DATA flag. */ + if (hadPgTypeData) + delete_rel_type_cache_if_needed(typentry); } } @@ -2905,3 +3006,85 @@ shared_record_typmod_registry_detach(dsm_segment *segment, Datum datum) } CurrentSession->shared_typmod_registry = NULL; } + +/* + * Insert RelIdToTypeIdCacheHash entry after setting of the + * TCFLAGS_HAVE_PG_TYPE_DATA flag if needed. + */ +static void +insert_rel_type_cache_if_needed(TypeCacheEntry *typentry) +{ + Assert(typentry->flags & TCFLAGS_HAVE_PG_TYPE_DATA); + + /* Immediately quit for non-composite types */ + if (typentry->typtype != TYPTYPE_COMPOSITE) + return; + + /* typrelid should be given for composite types */ + Assert(OidIsValid(typentry->typrelid)); + + /* + * Insert a RelIdToTypeIdCacheHash entry if the typentry doesn't have any + * information indicating there should be such an entry already. + */ + if (!(typentry->flags & TCFLAGS_OPERATOR_FLAGS) && + typentry->tupDesc == NULL) + { + RelIdToTypeIdCacheEntry *relentry; + bool found; + + relentry = (RelIdToTypeIdCacheEntry *) hash_search(RelIdToTypeIdCacheHash, + &typentry->typrelid, + HASH_ENTER, &found); + Assert(!found); + relentry->relid = typentry->typrelid; + relentry->composite_typid = typentry->type_id; + } +} + +/* + * Delete entry RelIdToTypeIdCacheHash if needed after resetting of the + * TCFLAGS_HAVE_PG_TYPE_DATA flag, or any of TCFLAGS_OPERATOR_FLAGS flags, + * or tupDesc if needed. + */ +static void +delete_rel_type_cache_if_needed(TypeCacheEntry *typentry) +{ + /* Immediately quit for non-composite types */ + if (typentry->typtype != TYPTYPE_COMPOSITE) + return; + + /* typrelid should be given for composite types */ + Assert(OidIsValid(typentry->typrelid)); + + /* + * Delete a RelIdToTypeIdCacheHash entry if the typentry doesn't have any + * information indicating entry should be still there. + */ + if (!(typentry->flags & TCFLAGS_HAVE_PG_TYPE_DATA) && + !(typentry->flags & TCFLAGS_OPERATOR_FLAGS) && + typentry->tupDesc == NULL) + { + bool found; + + (void) hash_search(RelIdToTypeIdCacheHash, + &typentry->typrelid, + HASH_REMOVE, &found); + Assert(found); + } + else + { +#ifdef USE_ASSERT_CHECKING + /* + * In assert-enabled builds otherwise check for RelIdToTypeIdCacheHash + * entry if it should exist. + */ + bool found; + + (void) hash_search(RelIdToTypeIdCacheHash, + &typentry->typrelid, + HASH_FIND, &found); + Assert(found); +#endif + } +} diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 6d424c89186..0cdb33801da 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2376,6 +2376,7 @@ RelFileLocator RelFileLocatorBackend RelFileNumber RelIdCacheEnt +RelIdToTypeIdCacheEntry RelInfo RelInfoArr RelMapFile -- 2.39.3 (Apple Git-146) ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-08-22 10:02 Pavel Borisov <[email protected]> parent: Alexander Korotkov <[email protected]> 1 sibling, 1 reply; 28+ messages in thread From: Pavel Borisov @ 2024-08-22 10:02 UTC (permalink / raw) To: Alexander Korotkov <[email protected]>; +Cc: Andrei Lepikhov <[email protected]>; Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]> Hi, Alexander! On Wed, 21 Aug 2024 at 19:29, Alexander Korotkov <[email protected]> wrote: > Hi, Pavel! > > > On Wed, Aug 21, 2024 at 4:28 PM Pavel Borisov <[email protected]> > wrote: > > I've looked at patch v8. > > > > 1. > > In function check_insert_rel_type_cache() the block: > > > > +#ifdef USE_ASSERT_CHECKING > > + > > + /* > > + * In assert-enabled builds otherwise check for > RelIdToTypeIdCacheHash > > + * entry if it should exist. > > + */ > > + if (!(typentry->flags & TCFLAGS_OPERATOR_FLAGS) && > > + typentry->tupDesc == NULL) > > + { > > + bool found; > > + > > + (void) hash_search(RelIdToTypeIdCacheHash, > > + &typentry->typrelid, > > + HASH_FIND, &found); > > + Assert(found); > > + } > > +#endif > > > > As I understand it does HASH_FIND after the same value just inserted by > HASH_ENT > > ER above under the same if condition: > > > > if (!(typentry->flags & TCFLAGS_OPERATOR_FLAGS) && > > + typentry->tupDesc == NULL) > > > > Why do we need to do this re-check HASH_ENTER? Also I see "otherwise" in > comment in a quoted block, but if condition is the same. > > Yep, these are remains from one of my previous attempt. No sense to > check for HASH_FIND right after HASH_ENTER. Removed. > > > 2. > > For function check_delete_rel_type_cache(): > > I'd modify the block: > > +#ifdef USE_ASSERT_CHECKING > > + > > + /* > > + * In assert-enabled builds otherwise check for > RelIdToTypeIdCacheHash > > + * entry if it should exist. > > + */ > > + if ((typentry->flags & TCFLAGS_HAVE_PG_TYPE_DATA) || > > + (typentry->flags & TCFLAGS_OPERATOR_FLAGS) || > > + typentry->tupDesc != NULL) > > + { > > + bool found; > > + > > + (void) hash_search(RelIdToTypeIdCacheHash, > > + &typentry->typrelid, > > + HASH_FIND, &found); > > + Assert(found); > > + } > > +#endif > > > > as: > > + > > + /* > > + * In assert-enabled builds otherwise check for > RelIdToTypeIdCacheHash > > + * entry if it should exist. > > + */ > > + else > > +{ > > + #ifdef USE_ASSERT_CHECKING > > + bool found; > > + > > + (void) hash_search(RelIdToTypeIdCacheHash, > > + &typentry->typrelid, > > + HASH_FIND, &found); > > + Assert(found); > > +#endif > > +} > > Changed in the way you proposed, except I put the comment inside the > #ifdef. I this it's easier to understand this way. > > > 3. I think check_delete_rel_type_cache and check_insert_rel_type_cache > are better to be renamed to be more clear, though I don't have exact > proposals yet, > > Renamed to delete_rel_type_cache_if_needed and > insert_rel_type_cache_if_needed. I've checked that > > > 4. I haven't looked into comments, though I'd recommend oid -> OID > replacement in the comments. > > I've changed oid -> OID in the comments and in the commit message. > > > Thank you for working on this patchset! > > Thank you for review! > Looked at v9: Patch looks good to me. I'd only suggest comments changes: "The map from relation's OID to the corresponding composite type OID" -> "The mapping of relation's OID to the corresponding composite type OID" "We're keeping the map entry when corresponding typentry have either TCFLAGS_HAVE_PG_TYPE_DATA, or TCFLAGS_OPERATOR_FLAGS, or tupdesc. That is we're keeping map entry if the entry has something to clear." -> "We're keeping the map entry when the corresponding typentry has something to clear i.e it has either TCFLAGS_HAVE_PG_TYPE_DATA, or TCFLAGS_OPERATOR_FLAGS, or tupdesc." "Invalidate particular TypeCacheEntry on Relcache inval callback" - remove extra tabs before. Maybe also add empty line above. "Typically shouldn't be a problem" -> "Typically this shouldn't affect performance" "Relid = 0, so we need" -> "Relid is invalid. By convention we need" "if cleaned TCFLAGS_HAVE_PG_TYPE_DATA flag" -> "if we cleaned TCFLAGS_HAVE_PG_TYPE_DATA flag previously" "+/* + * Delete entry RelIdToTypeIdCacheHash if needed after resetting of the + * TCFLAGS_HAVE_PG_TYPE_DATA flag, or any of TCFLAGS_OPERATOR_FLAGS flags, + * or tupDesc if needed." - remove one "if needed" Regards, Pavel Borisov Supabase ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-08-22 12:02 Andrei Lepikhov <[email protected]> parent: Alexander Korotkov <[email protected]> 1 sibling, 0 replies; 28+ messages in thread From: Andrei Lepikhov @ 2024-08-22 12:02 UTC (permalink / raw) To: Alexander Korotkov <[email protected]>; Pavel Borisov <[email protected]>; +Cc: Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]> On 21/8/2024 17:28, Alexander Korotkov wrote: > > I've changed oid -> OID in the comments and in the commit message. I passed through the patch again: no objections and +1 to the changes of comments proposed by Pavel. -- regards, Andrei Lepikhov ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-08-22 16:52 Alexander Korotkov <[email protected]> parent: Pavel Borisov <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Alexander Korotkov @ 2024-08-22 16:52 UTC (permalink / raw) To: Pavel Borisov <[email protected]>; +Cc: Andrei Lepikhov <[email protected]>; Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]> Hi! On Thu, Aug 22, 2024 at 1:02 PM Pavel Borisov <[email protected]> wrote: Looked at v9: > Patch looks good to me. I'd only suggest comments changes: > > "The map from relation's OID to the corresponding composite type OID" -> "The mapping of relation's OID to the corresponding composite type OID" > "We're keeping the map entry when corresponding typentry have either TCFLAGS_HAVE_PG_TYPE_DATA, or TCFLAGS_OPERATOR_FLAGS, or tupdesc. That is we're keeping map entry if the entry has something to clear." -> "We're keeping the map entry when the corresponding typentry has something to clear i.e it has either TCFLAGS_HAVE_PG_TYPE_DATA, or TCFLAGS_OPERATOR_FLAGS, or tupdesc." > "Invalidate particular TypeCacheEntry on Relcache inval callback" - remove extra tabs before. Maybe also add empty line above. > "Typically shouldn't be a problem" -> "Typically this shouldn't affect performance" > "Relid = 0, so we need" -> "Relid is invalid. By convention we need" > "if cleaned TCFLAGS_HAVE_PG_TYPE_DATA flag" -> "if we cleaned TCFLAGS_HAVE_PG_TYPE_DATA flag previously" > "+/* > + * Delete entry RelIdToTypeIdCacheHash if needed after resetting of the > + * TCFLAGS_HAVE_PG_TYPE_DATA flag, or any of TCFLAGS_OPERATOR_FLAGS flags, > + * or tupDesc if needed." - remove one "if needed" Thank you for your feedback. I've integrated all your edits except the formatting change of InvalidateCompositeTypeCacheEntry() header comment. I think the functions below have the same formatting of header comments, and it's not necessary to change format. If no objections, I'm planning to push this after reverting PARTITION SPLIT/MERGE. ------ Regards, Alexander Korotkov Supabase Attachments: [application/octet-stream] v10-0001-Avoid-looping-over-all-type-cache-entries-in-Typ.patch (14.2K, ../../CAPpHfdvG-PzE5UN36my_4+FYv0n5rZJ-pN=z-gDCCk9ViPQ1Cw@mail.gmail.com/2-v10-0001-Avoid-looping-over-all-type-cache-entries-in-Typ.patch) download | inline diff: From d5ecbae3588f4c4ecec02ab3fd9251553d1cf0eb Mon Sep 17 00:00:00 2001 From: Alexander Korotkov <[email protected]> Date: Mon, 5 Aug 2024 00:20:24 +0300 Subject: [PATCH v10] Avoid looping over all type cache entries in TypeCacheRelCallback() Currently when a single relcache entry gets invalidated, TypeCacheRelCallback() has to loop over all type cache entries to find appropriate typentry to invalidate. Unfortunately, using the syscache here is impossible, because this callback could be called outside a transaction and this makes impossible catalog lookups. This is why present commit introduces RelIdToTypeIdCacheHash to map relation OID to its composite type OID. We are keeping RelIdToTypeIdCacheHash entry while corresponding type cache entry have something to clean. Therefore, RelIdToTypeIdCacheHash shouldn't get bloat in the case of temporary tables flood. Discussion: https://postgr.es/m/5812a6e5-68ae-4d84-9d85-b443176966a1%40sigaev.ru Author: Teodor Sigaev Reviewed-by: Aleksander Alekseev, Tom Lane, Michael Paquier, Roman Zharkov Reviewed-by: Andrei Lepikhov --- src/backend/utils/cache/typcache.c | 275 ++++++++++++++++++++++++----- src/tools/pgindent/typedefs.list | 1 + 2 files changed, 232 insertions(+), 44 deletions(-) diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c index 0b9e60845b2..494e5a41d79 100644 --- a/src/backend/utils/cache/typcache.c +++ b/src/backend/utils/cache/typcache.c @@ -77,6 +77,20 @@ /* The main type cache hashtable searched by lookup_type_cache */ static HTAB *TypeCacheHash = NULL; +/* + * The mapping of relation's OID to the corresponding composite type OID. + * We're keeping the map entry when the corresponding typentry has something + * to clear i.e it has either TCFLAGS_HAVE_PG_TYPE_DATA, or + * TCFLAGS_OPERATOR_FLAGS, or tupdesc. + */ +static HTAB *RelIdToTypeIdCacheHash = NULL; + +typedef struct RelIdToTypeIdCacheEntry +{ + Oid relid; /* OID of the relation */ + Oid composite_typid; /* OID of the relation's composite type */ +} RelIdToTypeIdCacheEntry; + /* List of type cache entries for domain types */ static TypeCacheEntry *firstDomainTypeEntry = NULL; @@ -329,6 +343,8 @@ static void shared_record_typmod_registry_detach(dsm_segment *segment, static TupleDesc find_or_make_matching_shared_tupledesc(TupleDesc tupdesc); static dsa_pointer share_tupledesc(dsa_area *area, TupleDesc tupdesc, uint32 typmod); +static void insert_rel_type_cache_if_needed(TypeCacheEntry *typentry); +static void delete_rel_type_cache_if_needed(TypeCacheEntry *typentry); /* @@ -376,6 +392,13 @@ lookup_type_cache(Oid type_id, int flags) TypeCacheHash = hash_create("Type information cache", 64, &ctl, HASH_ELEM | HASH_FUNCTION); + Assert(RelIdToTypeIdCacheHash == NULL); + + ctl.keysize = sizeof(Oid); + ctl.entrysize = sizeof(RelIdToTypeIdCacheEntry); + RelIdToTypeIdCacheHash = hash_create("Map from relid to OID of cached composite type", 64, + &ctl, HASH_ELEM | HASH_BLOBS); + /* Also set up callbacks for SI invalidations */ CacheRegisterRelcacheCallback(TypeCacheRelCallback, (Datum) 0); CacheRegisterSyscacheCallback(TYPEOID, TypeCacheTypCallback, (Datum) 0); @@ -387,6 +410,8 @@ lookup_type_cache(Oid type_id, int flags) CreateCacheMemoryContext(); } + Assert(TypeCacheHash != NULL && RelIdToTypeIdCacheHash != NULL); + /* Try to look up an existing entry */ typentry = (TypeCacheEntry *) hash_search(TypeCacheHash, &type_id, @@ -438,6 +463,7 @@ lookup_type_cache(Oid type_id, int flags) typentry->typelem = typtup->typelem; typentry->typcollation = typtup->typcollation; typentry->flags |= TCFLAGS_HAVE_PG_TYPE_DATA; + insert_rel_type_cache_if_needed(typentry); /* If it's a domain, immediately thread it into the domain cache list */ if (typentry->typtype == TYPTYPE_DOMAIN) @@ -483,6 +509,7 @@ lookup_type_cache(Oid type_id, int flags) typentry->typelem = typtup->typelem; typentry->typcollation = typtup->typcollation; typentry->flags |= TCFLAGS_HAVE_PG_TYPE_DATA; + insert_rel_type_cache_if_needed(typentry); ReleaseSysCache(tp); } @@ -2281,6 +2308,53 @@ SharedRecordTypmodRegistryAttach(SharedRecordTypmodRegistry *registry) CurrentSession->shared_typmod_table = typmod_table; } +/* + * InvalidateCompositeTypeCacheEntry + * Invalidate particular TypeCacheEntry on Relcache inval callback + * + * Delete the cached tuple descriptor (if any) for the given composite + * type, and reset whatever info we have cached about the composite type's + * comparability. + */ +static void +InvalidateCompositeTypeCacheEntry(TypeCacheEntry *typentry) +{ + bool hadTupDescOrOpclass; + + Assert(typentry->typtype == TYPTYPE_COMPOSITE && + OidIsValid(typentry->typrelid)); + + hadTupDescOrOpclass = (typentry->tupDesc != NULL) || + (typentry->flags & TCFLAGS_OPERATOR_FLAGS); + + /* Delete tupdesc if we have it */ + if (typentry->tupDesc != NULL) + { + /* + * Release our refcount and free the tupdesc if none remain. We can't + * use DecrTupleDescRefCount here because this reference is not logged + * by the current resource owner. + */ + Assert(typentry->tupDesc->tdrefcount > 0); + if (--typentry->tupDesc->tdrefcount == 0) + FreeTupleDesc(typentry->tupDesc); + typentry->tupDesc = NULL; + + /* + * Also clear tupDesc_identifier, so that anyone watching it will + * realize that the tupdesc has changed. + */ + typentry->tupDesc_identifier = 0; + } + + /* Reset equality/comparison/hashing validity information */ + typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; + + /* Call check_delete_rel_type_cache() if we actually cleared something */ + if (hadTupDescOrOpclass) + delete_rel_type_cache_if_needed(typentry); +} + /* * TypeCacheRelCallback * Relcache inval callback function @@ -2290,63 +2364,55 @@ SharedRecordTypmodRegistryAttach(SharedRecordTypmodRegistry *registry) * whatever info we have cached about the composite type's comparability. * * This is called when a relcache invalidation event occurs for the given - * relid. We must scan the whole typcache hash since we don't know the - * type OID corresponding to the relid. We could do a direct search if this - * were a syscache-flush callback on pg_type, but then we would need all - * ALTER-TABLE-like commands that could modify a rowtype to issue syscache - * invals against the rel's pg_type OID. The extra SI signaling could very - * well cost more than we'd save, since in most usages there are not very - * many entries in a backend's typcache. The risk of bugs-of-omission seems - * high, too. - * - * Another possibility, with only localized impact, is to maintain a second - * hashtable that indexes composite-type typcache entries by their typrelid. - * But it's still not clear it's worth the trouble. + * relid. We can't use syscache to find a type corresponding to the given + * relation because the code can be called outside of transaction. Thus we use + * the RelIdToTypeIdCacheHash map to locate appropriate typcache entry. */ static void TypeCacheRelCallback(Datum arg, Oid relid) { - HASH_SEQ_STATUS status; TypeCacheEntry *typentry; - /* TypeCacheHash must exist, else this callback wouldn't be registered */ - hash_seq_init(&status, TypeCacheHash); - while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) + /* + * RelIdToTypeIdCacheHash and TypeCacheHash should exist, otherwise this + * callback wouldn't be registered + */ + if (OidIsValid(relid)) { - if (typentry->typtype == TYPTYPE_COMPOSITE) + RelIdToTypeIdCacheEntry *relentry; + + /* + * Find an RelIdToTypeIdCacheHash entry, which should exist as soon as + * corresponding typcache entry has something to clean. + */ + relentry = (RelIdToTypeIdCacheEntry *) hash_search(RelIdToTypeIdCacheHash, + &relid, + HASH_FIND, NULL); + + if (relentry != NULL) { - /* Skip if no match, unless we're zapping all composite types */ - if (relid != typentry->typrelid && relid != InvalidOid) - continue; + typentry = (TypeCacheEntry *) hash_search(TypeCacheHash, + &relentry->composite_typid, + HASH_FIND, NULL); - /* Delete tupdesc if we have it */ - if (typentry->tupDesc != NULL) + if (typentry != NULL) { - /* - * Release our refcount, and free the tupdesc if none remain. - * (Can't use DecrTupleDescRefCount because this reference is - * not logged in current resource owner.) - */ - Assert(typentry->tupDesc->tdrefcount > 0); - if (--typentry->tupDesc->tdrefcount == 0) - FreeTupleDesc(typentry->tupDesc); - typentry->tupDesc = NULL; + Assert(typentry->typtype == TYPTYPE_COMPOSITE); + Assert(relid == typentry->typrelid); - /* - * Also clear tupDesc_identifier, so that anything watching - * that will realize that the tupdesc has possibly changed. - * (Alternatively, we could specify that to detect possible - * tupdesc change, one must check for tupDesc != NULL as well - * as tupDesc_identifier being the same as what was previously - * seen. That seems error-prone.) - */ - typentry->tupDesc_identifier = 0; + InvalidateCompositeTypeCacheEntry(typentry); } - - /* Reset equality/comparison/hashing validity information */ - typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; } - else if (typentry->typtype == TYPTYPE_DOMAIN) + + /* + * Visit all the domain types sequentially. Typically this shouldn't + * affect performance since domain types are less tended to bloat. + * Domain types are created manually, unlike composite types which are + * automatically created for every temporary table. + */ + for (typentry = firstDomainTypeEntry; + typentry != NULL; + typentry = typentry->nextDomain) { /* * If it's domain over composite, reset flags. (We don't bother @@ -2358,6 +2424,36 @@ TypeCacheRelCallback(Datum arg, Oid relid) typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; } } + else + { + HASH_SEQ_STATUS status; + + /* + * Relid is invalid. By convention we need to reset all composite + * types in cache. Also, we should reset flags for domain types, and + * we loop over all entries in hash, so, do it in a single scan. + */ + hash_seq_init(&status, TypeCacheHash); + while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) + { + if (typentry->typtype == TYPTYPE_COMPOSITE) + { + InvalidateCompositeTypeCacheEntry(typentry); + } + else if (typentry->typtype == TYPTYPE_DOMAIN) + { + /* + * If it's domain over composite, reset flags. (We don't + * bother trying to determine whether the specific base type + * needs a reset.) Note that if we haven't determined whether + * the base type is composite, we don't need to reset + * anything. + */ + if (typentry->flags & TCFLAGS_DOMAIN_BASE_IS_COMPOSITE) + typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; + } + } + } } /* @@ -2388,6 +2484,8 @@ TypeCacheTypCallback(Datum arg, int cacheid, uint32 hashvalue) while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) { + bool hadPgTypeData = (typentry->flags & TCFLAGS_HAVE_PG_TYPE_DATA); + Assert(hashvalue == 0 || typentry->type_id_hash == hashvalue); /* @@ -2397,6 +2495,13 @@ TypeCacheTypCallback(Datum arg, int cacheid, uint32 hashvalue) */ typentry->flags &= ~(TCFLAGS_HAVE_PG_TYPE_DATA | TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS); + + /* + * Call check_delete_rel_type_cache() if we cleaned + * TCFLAGS_HAVE_PG_TYPE_DATA flag previously. + */ + if (hadPgTypeData) + delete_rel_type_cache_if_needed(typentry); } } @@ -2905,3 +3010,85 @@ shared_record_typmod_registry_detach(dsm_segment *segment, Datum datum) } CurrentSession->shared_typmod_registry = NULL; } + +/* + * Insert RelIdToTypeIdCacheHash entry after setting of the + * TCFLAGS_HAVE_PG_TYPE_DATA flag if needed. + */ +static void +insert_rel_type_cache_if_needed(TypeCacheEntry *typentry) +{ + Assert(typentry->flags & TCFLAGS_HAVE_PG_TYPE_DATA); + + /* Immediately quit for non-composite types */ + if (typentry->typtype != TYPTYPE_COMPOSITE) + return; + + /* typrelid should be given for composite types */ + Assert(OidIsValid(typentry->typrelid)); + + /* + * Insert a RelIdToTypeIdCacheHash entry if the typentry doesn't have any + * information indicating there should be such an entry already. + */ + if (!(typentry->flags & TCFLAGS_OPERATOR_FLAGS) && + typentry->tupDesc == NULL) + { + RelIdToTypeIdCacheEntry *relentry; + bool found; + + relentry = (RelIdToTypeIdCacheEntry *) hash_search(RelIdToTypeIdCacheHash, + &typentry->typrelid, + HASH_ENTER, &found); + Assert(!found); + relentry->relid = typentry->typrelid; + relentry->composite_typid = typentry->type_id; + } +} + +/* + * Delete entry RelIdToTypeIdCacheHash if needed after resetting of the + * TCFLAGS_HAVE_PG_TYPE_DATA flag, or any of TCFLAGS_OPERATOR_FLAGS flags, + * or tupDesc. + */ +static void +delete_rel_type_cache_if_needed(TypeCacheEntry *typentry) +{ + /* Immediately quit for non-composite types */ + if (typentry->typtype != TYPTYPE_COMPOSITE) + return; + + /* typrelid should be given for composite types */ + Assert(OidIsValid(typentry->typrelid)); + + /* + * Delete a RelIdToTypeIdCacheHash entry if the typentry doesn't have any + * information indicating entry should be still there. + */ + if (!(typentry->flags & TCFLAGS_HAVE_PG_TYPE_DATA) && + !(typentry->flags & TCFLAGS_OPERATOR_FLAGS) && + typentry->tupDesc == NULL) + { + bool found; + + (void) hash_search(RelIdToTypeIdCacheHash, + &typentry->typrelid, + HASH_REMOVE, &found); + Assert(found); + } + else + { +#ifdef USE_ASSERT_CHECKING + /* + * In assert-enabled builds otherwise check for RelIdToTypeIdCacheHash + * entry if it should exist. + */ + bool found; + + (void) hash_search(RelIdToTypeIdCacheHash, + &typentry->typrelid, + HASH_FIND, &found); + Assert(found); +#endif + } +} diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 3f3a8f2634b..91203a7225f 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2377,6 +2377,7 @@ RelFileLocator RelFileLocatorBackend RelFileNumber RelIdCacheEnt +RelIdToTypeIdCacheEntry RelInfo RelInfoArr RelMapFile -- 2.39.3 (Apple Git-146) ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-08-25 19:00 Alexander Lakhin <[email protected]> parent: Alexander Korotkov <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Alexander Lakhin @ 2024-08-25 19:00 UTC (permalink / raw) To: Alexander Korotkov <[email protected]>; Pavel Borisov <[email protected]>; +Cc: Andrei Lepikhov <[email protected]>; Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]> Hello Alexander, 22.08.2024 19:52, Alexander Korotkov wrotd: > If no objections, I'm planning to push this after reverting PARTITION > SPLIT/MERGE. > Please try to perform `make check` against a CLOBBER_CACHE_ALWAYS build. trilobite failed it: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=trilobite&dt=2024-08-25%2005%3A22%3A07 and I'm observing the same locally: ... #5 0x00005636d37555f8 in ExceptionalCondition (conditionName=0x5636d39b1940 "found", fileName=0x5636d39b1308 "typcache.c", lineNumber=3077) at assert.c:66 #6 0x00005636d37554a4 in delete_rel_type_cache_if_needed (typentry=0x5636d41d5d10) at typcache.c:3077 #7 0x00005636d3754063 in InvalidateCompositeTypeCacheEntry (typentry=0x5636d41d5d10) at typcache.c:2355 #8 0x00005636d37541d3 in TypeCacheRelCallback (arg=0, relid=0) at typcache.c:2441 ... (gdb) f 6 #6 0x00005636d37554a4 in delete_rel_type_cache_if_needed (typentry=0x5636d41d5d10) at typcache.c:3077 3077 Assert(found); (gdb) p found $1 = false (This Assert is introduced by c14d4acb8.) Best regards, Alexander ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-08-25 19:21 Alexander Korotkov <[email protected]> parent: Alexander Lakhin <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Alexander Korotkov @ 2024-08-25 19:21 UTC (permalink / raw) To: Alexander Lakhin <[email protected]>; +Cc: Pavel Borisov <[email protected]>; Andrei Lepikhov <[email protected]>; Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]> On Sun, Aug 25, 2024 at 10:00 PM Alexander Lakhin <[email protected]> wrote: > 22.08.2024 19:52, Alexander Korotkov wrotd: > > If no objections, I'm planning to push this after reverting PARTITION > > SPLIT/MERGE. > > > > Please try to perform `make check` against a CLOBBER_CACHE_ALWAYS build. > trilobite failed it: > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=trilobite&dt=2024-08-25%2005%3A22%3A07 > > and I'm observing the same locally: > ... > #5 0x00005636d37555f8 in ExceptionalCondition (conditionName=0x5636d39b1940 "found", > fileName=0x5636d39b1308 "typcache.c", lineNumber=3077) at assert.c:66 > #6 0x00005636d37554a4 in delete_rel_type_cache_if_needed (typentry=0x5636d41d5d10) at typcache.c:3077 > #7 0x00005636d3754063 in InvalidateCompositeTypeCacheEntry (typentry=0x5636d41d5d10) at typcache.c:2355 > #8 0x00005636d37541d3 in TypeCacheRelCallback (arg=0, relid=0) at typcache.c:2441 > ... > > (gdb) f 6 > #6 0x00005636d37554a4 in delete_rel_type_cache_if_needed (typentry=0x5636d41d5d10) at typcache.c:3077 > 3077 Assert(found); > (gdb) p found > $1 = false > > (This Assert is introduced by c14d4acb8.) Thank you for noticing. I'm checking this. ------ Regards, Alexander Korotkov Supabase ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-08-25 21:22 Alexander Korotkov <[email protected]> parent: Alexander Korotkov <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Alexander Korotkov @ 2024-08-25 21:22 UTC (permalink / raw) To: Alexander Lakhin <[email protected]>; +Cc: Pavel Borisov <[email protected]>; Andrei Lepikhov <[email protected]>; Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]> On Sun, Aug 25, 2024 at 10:21 PM Alexander Korotkov <[email protected]> wrote: > On Sun, Aug 25, 2024 at 10:00 PM Alexander Lakhin <[email protected]> wrote: > > 22.08.2024 19:52, Alexander Korotkov wrotd: > > > If no objections, I'm planning to push this after reverting PARTITION > > > SPLIT/MERGE. > > > > > > > Please try to perform `make check` against a CLOBBER_CACHE_ALWAYS build. > > trilobite failed it: > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=trilobite&dt=2024-08-25%2005%3A22%3A07 > > > > and I'm observing the same locally: > > ... > > #5 0x00005636d37555f8 in ExceptionalCondition (conditionName=0x5636d39b1940 "found", > > fileName=0x5636d39b1308 "typcache.c", lineNumber=3077) at assert.c:66 > > #6 0x00005636d37554a4 in delete_rel_type_cache_if_needed (typentry=0x5636d41d5d10) at typcache.c:3077 > > #7 0x00005636d3754063 in InvalidateCompositeTypeCacheEntry (typentry=0x5636d41d5d10) at typcache.c:2355 > > #8 0x00005636d37541d3 in TypeCacheRelCallback (arg=0, relid=0) at typcache.c:2441 > > ... > > > > (gdb) f 6 > > #6 0x00005636d37554a4 in delete_rel_type_cache_if_needed (typentry=0x5636d41d5d10) at typcache.c:3077 > > 3077 Assert(found); > > (gdb) p found > > $1 = false > > > > (This Assert is introduced by c14d4acb8.) > > Thank you for noticing. I'm checking this. I didn't take into account that TypeCacheEntry could be invalidated while lookup_type_cache() does syscache lookups. When I realized that I was curious on how does it currently work. It appears that type cache invalidation mostly only clears the flags while values are remaining in place and still available for lookup_type_cache() caller. TypeCacheEntry.tupDesc is invalidated directly, and it has guarantee to survive only because we don't do any syscache lookups for composite data types later in lookup_type_cache(). I'm becoming less fan of how this works... I think these aspects needs to be at least documented in details. Regarding c14d4acb8, it appears to require redesign. I'm going to revert it. ------ Regards, Alexander Korotkov Supabase ^ permalink raw reply [nested|flat] 28+ messages in thread
* [PATCH v21 3/8] Row pattern recognition patch (rewriter). @ 2024-08-26 04:32 Tatsuo Ishii <[email protected]> 0 siblings, 0 replies; 28+ messages in thread From: Tatsuo Ishii @ 2024-08-26 04:32 UTC (permalink / raw) --- src/backend/utils/adt/ruleutils.c | 103 ++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index 00eda1b34c..dff7e169e7 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -426,6 +426,10 @@ static void get_rule_groupingset(GroupingSet *gset, List *targetlist, bool omit_parens, deparse_context *context); static void get_rule_orderby(List *orderList, List *targetList, bool force_colno, deparse_context *context); +static void get_rule_pattern(List *patternVariable, List *patternRegexp, + bool force_colno, deparse_context *context); +static void get_rule_define(List *defineClause, List *patternVariables, + bool force_colno, deparse_context *context); static void get_rule_windowclause(Query *query, deparse_context *context); static void get_rule_windowspec(WindowClause *wc, List *targetList, deparse_context *context); @@ -6460,6 +6464,67 @@ get_rule_orderby(List *orderList, List *targetList, } } +/* + * Display a PATTERN clause. + */ +static void +get_rule_pattern(List *patternVariable, List *patternRegexp, + bool force_colno, deparse_context *context) +{ + StringInfo buf = context->buf; + const char *sep; + ListCell *lc_var, + *lc_reg = list_head(patternRegexp); + + sep = ""; + appendStringInfoChar(buf, '('); + foreach(lc_var, patternVariable) + { + char *variable = strVal((String *) lfirst(lc_var)); + char *regexp = NULL; + + if (lc_reg != NULL) + { + regexp = strVal((String *) lfirst(lc_reg)); + + lc_reg = lnext(patternRegexp, lc_reg); + } + + appendStringInfo(buf, "%s%s", sep, variable); + if (regexp !=NULL) + appendStringInfoString(buf, regexp); + + sep = " "; + } + appendStringInfoChar(buf, ')'); +} + +/* + * Display a DEFINE clause. + */ +static void +get_rule_define(List *defineClause, List *patternVariables, + bool force_colno, deparse_context *context) +{ + StringInfo buf = context->buf; + const char *sep; + ListCell *lc_var, + *lc_def; + + sep = " "; + Assert(list_length(patternVariables) == list_length(defineClause)); + + forboth(lc_var, patternVariables, lc_def, defineClause) + { + char *varName = strVal(lfirst(lc_var)); + TargetEntry *te = (TargetEntry *) lfirst(lc_def); + + appendStringInfo(buf, "%s%s AS ", sep, varName); + get_rule_expr((Node *) te->expr, context, false); + sep = ",\n "; + } +} + /* * Display a WINDOW clause. * @@ -6597,6 +6662,44 @@ get_rule_windowspec(WindowClause *wc, List *targetList, appendStringInfoString(buf, "EXCLUDE GROUP "); else if (wc->frameOptions & FRAMEOPTION_EXCLUDE_TIES) appendStringInfoString(buf, "EXCLUDE TIES "); + /* RPR */ + if (wc->rpSkipTo == ST_NEXT_ROW) + appendStringInfoString(buf, + "\n AFTER MATCH SKIP TO NEXT ROW "); + else if (wc->rpSkipTo == ST_PAST_LAST_ROW) + appendStringInfoString(buf, + "\n AFTER MATCH SKIP PAST LAST ROW "); + else if (wc->rpSkipTo == ST_FIRST_VARIABLE) + appendStringInfo(buf, + "\n AFTER MATCH SKIP TO FIRST %s ", + wc->rpSkipVariable); + else if (wc->rpSkipTo == ST_LAST_VARIABLE) + appendStringInfo(buf, + "\n AFTER MATCH SKIP TO LAST %s ", + wc->rpSkipVariable); + else if (wc->rpSkipTo == ST_VARIABLE) + appendStringInfo(buf, + "\n AFTER MATCH SKIP TO %s ", + wc->rpSkipVariable); + + if (wc->initial) + appendStringInfoString(buf, "\n INITIAL"); + + if (wc->patternVariable) + { + appendStringInfoString(buf, "\n PATTERN "); + get_rule_pattern(wc->patternVariable, wc->patternRegexp, + false, context); + } + + if (wc->defineClause) + { + appendStringInfoString(buf, "\n DEFINE\n"); + get_rule_define(wc->defineClause, wc->patternVariable, + false, context); + appendStringInfoChar(buf, ' '); + } + /* we will now have a trailing space; remove it */ buf->len--; } -- 2.25.1 ----Next_Part(Mon_Aug_26_13_39_47_2024_878)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v21-0004-Row-pattern-recognition-patch-planner.patch" ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-08-26 06:37 Andrei Lepikhov <[email protected]> parent: Alexander Korotkov <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Andrei Lepikhov @ 2024-08-26 06:37 UTC (permalink / raw) To: Alexander Korotkov <[email protected]>; Alexander Lakhin <[email protected]>; +Cc: Pavel Borisov <[email protected]>; Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]> On 25/8/2024 23:22, Alexander Korotkov wrote: > On Sun, Aug 25, 2024 at 10:21 PM Alexander Korotkov >>> (This Assert is introduced by c14d4acb8.) >> >> Thank you for noticing. I'm checking this. > > I didn't take into account that TypeCacheEntry could be invalidated > while lookup_type_cache() does syscache lookups. When I realized that > I was curious on how does it currently work. It appears that type > cache invalidation mostly only clears the flags while values are > remaining in place and still available for lookup_type_cache() caller. > TypeCacheEntry.tupDesc is invalidated directly, and it has guarantee > to survive only because we don't do any syscache lookups for composite > data types later in lookup_type_cache(). I'm becoming less fan of how > this works... I think these aspects needs to be at least documented > in details. > > Regarding c14d4acb8, it appears to require redesign. I'm going to revert it. Sorry, but I don't understand your point. Let's refocus on the problem at hand. The issue arose when the TypeCacheTypCallback and the TypeCacheRelCallback were executed in sequence within InvalidateSystemCachesExtended. The first callback cleaned the flags TCFLAGS_HAVE_PG_TYPE_DATA and TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS. But the call of the second callback checks the typentry->tupDesc and, because it wasn't NULL, attempted to remove this record a second time. I think there is no case for redesign, but we have a mess in insertion/deletion conditions. -- regards, Andrei Lepikhov ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-08-26 08:26 Alexander Korotkov <[email protected]> parent: Andrei Lepikhov <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Alexander Korotkov @ 2024-08-26 08:26 UTC (permalink / raw) To: Andrei Lepikhov <[email protected]>; +Cc: Alexander Lakhin <[email protected]>; Pavel Borisov <[email protected]>; Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]> On Mon, Aug 26, 2024 at 9:37 AM Andrei Lepikhov <[email protected]> wrote: > On 25/8/2024 23:22, Alexander Korotkov wrote: > > On Sun, Aug 25, 2024 at 10:21 PM Alexander Korotkov > >>> (This Assert is introduced by c14d4acb8.) > >> > >> Thank you for noticing. I'm checking this. > > > > I didn't take into account that TypeCacheEntry could be invalidated > > while lookup_type_cache() does syscache lookups. When I realized that > > I was curious on how does it currently work. It appears that type > > cache invalidation mostly only clears the flags while values are > > remaining in place and still available for lookup_type_cache() caller. > > TypeCacheEntry.tupDesc is invalidated directly, and it has guarantee > > to survive only because we don't do any syscache lookups for composite > > data types later in lookup_type_cache(). I'm becoming less fan of how > > this works... I think these aspects needs to be at least documented > > in details. > > > > Regarding c14d4acb8, it appears to require redesign. I'm going to revert it. > Sorry, but I don't understand your point. > Let's refocus on the problem at hand. The issue arose when the > TypeCacheTypCallback and the TypeCacheRelCallback were executed in > sequence within InvalidateSystemCachesExtended. > The first callback cleaned the flags TCFLAGS_HAVE_PG_TYPE_DATA and > TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS. But the call of the second callback > checks the typentry->tupDesc and, because it wasn't NULL, attempted to > remove this record a second time. > I think there is no case for redesign, but we have a mess in > insertion/deletion conditions. Yes, it's possible to repair the current approach. But we need to do this correct, not just "not failing with current usages". Then we need to call insert_rel_type_cache_if_needed() not just when we set TCFLAGS_HAVE_PG_TYPE_DATA flag, but every time we set any of TCFLAGS_OPERATOR_FLAGS or tupDesc. That's a lot of places, not as simple and elegant as it was planned. This is why I wonder if there is a better approach. Secondly, I'm not terribly happy with current state of type cache. The caller of lookup_type_cache() might get already invalidated data. This probably OK, because caller probably hold locks on dependent objects to guarantee that relevant properties of type actually persists. At very least this should be documented, but it doesn't seem so. Setting of tupdesc is sensitive to its order of execution. That feels quite fragile to me, and not documented either. I think this area needs improvements before we push additional functionality there. ------ Regards, Alexander Korotkov Supabase ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-08-29 09:01 Alexander Korotkov <[email protected]> parent: Alexander Korotkov <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Alexander Korotkov @ 2024-08-29 09:01 UTC (permalink / raw) To: Andrei Lepikhov <[email protected]>; +Cc: Alexander Lakhin <[email protected]>; Pavel Borisov <[email protected]>; Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]> On Mon, Aug 26, 2024 at 11:26 AM Alexander Korotkov <[email protected]> wrote: > > On Mon, Aug 26, 2024 at 9:37 AM Andrei Lepikhov <[email protected]> wrote: > > On 25/8/2024 23:22, Alexander Korotkov wrote: > > > On Sun, Aug 25, 2024 at 10:21 PM Alexander Korotkov > > >>> (This Assert is introduced by c14d4acb8.) > > >> > > >> Thank you for noticing. I'm checking this. > > > > > > I didn't take into account that TypeCacheEntry could be invalidated > > > while lookup_type_cache() does syscache lookups. When I realized that > > > I was curious on how does it currently work. It appears that type > > > cache invalidation mostly only clears the flags while values are > > > remaining in place and still available for lookup_type_cache() caller. > > > TypeCacheEntry.tupDesc is invalidated directly, and it has guarantee > > > to survive only because we don't do any syscache lookups for composite > > > data types later in lookup_type_cache(). I'm becoming less fan of how > > > this works... I think these aspects needs to be at least documented > > > in details. > > > > > > Regarding c14d4acb8, it appears to require redesign. I'm going to revert it. > > Sorry, but I don't understand your point. > > Let's refocus on the problem at hand. The issue arose when the > > TypeCacheTypCallback and the TypeCacheRelCallback were executed in > > sequence within InvalidateSystemCachesExtended. > > The first callback cleaned the flags TCFLAGS_HAVE_PG_TYPE_DATA and > > TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS. But the call of the second callback > > checks the typentry->tupDesc and, because it wasn't NULL, attempted to > > remove this record a second time. > > I think there is no case for redesign, but we have a mess in > > insertion/deletion conditions. > > Yes, it's possible to repair the current approach. But we need to do > this correct, not just "not failing with current usages". Then we > need to call insert_rel_type_cache_if_needed() not just when we set > TCFLAGS_HAVE_PG_TYPE_DATA flag, but every time we set any of > TCFLAGS_OPERATOR_FLAGS or tupDesc. That's a lot of places, not as > simple and elegant as it was planned. This is why I wonder if there > is a better approach. > > Secondly, I'm not terribly happy with current state of type cache. > The caller of lookup_type_cache() might get already invalidated data. > This probably OK, because caller probably hold locks on dependent > objects to guarantee that relevant properties of type actually > persists. At very least this should be documented, but it doesn't > seem so. Setting of tupdesc is sensitive to its order of execution. > That feels quite fragile to me, and not documented either. I think > this area needs improvements before we push additional functionality > there. I see fdd965d074 added a proper handling for concurrent invalidation for relation cache. If a concurrent invalidation occurs, we retry building a relation descriptor. Thus, we end up with returning of a valid relation descriptor to caller. I wonder if we can take the same approach to type cache. That would make the whole type cache more consistent and less fragile. Also, this patch will be simpler. ------ Regards, Alexander Korotkov Supabase ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-08-31 19:33 Andrei Lepikhov <[email protected]> parent: Alexander Korotkov <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Andrei Lepikhov @ 2024-08-31 19:33 UTC (permalink / raw) To: Alexander Korotkov <[email protected]>; +Cc: Alexander Lakhin <[email protected]>; Pavel Borisov <[email protected]>; Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]> On 29/8/2024 11:01, Alexander Korotkov wrote: > On Mon, Aug 26, 2024 at 11:26 AM Alexander Korotkov >> Secondly, I'm not terribly happy with current state of type cache. >> The caller of lookup_type_cache() might get already invalidated data. >> This probably OK, because caller probably hold locks on dependent >> objects to guarantee that relevant properties of type actually >> persists. At very least this should be documented, but it doesn't >> seem so. Setting of tupdesc is sensitive to its order of execution. >> That feels quite fragile to me, and not documented either. I think >> this area needs improvements before we push additional functionality >> there. > > I see fdd965d074 added a proper handling for concurrent invalidation > for relation cache. If a concurrent invalidation occurs, we retry > building a relation descriptor. Thus, we end up with returning of a > valid relation descriptor to caller. I wonder if we can take the same > approach to type cache. That would make the whole type cache more > consistent and less fragile. Also, this patch will be simpler. I think I understand the solution from the commit fdd965d074. Just for the record, you mentioned invalidation inside the lookup_type_cache above. Passing through the code, I found the only place for such a case - the call of the GetDefaultOpClass, which triggers the opening of the relation pg_opclass, which can cause an AcceptInvalidationMessages call. Did you mean this case, or does a wider field of cases exist here? -- regards, Andrei Lepikhov ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-09-12 23:38 Alexander Korotkov <[email protected]> parent: Andrei Lepikhov <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Alexander Korotkov @ 2024-09-12 23:38 UTC (permalink / raw) To: Andrei Lepikhov <[email protected]>; +Cc: Alexander Lakhin <[email protected]>; Pavel Borisov <[email protected]>; Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]> On Sat, Aug 31, 2024 at 10:33 PM Andrei Lepikhov <[email protected]> wrote: > On 29/8/2024 11:01, Alexander Korotkov wrote: > > On Mon, Aug 26, 2024 at 11:26 AM Alexander Korotkov > >> Secondly, I'm not terribly happy with current state of type cache. > >> The caller of lookup_type_cache() might get already invalidated data. > >> This probably OK, because caller probably hold locks on dependent > >> objects to guarantee that relevant properties of type actually > >> persists. At very least this should be documented, but it doesn't > >> seem so. Setting of tupdesc is sensitive to its order of execution. > >> That feels quite fragile to me, and not documented either. I think > >> this area needs improvements before we push additional functionality > >> there. > > > > I see fdd965d074 added a proper handling for concurrent invalidation > > for relation cache. If a concurrent invalidation occurs, we retry > > building a relation descriptor. Thus, we end up with returning of a > > valid relation descriptor to caller. I wonder if we can take the same > > approach to type cache. That would make the whole type cache more > > consistent and less fragile. Also, this patch will be simpler. > I think I understand the solution from the commit fdd965d074. > Just for the record, you mentioned invalidation inside the > lookup_type_cache above. Passing through the code, I found the only > place for such a case - the call of the GetDefaultOpClass, which > triggers the opening of the relation pg_opclass, which can cause an > AcceptInvalidationMessages call. Did you mean this case, or does a wider > field of cases exist here? I've tried to implement handling of concurrent invalidation similar to commit fdd965d074. However that appears to be more difficult that I thought, because for some datatypes like arrays, ranges etc we might need fill the element type and reference it. So, I decided to continue with the current approach but borrowing some ideas from fdd965d074. The revised patchset attached. 0001 - adds comment about concurrent invalidation handling 0002 - revised c14d4acb8. Now we track type oids, whose TypeCacheEntry's filing is in-progress. Add entry to RelIdToTypeIdCacheHash at the end of lookup_type_cache() or on the transaction abort. During invalidation don't assert RelIdToTypeIdCacheHash to be here if TypeCacheEntry is in-progress. ------ Regards, Alexander Korotkov Supabase Attachments: [application/x-patch] v11-0001-Update-header-comment-for-lookup_type_cache.patch (1.4K, ../../CAPpHfdsQhwUrnB3of862j9RgHoJM--eRbifvBMvtQxpC57dxCA@mail.gmail.com/2-v11-0001-Update-header-comment-for-lookup_type_cache.patch) download | inline diff: From 0ed4be04b42acab74efc59ea35772fd51f1b954c Mon Sep 17 00:00:00 2001 From: Alexander Korotkov <[email protected]> Date: Fri, 13 Sep 2024 02:10:04 +0300 Subject: [PATCH v11 1/2] Update header comment for lookup_type_cache() Describe the way we handle concurrent invalidation messages. --- src/backend/utils/cache/typcache.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c index 2ec136b7d30..11382547ec0 100644 --- a/src/backend/utils/cache/typcache.c +++ b/src/backend/utils/cache/typcache.c @@ -351,6 +351,15 @@ type_cache_syshash(const void *key, Size keysize) * invalid. Note however that we may fail to find one or more of the * values requested by 'flags'; the caller needs to check whether the fields * are InvalidOid or not. + * + * Note that while filling TypeCacheEntry we might process concurrent + * invalidation messages, causing our not-yet-filled TypeCacheEntry to be + * invalidated. In this case, we typically only clear flags while values are + * still available for the caller. It's expected that the caller holds + * enough locks on type-depending objects that the values are still relevant. + * It's also important that the tupdesc is filled in the last for + * TYPTYPE_COMPOSITE. So, it can't get invalidated during the + * lookup_type_cache() call. */ TypeCacheEntry * lookup_type_cache(Oid type_id, int flags) -- 2.39.3 (Apple Git-146) [application/x-patch] v11-0002-Avoid-looping-over-all-type-cache-entries-in-Typ.patch (18.0K, ../../CAPpHfdsQhwUrnB3of862j9RgHoJM--eRbifvBMvtQxpC57dxCA@mail.gmail.com/3-v11-0002-Avoid-looping-over-all-type-cache-entries-in-Typ.patch) download | inline diff: From 9735f1266f3dad2955758a28814c4b8a593d834b Mon Sep 17 00:00:00 2001 From: Alexander Korotkov <[email protected]> Date: Tue, 10 Sep 2024 23:25:04 +0300 Subject: [PATCH v11 2/2] Avoid looping over all type cache entries in TypeCacheRelCallback() Currently when a single relcache entry gets invalidated, TypeCacheRelCallback() has to loop over all type cache entries to find appropriate typentry to invalidate. Unfortunately, using the syscache here is impossible, because this callback could be called outside a transaction and this makes impossible catalog lookups. This is why present commit introduces RelIdToTypeIdCacheHash to map relation OID to its composite type OID. We are keeping RelIdToTypeIdCacheHash entry while corresponding type cache entry have something to clean. Therefore, RelIdToTypeIdCacheHash shouldn't get bloat in the case of temporary tables flood. Discussion: https://postgr.es/m/5812a6e5-68ae-4d84-9d85-b443176966a1%40sigaev.ru Author: Teodor Sigaev Reviewed-by: Aleksander Alekseev, Tom Lane, Michael Paquier, Roman Zharkov Reviewed-by: Andrei Lepikhov, Pavel Borisov --- src/backend/access/transam/xact.c | 10 + src/backend/utils/cache/typcache.c | 350 +++++++++++++++++++++++++---- src/include/utils/typcache.h | 4 + src/tools/pgindent/typedefs.list | 1 + 4 files changed, 321 insertions(+), 44 deletions(-) diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 87700c7c5c7..b0b05e28790 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -70,6 +70,7 @@ #include "utils/snapmgr.h" #include "utils/timeout.h" #include "utils/timestamp.h" +#include "utils/typcache.h" /* * User-tweakable parameters @@ -2407,6 +2408,9 @@ CommitTransaction(void) /* Clean up the relation cache */ AtEOXact_RelationCache(true); + /* Clean up the type cache */ + AtEOXact_TypeCache(); + /* * Make catalog changes visible to all backends. This has to happen after * relcache references are dropped (see comments for @@ -2709,6 +2713,9 @@ PrepareTransaction(void) /* Clean up the relation cache */ AtEOXact_RelationCache(true); + /* Clean up the type cache */ + AtEOXact_TypeCache(); + /* notify doesn't need a postprepare call */ PostPrepare_PgStat(); @@ -2951,6 +2958,7 @@ AbortTransaction(void) false, true); AtEOXact_Buffers(false); AtEOXact_RelationCache(false); + AtEOXact_TypeCache(); AtEOXact_Inval(false); AtEOXact_MultiXact(); ResourceOwnerRelease(TopTransactionResourceOwner, @@ -5153,6 +5161,7 @@ CommitSubTransaction(void) true, false); AtEOSubXact_RelationCache(true, s->subTransactionId, s->parent->subTransactionId); + AtEOSubXact_TypeCache(); AtEOSubXact_Inval(true); AtSubCommit_smgr(); @@ -5328,6 +5337,7 @@ AbortSubTransaction(void) AtEOSubXact_RelationCache(false, s->subTransactionId, s->parent->subTransactionId); + AtEOSubXact_TypeCache(); AtEOSubXact_Inval(false); ResourceOwnerRelease(s->curTransactionOwner, RESOURCE_RELEASE_LOCKS, diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c index 11382547ec0..dcbf50e865b 100644 --- a/src/backend/utils/cache/typcache.c +++ b/src/backend/utils/cache/typcache.c @@ -77,6 +77,20 @@ /* The main type cache hashtable searched by lookup_type_cache */ static HTAB *TypeCacheHash = NULL; +/* + * The mapping of relation's OID to the corresponding composite type OID. + * We're keeping the map entry when the corresponding typentry has something + * to clear i.e it has either TCFLAGS_HAVE_PG_TYPE_DATA, or + * TCFLAGS_OPERATOR_FLAGS, or tupdesc. + */ +static HTAB *RelIdToTypeIdCacheHash = NULL; + +typedef struct RelIdToTypeIdCacheEntry +{ + Oid relid; /* OID of the relation */ + Oid composite_typid; /* OID of the relation's composite type */ +} RelIdToTypeIdCacheEntry; + /* List of type cache entries for domain types */ static TypeCacheEntry *firstDomainTypeEntry = NULL; @@ -208,6 +222,10 @@ typedef struct SharedTypmodTableEntry dsa_pointer shared_tupdesc; } SharedTypmodTableEntry; +static Oid *in_progress_list; +static int in_progress_list_len; +static int in_progress_list_maxlen; + /* * A comparator function for SharedRecordTableKey. */ @@ -329,6 +347,8 @@ static void shared_record_typmod_registry_detach(dsm_segment *segment, static TupleDesc find_or_make_matching_shared_tupledesc(TupleDesc tupdesc); static dsa_pointer share_tupledesc(dsa_area *area, TupleDesc tupdesc, uint32 typmod); +static void insert_rel_type_cache_if_needed(TypeCacheEntry *typentry); +static void delete_rel_type_cache_if_needed(TypeCacheEntry *typentry); /* @@ -366,11 +386,22 @@ lookup_type_cache(Oid type_id, int flags) { TypeCacheEntry *typentry; bool found; + int in_progress_offset; if (TypeCacheHash == NULL) { /* First time through: initialize the hash table */ HASHCTL ctl; + int allocsize; + + /* + * reserve enough in_progress_list slots for many cases + */ + allocsize = 4; + in_progress_list = + MemoryContextAlloc(CacheMemoryContext, + allocsize * sizeof(*in_progress_list)); + in_progress_list_maxlen = allocsize; ctl.keysize = sizeof(Oid); ctl.entrysize = sizeof(TypeCacheEntry); @@ -385,6 +416,13 @@ lookup_type_cache(Oid type_id, int flags) TypeCacheHash = hash_create("Type information cache", 64, &ctl, HASH_ELEM | HASH_FUNCTION); + Assert(RelIdToTypeIdCacheHash == NULL); + + ctl.keysize = sizeof(Oid); + ctl.entrysize = sizeof(RelIdToTypeIdCacheEntry); + RelIdToTypeIdCacheHash = hash_create("Map from relid to OID of cached composite type", 64, + &ctl, HASH_ELEM | HASH_BLOBS); + /* Also set up callbacks for SI invalidations */ CacheRegisterRelcacheCallback(TypeCacheRelCallback, (Datum) 0); CacheRegisterSyscacheCallback(TYPEOID, TypeCacheTypCallback, (Datum) 0); @@ -396,6 +434,21 @@ lookup_type_cache(Oid type_id, int flags) CreateCacheMemoryContext(); } + Assert(TypeCacheHash != NULL && RelIdToTypeIdCacheHash != NULL); + + /* Register to catch invalidation messages */ + if (in_progress_list_len >= in_progress_list_maxlen) + { + int allocsize; + + allocsize = in_progress_list_maxlen * 2; + in_progress_list = repalloc(in_progress_list, + allocsize * sizeof(*in_progress_list)); + in_progress_list_maxlen = allocsize; + } + in_progress_offset = in_progress_list_len++; + in_progress_list[in_progress_offset] = type_id; + /* Try to look up an existing entry */ typentry = (TypeCacheEntry *) hash_search(TypeCacheHash, &type_id, @@ -896,6 +949,11 @@ lookup_type_cache(Oid type_id, int flags) load_domaintype_info(typentry); } + Assert(in_progress_offset + 1 == in_progress_list_len); + in_progress_list_len--; + + insert_rel_type_cache_if_needed(typentry); + return typentry; } @@ -2290,6 +2348,53 @@ SharedRecordTypmodRegistryAttach(SharedRecordTypmodRegistry *registry) CurrentSession->shared_typmod_table = typmod_table; } +/* + * InvalidateCompositeTypeCacheEntry + * Invalidate particular TypeCacheEntry on Relcache inval callback + * + * Delete the cached tuple descriptor (if any) for the given composite + * type, and reset whatever info we have cached about the composite type's + * comparability. + */ +static void +InvalidateCompositeTypeCacheEntry(TypeCacheEntry *typentry) +{ + bool hadTupDescOrOpclass; + + Assert(typentry->typtype == TYPTYPE_COMPOSITE && + OidIsValid(typentry->typrelid)); + + hadTupDescOrOpclass = (typentry->tupDesc != NULL) || + (typentry->flags & TCFLAGS_OPERATOR_FLAGS); + + /* Delete tupdesc if we have it */ + if (typentry->tupDesc != NULL) + { + /* + * Release our refcount and free the tupdesc if none remain. We can't + * use DecrTupleDescRefCount here because this reference is not logged + * by the current resource owner. + */ + Assert(typentry->tupDesc->tdrefcount > 0); + if (--typentry->tupDesc->tdrefcount == 0) + FreeTupleDesc(typentry->tupDesc); + typentry->tupDesc = NULL; + + /* + * Also clear tupDesc_identifier, so that anyone watching it will + * realize that the tupdesc has changed. + */ + typentry->tupDesc_identifier = 0; + } + + /* Reset equality/comparison/hashing validity information */ + typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; + + /* Call check_delete_rel_type_cache() if we actually cleared something */ + if (hadTupDescOrOpclass) + delete_rel_type_cache_if_needed(typentry); +} + /* * TypeCacheRelCallback * Relcache inval callback function @@ -2299,63 +2404,55 @@ SharedRecordTypmodRegistryAttach(SharedRecordTypmodRegistry *registry) * whatever info we have cached about the composite type's comparability. * * This is called when a relcache invalidation event occurs for the given - * relid. We must scan the whole typcache hash since we don't know the - * type OID corresponding to the relid. We could do a direct search if this - * were a syscache-flush callback on pg_type, but then we would need all - * ALTER-TABLE-like commands that could modify a rowtype to issue syscache - * invals against the rel's pg_type OID. The extra SI signaling could very - * well cost more than we'd save, since in most usages there are not very - * many entries in a backend's typcache. The risk of bugs-of-omission seems - * high, too. - * - * Another possibility, with only localized impact, is to maintain a second - * hashtable that indexes composite-type typcache entries by their typrelid. - * But it's still not clear it's worth the trouble. + * relid. We can't use syscache to find a type corresponding to the given + * relation because the code can be called outside of transaction. Thus we use + * the RelIdToTypeIdCacheHash map to locate appropriate typcache entry. */ static void TypeCacheRelCallback(Datum arg, Oid relid) { - HASH_SEQ_STATUS status; TypeCacheEntry *typentry; - /* TypeCacheHash must exist, else this callback wouldn't be registered */ - hash_seq_init(&status, TypeCacheHash); - while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) + /* + * RelIdToTypeIdCacheHash and TypeCacheHash should exist, otherwise this + * callback wouldn't be registered + */ + if (OidIsValid(relid)) { - if (typentry->typtype == TYPTYPE_COMPOSITE) + RelIdToTypeIdCacheEntry *relentry; + + /* + * Find an RelIdToTypeIdCacheHash entry, which should exist as soon as + * corresponding typcache entry has something to clean. + */ + relentry = (RelIdToTypeIdCacheEntry *) hash_search(RelIdToTypeIdCacheHash, + &relid, + HASH_FIND, NULL); + + if (relentry != NULL) { - /* Skip if no match, unless we're zapping all composite types */ - if (relid != typentry->typrelid && relid != InvalidOid) - continue; + typentry = (TypeCacheEntry *) hash_search(TypeCacheHash, + &relentry->composite_typid, + HASH_FIND, NULL); - /* Delete tupdesc if we have it */ - if (typentry->tupDesc != NULL) + if (typentry != NULL) { - /* - * Release our refcount, and free the tupdesc if none remain. - * (Can't use DecrTupleDescRefCount because this reference is - * not logged in current resource owner.) - */ - Assert(typentry->tupDesc->tdrefcount > 0); - if (--typentry->tupDesc->tdrefcount == 0) - FreeTupleDesc(typentry->tupDesc); - typentry->tupDesc = NULL; + Assert(typentry->typtype == TYPTYPE_COMPOSITE); + Assert(relid == typentry->typrelid); - /* - * Also clear tupDesc_identifier, so that anything watching - * that will realize that the tupdesc has possibly changed. - * (Alternatively, we could specify that to detect possible - * tupdesc change, one must check for tupDesc != NULL as well - * as tupDesc_identifier being the same as what was previously - * seen. That seems error-prone.) - */ - typentry->tupDesc_identifier = 0; + InvalidateCompositeTypeCacheEntry(typentry); } - - /* Reset equality/comparison/hashing validity information */ - typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; } - else if (typentry->typtype == TYPTYPE_DOMAIN) + + /* + * Visit all the domain types sequentially. Typically this shouldn't + * affect performance since domain types are less tended to bloat. + * Domain types are created manually, unlike composite types which are + * automatically created for every temporary table. + */ + for (typentry = firstDomainTypeEntry; + typentry != NULL; + typentry = typentry->nextDomain) { /* * If it's domain over composite, reset flags. (We don't bother @@ -2367,6 +2464,36 @@ TypeCacheRelCallback(Datum arg, Oid relid) typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; } } + else + { + HASH_SEQ_STATUS status; + + /* + * Relid is invalid. By convention we need to reset all composite + * types in cache. Also, we should reset flags for domain types, and + * we loop over all entries in hash, so, do it in a single scan. + */ + hash_seq_init(&status, TypeCacheHash); + while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) + { + if (typentry->typtype == TYPTYPE_COMPOSITE) + { + InvalidateCompositeTypeCacheEntry(typentry); + } + else if (typentry->typtype == TYPTYPE_DOMAIN) + { + /* + * If it's domain over composite, reset flags. (We don't + * bother trying to determine whether the specific base type + * needs a reset.) Note that if we haven't determined whether + * the base type is composite, we don't need to reset + * anything. + */ + if (typentry->flags & TCFLAGS_DOMAIN_BASE_IS_COMPOSITE) + typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS; + } + } + } } /* @@ -2397,6 +2524,8 @@ TypeCacheTypCallback(Datum arg, int cacheid, uint32 hashvalue) while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL) { + bool hadPgTypeData = (typentry->flags & TCFLAGS_HAVE_PG_TYPE_DATA); + Assert(hashvalue == 0 || typentry->type_id_hash == hashvalue); /* @@ -2406,6 +2535,13 @@ TypeCacheTypCallback(Datum arg, int cacheid, uint32 hashvalue) */ typentry->flags &= ~(TCFLAGS_HAVE_PG_TYPE_DATA | TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS); + + /* + * Call check_delete_rel_type_cache() if we cleaned + * TCFLAGS_HAVE_PG_TYPE_DATA flag previously. + */ + if (hadPgTypeData) + delete_rel_type_cache_if_needed(typentry); } } @@ -2914,3 +3050,129 @@ shared_record_typmod_registry_detach(dsm_segment *segment, Datum datum) } CurrentSession->shared_typmod_registry = NULL; } + +/* + * Insert RelIdToTypeIdCacheHash entry if needed. + */ +static void +insert_rel_type_cache_if_needed(TypeCacheEntry *typentry) +{ + /* Immediately quit for non-composite types */ + if (typentry->typtype != TYPTYPE_COMPOSITE) + return; + + /* typrelid should be given for composite types */ + Assert(OidIsValid(typentry->typrelid)); + + /* + * Insert a RelIdToTypeIdCacheHash entry if the typentry have any + * information indicating it should be here. + */ + if ((typentry->flags & TCFLAGS_HAVE_PG_TYPE_DATA) || + (typentry->flags & TCFLAGS_OPERATOR_FLAGS) || + typentry->tupDesc != NULL) + { + RelIdToTypeIdCacheEntry *relentry; + bool found; + + relentry = (RelIdToTypeIdCacheEntry *) hash_search(RelIdToTypeIdCacheHash, + &typentry->typrelid, + HASH_ENTER, &found); + relentry->relid = typentry->typrelid; + relentry->composite_typid = typentry->type_id; + } +} + +/* + * Delete entry RelIdToTypeIdCacheHash if needed after resetting of the + * TCFLAGS_HAVE_PG_TYPE_DATA flag, or any of TCFLAGS_OPERATOR_FLAGS flags, + * or tupDesc. + */ +static void +delete_rel_type_cache_if_needed(TypeCacheEntry *typentry) +{ +#ifdef USE_ASSERT_CHECKING + int i; + bool is_in_progress = false; + + for (i = 0; i < in_progress_list_len; i++) + { + if (in_progress_list[i] == typentry->type_id) + { + is_in_progress = true; + break; + } + } +#endif + + /* Immediately quit for non-composite types */ + if (typentry->typtype != TYPTYPE_COMPOSITE) + return; + + /* typrelid should be given for composite types */ + Assert(OidIsValid(typentry->typrelid)); + + /* + * Delete a RelIdToTypeIdCacheHash entry if the typentry doesn't have any + * information indicating entry should be still there. + */ + if (!(typentry->flags & TCFLAGS_HAVE_PG_TYPE_DATA) && + !(typentry->flags & TCFLAGS_OPERATOR_FLAGS) && + typentry->tupDesc == NULL) + { + bool found; + + (void) hash_search(RelIdToTypeIdCacheHash, + &typentry->typrelid, + HASH_REMOVE, &found); + Assert(found || is_in_progress); + } + else + { +#ifdef USE_ASSERT_CHECKING + /* + * In assert-enabled builds otherwise check for RelIdToTypeIdCacheHash + * entry if it should exist. + */ + bool found; + + if (!is_in_progress) + { + (void) hash_search(RelIdToTypeIdCacheHash, + &typentry->typrelid, + HASH_FIND, &found); + Assert(found); + } +#endif + } +} + +static void +cleanup_in_progress_typentries(void) +{ + int i; + + for (i = 0; i < in_progress_list_len; i++) + { + TypeCacheEntry *typentry; + + typentry = (TypeCacheEntry *) hash_search(TypeCacheHash, + &in_progress_list[i], + HASH_FIND, NULL); + insert_rel_type_cache_if_needed(typentry); + } + + in_progress_list_len = 0; +} + +void +AtEOXact_TypeCache(void) +{ + cleanup_in_progress_typentries(); +} + +void +AtEOSubXact_TypeCache(void) +{ + cleanup_in_progress_typentries(); +} diff --git a/src/include/utils/typcache.h b/src/include/utils/typcache.h index f506cc4aa35..f3d73ecee3a 100644 --- a/src/include/utils/typcache.h +++ b/src/include/utils/typcache.h @@ -207,4 +207,8 @@ extern void SharedRecordTypmodRegistryInit(SharedRecordTypmodRegistry *, extern void SharedRecordTypmodRegistryAttach(SharedRecordTypmodRegistry *); +extern void AtEOXact_TypeCache(void); + +extern void AtEOSubXact_TypeCache(void); + #endif /* TYPCACHE_H */ diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index e9ebddde24d..38b60a5944b 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2378,6 +2378,7 @@ RelFileLocator RelFileLocatorBackend RelFileNumber RelIdCacheEnt +RelIdToTypeIdCacheEntry RelInfo RelInfoArr RelMapFile -- 2.39.3 (Apple Git-146) ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-09-18 14:10 Andrei Lepikhov <[email protected]> parent: Alexander Korotkov <[email protected]> 0 siblings, 1 reply; 28+ messages in thread From: Andrei Lepikhov @ 2024-09-18 14:10 UTC (permalink / raw) To: Alexander Korotkov <[email protected]>; +Cc: Alexander Lakhin <[email protected]>; Pavel Borisov <[email protected]>; Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]> On 13/9/2024 01:38, Alexander Korotkov wrote: > I've tried to implement handling of concurrent invalidation similar to > commit fdd965d074. However that appears to be more difficult that I > thought, because for some datatypes like arrays, ranges etc we might > need fill the element type and reference it. So, I decided to > continue with the current approach but borrowing some ideas from > fdd965d074. The revised patchset attached. Let me rephrase the issue in more straightforward terms to ensure we are all clear on the problem: The critical problem of the typcache lookup on not-yet-locked data is that it can lead to an inconsistent state of the TypEntry, potentially causing disruptions in the DBMS's operations, correct? Let's exemplify this statement. By filling typentry's lt_opr, eq_opr, and gt_opr fields, we access the AMOPSTRATEGY cache. One operation can successfully fetch data from the cache, but another can miss data and touch the catalogue table, causing invalidations. In this case, we can get an inconsistent set of operators. Do I understand the problem statement correctly? If this view is correct, your derived approach should work fine if all necessary callbacks are registered. I see that at least AMOPSTRATEGY and PROCOID were missed at the moment of the typcache initialization. -- regards, Andrei Lepikhov ^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: type cache cleanup improvements @ 2024-09-23 10:53 Alexander Korotkov <[email protected]> parent: Andrei Lepikhov <[email protected]> 0 siblings, 0 replies; 28+ messages in thread From: Alexander Korotkov @ 2024-09-23 10:53 UTC (permalink / raw) To: Andrei Lepikhov <[email protected]>; +Cc: Alexander Lakhin <[email protected]>; Pavel Borisov <[email protected]>; Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]> On Wed, Sep 18, 2024 at 5:10 PM Andrei Lepikhov <[email protected]> wrote: > On 13/9/2024 01:38, Alexander Korotkov wrote: > > I've tried to implement handling of concurrent invalidation similar to > > commit fdd965d074. However that appears to be more difficult that I > > thought, because for some datatypes like arrays, ranges etc we might > > need fill the element type and reference it. So, I decided to > > continue with the current approach but borrowing some ideas from > > fdd965d074. The revised patchset attached. > Let me rephrase the issue in more straightforward terms to ensure we are > all clear on the problem: > The critical problem of the typcache lookup on not-yet-locked data is > that it can lead to an inconsistent state of the TypEntry, potentially > causing disruptions in the DBMS's operations, correct? > Let's exemplify this statement. By filling typentry's lt_opr, eq_opr, > and gt_opr fields, we access the AMOPSTRATEGY cache. One operation can > successfully fetch data from the cache, but another can miss data and > touch the catalogue table, causing invalidations. In this case, we can > get an inconsistent set of operators. Do I understand the problem > statement correctly? Actually, I didn't research much if there is a material problem. So, I didn't try to concurrently delete some operator class members concurrently to lookup_type_cache(). There are probably some bugs, but they likely have low impact in practice, given that type/opclass changes are very rare. Yet I was concentrated on why do lookup_type_cache() returns TypeCacheEntry filled with whatever caller asked given there could be concurrent invalidations. So, my approach was to 1) Document how we currently handle concurrent invalidations. 2) Maintain RelIdToTypeIdCacheHash correctly with concurrent invalidations. ------ Regards, Alexander Korotkov Supabase ^ permalink raw reply [nested|flat] 28+ messages in thread
end of thread, other threads:[~2024-09-23 10:53 UTC | newest] Thread overview: 28+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2024-03-11 12:24 Re: type cache cleanup improvements Aleksander Alekseev <[email protected]> 2024-03-12 15:55 ` Teodor Sigaev <[email protected]> 2024-03-13 05:47 ` Michael Paquier <[email protected]> 2024-03-13 13:40 ` Teodor Sigaev <[email protected]> 2024-03-14 07:42 ` Michael Paquier <[email protected]> 2024-03-14 13:27 ` Teodor Sigaev <[email protected]> 2024-03-14 22:58 ` Michael Paquier <[email protected]> 2024-03-15 10:57 ` Teodor Sigaev <[email protected]> 2024-03-29 04:49 ` Жарков Роман <[email protected]> 2024-04-03 06:07 ` Andrei Lepikhov <[email protected]> 2024-08-05 01:16 ` Alexander Korotkov <[email protected]> 2024-08-20 19:00 ` Alexander Korotkov <[email protected]> 2024-08-21 13:28 ` Pavel Borisov <[email protected]> 2024-08-21 15:28 ` Alexander Korotkov <[email protected]> 2024-08-22 10:02 ` Pavel Borisov <[email protected]> 2024-08-22 16:52 ` Alexander Korotkov <[email protected]> 2024-08-25 19:00 ` Alexander Lakhin <[email protected]> 2024-08-25 19:21 ` Alexander Korotkov <[email protected]> 2024-08-25 21:22 ` Alexander Korotkov <[email protected]> 2024-08-26 06:37 ` Andrei Lepikhov <[email protected]> 2024-08-26 08:26 ` Alexander Korotkov <[email protected]> 2024-08-29 09:01 ` Alexander Korotkov <[email protected]> 2024-08-31 19:33 ` Andrei Lepikhov <[email protected]> 2024-09-12 23:38 ` Alexander Korotkov <[email protected]> 2024-09-18 14:10 ` Andrei Lepikhov <[email protected]> 2024-09-23 10:53 ` Alexander Korotkov <[email protected]> 2024-08-22 12:02 ` Andrei Lepikhov <[email protected]> 2024-08-26 04:32 [PATCH v21 3/8] Row pattern recognition patch (rewriter). Tatsuo Ishii <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox