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%40mail.gmail.com
--
regards,
Andrei Lepikhov
Postgres Professional
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
