public inbox for [email protected]
help / color / mirror / Atom feedFrom: Andrei Lepikhov <[email protected]>
To: Teodor Sigaev <[email protected]>
To: Pgsql Hackers <[email protected]>
Cc: Aleksander Alekseev <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Michael Paquier <[email protected]>
Subject: Re: type cache cleanup improvements
Date: Wed, 3 Apr 2024 13:07:27 +0700
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<CAJ7c6TP3-bnL5J+fm3-zmXK9WrHxhtQQYdZeFFpba0Eh3j+atg@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAJ7c6TPyfqAEbyXcAx7zZKrh8Wyp2H3fkmhEeVnO8O97japYtQ@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[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
view thread (28+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: type cache cleanup improvements
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox