public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v2 5/8] jit: explain: remove backend lifetime module count from function name.
32+ messages / 8 participants
[nested] [flat]
* [PATCH v2 5/8] jit: explain: remove backend lifetime module count from function name.
@ 2019-09-26 21:05 Andres Freund <[email protected]>
0 siblings, 0 replies; 32+ messages in thread
From: Andres Freund @ 2019-09-26 21:05 UTC (permalink / raw)
Also expand function name to include in which module the function is -
without that it's harder to analyze which functions were emitted
separately (a performance concern).
Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
src/backend/commands/explain.c | 65 +++++++++++++++++++++++++++++-----
src/backend/jit/llvm/llvmjit.c | 18 +++++++---
src/include/jit/llvmjit.h | 5 ++-
3 files changed, 75 insertions(+), 13 deletions(-)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 3ccb76bdfd1..02455865d9f 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -2228,6 +2228,43 @@ show_expression(Node *node, ExprState *expr, const char *qlabel,
}
}
+/*
+ * To make JIT explain output reproducible, remove the module generation from
+ * function names. That makes it a bit harder to correlate with profiles etc,
+ * but reproducability is more important.
+ */
+static char *
+jit_funcname_for_display(const char *funcname)
+{
+ int func_counter; /* nth function in query */
+ size_t mod_num; /* nth module in query */
+ size_t mod_generation; /* nth module in backend */
+ int basename_end;
+ int matchcount = 0;
+
+ /*
+ * The pattern we need to match, see llvm_expand_funcname, is
+ * "%s_%zu_%d_mod_%zu". Find the fourth _ from the end, so a _ in the name
+ * is OK.
+ */
+ for (basename_end = strlen(funcname); basename_end >= 0; basename_end--)
+ {
+ if (funcname[basename_end] == '_' && ++matchcount == 4)
+ break;
+ }
+
+ /* couldn't parse, bail out */
+ if (matchcount != 4)
+ return pstrdup(funcname);
+
+ /* couldn't parse, bail out */
+ if (sscanf(funcname + basename_end, "_%zu_%d_mod_%zu",
+ &mod_num, &func_counter, &mod_generation) != 3)
+ return pstrdup(funcname);
+
+ return psprintf("%s_%zu_%d", pnstrdup(funcname, basename_end), mod_num, func_counter);
+}
+
static void
show_jit_expr_details(ExprState *expr, ExplainState *es)
{
@@ -2239,7 +2276,8 @@ show_jit_expr_details(ExprState *expr, ExplainState *es)
if (es->format == EXPLAIN_FORMAT_TEXT)
{
if (expr->flags & EEO_FLAG_JIT_EXPR)
- appendStringInfo(es->str, "JIT-Expr: %s", expr->expr_funcname);
+ appendStringInfo(es->str, "JIT-Expr: %s",
+ jit_funcname_for_display(expr->expr_funcname));
else
appendStringInfoString(es->str, "JIT-Expr: false");
@@ -2250,19 +2288,22 @@ show_jit_expr_details(ExprState *expr, ExplainState *es)
*/
if (expr->scan_funcname)
- appendStringInfo(es->str, ", JIT-Deform-Scan: %s", expr->scan_funcname);
+ appendStringInfo(es->str, ", JIT-Deform-Scan: %s",
+ jit_funcname_for_display(expr->scan_funcname));
else if (expr->flags & EEO_FLAG_JIT_EXPR &&
expr->flags & EEO_FLAG_DEFORM_SCAN)
appendStringInfo(es->str, ", JIT-Deform-Scan: false");
if (expr->outer_funcname)
- appendStringInfo(es->str, ", JIT-Deform-Outer: %s", expr->outer_funcname);
+ appendStringInfo(es->str, ", JIT-Deform-Outer: %s",
+ jit_funcname_for_display(expr->outer_funcname));
else if (expr->flags & EEO_FLAG_JIT_EXPR &&
expr->flags & EEO_FLAG_DEFORM_OUTER)
appendStringInfo(es->str, ", JIT-Deform-Outer: false");
if (expr->inner_funcname)
- appendStringInfo(es->str, ", JIT-Deform-Inner: %s", expr->inner_funcname);
+ appendStringInfo(es->str, ", JIT-Deform-Inner: %s",
+ jit_funcname_for_display(expr->inner_funcname));
else if (expr->flags & EEO_FLAG_JIT_EXPR &&
expr->flags & (EEO_FLAG_DEFORM_INNER))
appendStringInfo(es->str, ", JIT-Deform-Inner: false");
@@ -2270,26 +2311,34 @@ show_jit_expr_details(ExprState *expr, ExplainState *es)
else
{
if (expr->flags & EEO_FLAG_JIT_EXPR)
- ExplainPropertyText("JIT-Expr", expr->expr_funcname, es);
+ ExplainPropertyText("JIT-Expr",
+ jit_funcname_for_display(expr->expr_funcname),
+ es);
else
ExplainPropertyBool("JIT-Expr", false, es);
if (expr->scan_funcname)
- ExplainProperty("JIT-Deform-Scan", NULL, expr->scan_funcname, false, es);
+ ExplainProperty("JIT-Deform-Scan", NULL,
+ jit_funcname_for_display(expr->scan_funcname),
+ false, es);
else if (expr->flags & EEO_FLAG_DEFORM_SCAN)
ExplainProperty("JIT-Deform-Scan", NULL, "false", true, es);
else
ExplainProperty("JIT-Deform-Scan", NULL, "null", true, es);
if (expr->outer_funcname)
- ExplainProperty("JIT-Deform-Outer", NULL, expr->outer_funcname, false, es);
+ ExplainProperty("JIT-Deform-Outer", NULL,
+ jit_funcname_for_display(expr->outer_funcname),
+ false, es);
else if (expr->flags & EEO_FLAG_DEFORM_OUTER)
ExplainProperty("JIT-Deform-Outer", NULL, "false", true, es);
else
ExplainProperty("JIT-Deform-Outer", NULL, "null", true, es);
if (expr->inner_funcname)
- ExplainProperty("JIT-Deform-Inner", NULL, expr->inner_funcname, false, es);
+ ExplainProperty("JIT-Deform-Inner", NULL,
+ jit_funcname_for_display(expr->inner_funcname),
+ false, es);
else if (expr->flags & EEO_FLAG_DEFORM_INNER)
ExplainProperty("JIT-Deform-Inner", NULL, "false", true, es);
else
diff --git a/src/backend/jit/llvm/llvmjit.c b/src/backend/jit/llvm/llvmjit.c
index 5489e118041..177a00f3826 100644
--- a/src/backend/jit/llvm/llvmjit.c
+++ b/src/backend/jit/llvm/llvmjit.c
@@ -227,6 +227,8 @@ llvm_mutable_module(LLVMJitContext *context)
char *
llvm_expand_funcname(struct LLVMJitContext *context, const char *basename)
{
+ char *funcname;
+
Assert(context->module != NULL);
context->base.instr.created_functions++;
@@ -234,11 +236,19 @@ llvm_expand_funcname(struct LLVMJitContext *context, const char *basename)
/*
* Previously we used dots to separate, but turns out some tools, e.g.
* GDB, don't like that and truncate name.
+ *
+ * Append the backend-lifetime module count to the end, so it's easier for
+ * humans and machines to compare the generated function names across
+ * queries, the prefix will be the same from query execution to query
+ * execution.
*/
- return psprintf("%s_%zu_%d",
- basename,
- context->module_generation,
- context->counter++);
+ funcname = psprintf("%s_%zu_%d_mod_%zu",
+ basename,
+ context->base.instr.created_modules - 1,
+ context->counter++,
+ context->module_generation);
+
+ return funcname;
}
/*
diff --git a/src/include/jit/llvmjit.h b/src/include/jit/llvmjit.h
index 6178864b2e6..e45ff99194f 100644
--- a/src/include/jit/llvmjit.h
+++ b/src/include/jit/llvmjit.h
@@ -41,7 +41,10 @@ typedef struct LLVMJitContext
{
JitContext base;
- /* number of modules created */
+ /*
+ * llvm_generation when ->module was created, monotonically increasing
+ * within the lifetime of a backend.
+ */
size_t module_generation;
/* current, "open for write", module */
--
2.23.0.385.gbc12974a89
--ga6shgqrocqphdjc
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v2-0006-WIP-explain-Show-per-phase-information-about-aggr.patch"
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-10 15:54 Artur Zakirov <[email protected]>
0 siblings, 1 reply; 32+ messages in thread
From: Artur Zakirov @ 2024-10-10 15:54 UTC (permalink / raw)
To: Alexander Korotkov <[email protected]>; +Cc: Andrei Lepikhov <[email protected]>; 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]>
Hi all,
On Fri, 13 Sept 2024 at 01:38, Alexander Korotkov <[email protected]> wrote:
>
> 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.
Thank you Alexander for the patch. I reviewed and tested it.
I used Teodor's script to check the performance. On my laptop on
master ROLLBACK runs 11496.219 ms. With patch ROLLBACK runs 378.990
ms.
It seems to me that there are couple of possible issues in the patch:
In `lookup_type_cache()` `in_progress_list` is allocated using
`CacheMemoryContext`, on the other hand it seems there might be a case
when `CacheMemoryContext` is not created yet. It is created below in
the code if it doesn't exist:
/* Also make sure CacheMemoryContext exists */
if (!CacheMemoryContext)
CreateCacheMemoryContext();
It is probably a very rare case, but it might be better to allocate
`in_progress_list` after that line, or move creation of
`CacheMemoryContext` higher.
Within `insert_rel_type_cache_if_needed()` and
`delete_rel_type_cache_if_needed()` there is an if condition:
if ((typentry->flags & TCFLAGS_HAVE_PG_TYPE_DATA) ||
(typentry->flags & TCFLAGS_OPERATOR_FLAGS) ||
typentry->tupDesc != NULL)
Based on the logic of the rest of the code does it make sense to use
TCFLAGS_DOMAIN_BASE_IS_COMPOSITE instead of TCFLAGS_OPERATOR_FLAGS?
Otherwise the condition doesn't look logical.
--
Kind regards,
Artur
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-13 12:08 Alexander Korotkov <[email protected]>
parent: Artur Zakirov <[email protected]>
0 siblings, 1 reply; 32+ messages in thread
From: Alexander Korotkov @ 2024-10-13 12:08 UTC (permalink / raw)
To: Artur Zakirov <[email protected]>; +Cc: Andrei Lepikhov <[email protected]>; 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]>
Hi, Arthur!
Thank you so much for your review!
On Thu, Oct 10, 2024 at 6:54 PM Artur Zakirov <[email protected]> wrote:
> On Fri, 13 Sept 2024 at 01:38, Alexander Korotkov <[email protected]> wrote:
> >
> > 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.
>
> Thank you Alexander for the patch. I reviewed and tested it.
>
> I used Teodor's script to check the performance. On my laptop on
> master ROLLBACK runs 11496.219 ms. With patch ROLLBACK runs 378.990
> ms.
>
> It seems to me that there are couple of possible issues in the patch:
>
> In `lookup_type_cache()` `in_progress_list` is allocated using
> `CacheMemoryContext`, on the other hand it seems there might be a case
> when `CacheMemoryContext` is not created yet. It is created below in
> the code if it doesn't exist:
>
> /* Also make sure CacheMemoryContext exists */
> if (!CacheMemoryContext)
> CreateCacheMemoryContext();
>
> It is probably a very rare case, but it might be better to allocate
> `in_progress_list` after that line, or move creation of
> `CacheMemoryContext` higher.
Yes, it makes sense to initialize `in_progress_list ` when
`CacheMemoryContext` is guaranteed to be initialized. Fixed in the
attached patch.
> Within `insert_rel_type_cache_if_needed()` and
> `delete_rel_type_cache_if_needed()` there is an if condition:
>
> if ((typentry->flags & TCFLAGS_HAVE_PG_TYPE_DATA) ||
> (typentry->flags & TCFLAGS_OPERATOR_FLAGS) ||
> typentry->tupDesc != NULL)
>
> Based on the logic of the rest of the code does it make sense to use
> TCFLAGS_DOMAIN_BASE_IS_COMPOSITE instead of TCFLAGS_OPERATOR_FLAGS?
> Otherwise the condition doesn't look logical.
I'm not sure I get the point. This check ensures that type entry has
something to be cleared. In this case we need to keep
RelIdToTypeIdCacheHash entry to find this item on invalidation
message. I'm not sure how TCFLAGS_DOMAIN_BASE_IS_COMPOSITE is
relevant here, because it's valid only for TYPTYPE_DOMAIN while this
patch deals with TYPTYPE_COMPOSITE.
------
Regards,
Alexander Korotkov
Supabase
Attachments:
[application/octet-stream] v12-0002-Avoid-looping-over-all-type-cache-entries-in-Typ.patch (18.0K, ../../CAPpHfdtQHpr6aMALxRmB+6SGBsYWVdTim9iW+zJtUjjU1L2CPQ@mail.gmail.com/2-v12-0002-Avoid-looping-over-all-type-cache-entries-in-Typ.patch)
download | inline diff:
From ace04f1edd1f71300175738dd60f85570b1c646f Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Tue, 10 Sep 2024 23:25:04 +0300
Subject: [PATCH v12 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..fe37ff442b9 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,13 @@ 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;
ctl.keysize = sizeof(Oid);
ctl.entrysize = sizeof(TypeCacheEntry);
@@ -385,6 +407,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);
@@ -394,8 +423,32 @@ lookup_type_cache(Oid type_id, int flags)
/* Also make sure CacheMemoryContext exists */
if (!CacheMemoryContext)
CreateCacheMemoryContext();
+
+ /*
+ * 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;
}
+ 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 a65e1c07c5d..80c25099e67 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.5 (Apple Git-154)
[application/octet-stream] v12-0001-Update-header-comment-for-lookup_type_cache.patch (1.4K, ../../CAPpHfdtQHpr6aMALxRmB+6SGBsYWVdTim9iW+zJtUjjU1L2CPQ@mail.gmail.com/3-v12-0001-Update-header-comment-for-lookup_type_cache.patch)
download | inline diff:
From 92bbee11fa999ce9ad89349ab174f9e48ba2c753 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Fri, 13 Sep 2024 02:10:04 +0300
Subject: [PATCH v12 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.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-15 07:34 jian he <[email protected]>
parent: Alexander Korotkov <[email protected]>
0 siblings, 1 reply; 32+ messages in thread
From: jian he @ 2024-10-15 07:34 UTC (permalink / raw)
To: Alexander Korotkov <[email protected]>; +Cc: Artur Zakirov <[email protected]>; Andrei Lepikhov <[email protected]>; 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 Sun, Oct 13, 2024 at 8:09 PM Alexander Korotkov <[email protected]> wrote:
>
hi. Alexander.
I don't fully understand all of it. but I did some tests anyway.
static void
cleanup_in_progress_typentries(void)
{
int i;
if (in_progress_list_len > 1)
elog(INFO, "%s:%d in_progress_list_len > 1", __FILE_NAME__, __LINE__);
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;
}
the regress still passed.
I assume "elog(INFO, " won't interfere in cleanup_in_progress_typentries.
So we lack tests for larger in_progress_list_len values or i missed something?
/* Call check_delete_rel_type_cache() if we actually cleared something */
if (hadTupDescOrOpclass)
delete_rel_type_cache_if_needed(typentry);
/*
* 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);
check_delete_rel_type_cache don't exist, so these comments are wrong?
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-15 08:08 Alexander Korotkov <[email protected]>
parent: jian he <[email protected]>
0 siblings, 3 replies; 32+ messages in thread
From: Alexander Korotkov @ 2024-10-15 08:08 UTC (permalink / raw)
To: jian he <[email protected]>; +Cc: Artur Zakirov <[email protected]>; Andrei Lepikhov <[email protected]>; 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]>
Hi, Jian!
Thank you for your review.
On Tue, Oct 15, 2024 at 10:34 AM jian he <[email protected]> wrote:
> I don't fully understand all of it. but I did some tests anyway.
>
> static void
> cleanup_in_progress_typentries(void)
> {
> int i;
> if (in_progress_list_len > 1)
> elog(INFO, "%s:%d in_progress_list_len > 1", __FILE_NAME__, __LINE__);
> 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;
> }
>
> the regress still passed.
> I assume "elog(INFO, " won't interfere in cleanup_in_progress_typentries.
> So we lack tests for larger in_progress_list_len values or i missed something?
Try to run test suite with -DCLOBBER_CACHE_ALWAYS.
> /* Call check_delete_rel_type_cache() if we actually cleared something */
> if (hadTupDescOrOpclass)
> delete_rel_type_cache_if_needed(typentry);
>
> /*
> * 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);
>
> check_delete_rel_type_cache don't exist, so these comments are wrong?
Yep, they didn't get updated. Fixed in the attached patchset.
------
Regards,
Alexander Korotkov
Supabase
Attachments:
[application/octet-stream] v13-0001-Update-header-comment-for-lookup_type_cache.patch (1.4K, ../../CAPpHfdss4xOrAXD8bS+0nc2GyZBuNX=1QbgLwpjCLg4rzbbrhw@mail.gmail.com/2-v13-0001-Update-header-comment-for-lookup_type_cache.patch)
download | inline diff:
From 8e736ebc3f69fec323351bd466d178309b734e27 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Fri, 13 Sep 2024 02:10:04 +0300
Subject: [PATCH v13 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.5 (Apple Git-154)
[application/octet-stream] v13-0002-Avoid-looping-over-all-type-cache-entries-in-Typ.patch (18.0K, ../../CAPpHfdss4xOrAXD8bS+0nc2GyZBuNX=1QbgLwpjCLg4rzbbrhw@mail.gmail.com/3-v13-0002-Avoid-looping-over-all-type-cache-entries-in-Typ.patch)
download | inline diff:
From d2fe600b042ea6b21f4a2460b4754b47b3775e8e Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Tue, 10 Sep 2024 23:25:04 +0300
Subject: [PATCH v13 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..f54e7d531a8 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,13 @@ 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;
ctl.keysize = sizeof(Oid);
ctl.entrysize = sizeof(TypeCacheEntry);
@@ -385,6 +407,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);
@@ -394,8 +423,32 @@ lookup_type_cache(Oid type_id, int flags)
/* Also make sure CacheMemoryContext exists */
if (!CacheMemoryContext)
CreateCacheMemoryContext();
+
+ /*
+ * 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;
}
+ 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 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 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 57de1acff3a..9b3e7fd104b 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.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-15 08:45 Artur Zakirov <[email protected]>
parent: Alexander Korotkov <[email protected]>
2 siblings, 0 replies; 32+ messages in thread
From: Artur Zakirov @ 2024-10-15 08:45 UTC (permalink / raw)
To: Alexander Korotkov <[email protected]>; +Cc: jian he <[email protected]>; Andrei Lepikhov <[email protected]>; 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 Tue, 15 Oct 2024 at 10:09, Alexander Korotkov <[email protected]> wrote:
> > /* Call check_delete_rel_type_cache() if we actually cleared something */
> > if (hadTupDescOrOpclass)
> > delete_rel_type_cache_if_needed(typentry);
> >
> > /*
> > * 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);
> >
> > check_delete_rel_type_cache don't exist, so these comments are wrong?
>
> Yep, they didn't get updated. Fixed in the attached patchset.
Thank you Alexander for the fixes. The last version of the patch looks
good to me.
> I'm not sure I get the point. This check ensures that type entry has
> something to be cleared. In this case we need to keep
> RelIdToTypeIdCacheHash entry to find this item on invalidation
> message. I'm not sure how TCFLAGS_DOMAIN_BASE_IS_COMPOSITE is
> relevant here, because it's valid only for TYPTYPE_DOMAIN while this
> patch deals with TYPTYPE_COMPOSITE.
Regarding this discussion earlier, I assumed that TYPTYPE_DOMAIN also
needs to be handled by `insert_rel_type_cache_if_needed()`. And it
seems that handling of TYPTYPE_DOMAIN will remain the same as before.
--
Kind regards,
Artur
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-15 09:50 jian he <[email protected]>
parent: Alexander Korotkov <[email protected]>
2 siblings, 2 replies; 32+ messages in thread
From: jian he @ 2024-10-15 09:50 UTC (permalink / raw)
To: Alexander Korotkov <[email protected]>; +Cc: Artur Zakirov <[email protected]>; Andrei Lepikhov <[email protected]>; 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 Tue, Oct 15, 2024 at 4:09 PM Alexander Korotkov <[email protected]> wrote:
>
> Hi, Jian!
>
> Thank you for your review.
>
> On Tue, Oct 15, 2024 at 10:34 AM jian he <[email protected]> wrote:
> > I don't fully understand all of it. but I did some tests anyway.
> >
> > static void
> > cleanup_in_progress_typentries(void)
> > {
> > int i;
> > if (in_progress_list_len > 1)
> > elog(INFO, "%s:%d in_progress_list_len > 1", __FILE_NAME__, __LINE__);
> > 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;
> > }
> >
> > the regress still passed.
> > I assume "elog(INFO, " won't interfere in cleanup_in_progress_typentries.
> > So we lack tests for larger in_progress_list_len values or i missed something?
>
> Try to run test suite with -DCLOBBER_CACHE_ALWAYS.
>
build from source, DCLOBBER_CACHE_ALWAYS takes a very long time.
So I gave up.
in lookup_type_cache, we unconditionally do
in_progress_list_len++;
in_progress_list_len--;
"static int in_progress_list_len;"
means in_progress_list_len value change is confined in
src/backend/utils/cache/typcache.c.
based on above information, i am still confused with
cleanup_in_progress_typentries, in_progress_list_len
is there any simple sql example to demo
cleanup_in_progress_typentries, in_progress_list_len> 0.
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-15 13:16 Artur Zakirov <[email protected]>
parent: jian he <[email protected]>
1 sibling, 0 replies; 32+ messages in thread
From: Artur Zakirov @ 2024-10-15 13:16 UTC (permalink / raw)
To: jian he <[email protected]>; +Cc: Alexander Korotkov <[email protected]>; Andrei Lepikhov <[email protected]>; 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 Tue, 15 Oct 2024 at 11:50, jian he <[email protected]> wrote:
> based on above information, i am still confused with
> cleanup_in_progress_typentries, in_progress_list_len
> is there any simple sql example to demo
> cleanup_in_progress_typentries, in_progress_list_len> 0.
AFAIK to reproduce cases when `in_progress_list_len > 0`
`lookup_type_cache()` should fail during its execution.
To do so you can call `lookup_type_cache()` with non-existing type_id
from a C function.
--
Kind regards,
Artur
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-17 09:41 Andrei Lepikhov <[email protected]>
parent: Alexander Korotkov <[email protected]>
2 siblings, 1 reply; 32+ messages in thread
From: Andrei Lepikhov @ 2024-10-17 09:41 UTC (permalink / raw)
To: Alexander Korotkov <[email protected]>; +Cc: Artur Zakirov <[email protected]>; Alexander Lakhin <[email protected]>; jian he <[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 10/15/24 15:08, Alexander Korotkov wrote:
> Yep, they didn't get updated. Fixed in the attached patchset.
Let me wear Alexander Lakhin's mask for a moment and say that the code
may cause a segfault:
#0 0x000055e0da186000 in insert_rel_type_cache_if_needed (typentry=0x0)
at typcache.c:3066
b3066 if (typentry->typtype != TYPTYPE_COMPOSITE)
(gdb) bt 20
#0 0x000055e0da186000 in insert_rel_type_cache_if_needed (typentry=0x0)
at typcache.c:3066
#1 0x000055e0da18844f in cleanup_in_progress_typentries () at
typcache.c:3172
#2 0x000055e0da1883f9 in AtEOXact_TypeCache () at typcache.c:3181
#3 0x000055e0d9a22e59 in AbortTransaction () at xact.c:2961
#4 0x000055e0d9a1f75c in AbortCurrentTransactionInternal () at xact.c:3491
#5 0x000055e0d9a1f6be in AbortCurrentTransaction () at xact.c:3445
#6 0x000055e0d9f55f28 in PostgresMain (dbname=0x55e1057fb838
"regression", username=0x55e1057fb818 "danolivo") at postgres.c:4508
#7 0x000055e0d9f4e4a3 in BackendMain (startup_data=0x7ffeaf051310 "",
startup_data_len=4) at backend_startup.c:107
#8 0x000055e0d9e4bfee in postmaster_child_launch (child_type=B_BACKEND,
startup_data=0x7ffeaf051310 "", startup_data_len=4,
client_sock=0x7ffeaf051358) at launch_backend.c:274
#9 0x000055e0d9e522a3 in BackendStartup (client_sock=0x7ffeaf051358) at
postmaster.c:3420
#10 0x000055e0d9e502f9 in ServerLoop () at postmaster.c:1653
#11 0x000055e0d9e4f4fe in PostmasterMain (argc=3, argv=0x55e1057bb520)
at postmaster.c:1351
#12 0x000055e0d9cf4b2d in main (argc=3, argv=0x55e1057bb520) at main.c:197
It can happen if something triggers an error in the middle of
lookup_type_cache when in_progress_list[i] is already filled, but the
typentry wasn't created.
I think it can be easily shielded (see attached). Also, the name
cleanup_in_progress_typentries causes a lot of ponderings, I guess it
would be better to rename it likewise finalize_in_progress_typentries.
Also, I added trivial comments to better understand what the function does.
I think the first patch may already be committed, and this little burden
may be avoided in future versions.
--
regards, Andrei Lepikhov
Attachments:
[text/x-patch] minor-fix.diff (1.2K, ../../[email protected]/2-minor-fix.diff)
download | inline diff:
diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c
index f54e7d531a..45aed74019 100644
--- a/src/backend/utils/cache/typcache.c
+++ b/src/backend/utils/cache/typcache.c
@@ -3058,7 +3058,7 @@ static void
insert_rel_type_cache_if_needed(TypeCacheEntry *typentry)
{
/* Immediately quit for non-composite types */
- if (typentry->typtype != TYPTYPE_COMPOSITE)
+ if (!typentry || typentry->typtype != TYPTYPE_COMPOSITE)
return;
/* typrelid should be given for composite types */
@@ -3147,8 +3147,13 @@ delete_rel_type_cache_if_needed(TypeCacheEntry *typentry)
}
}
+/*
+ * Add into the accessory hash table entries, added into TypCache and not added
+ * into the RelIdToTypeId matching hash table.
+ * It may happen in case of an error raised during the lookup_type_cache call.
+ */
static void
-cleanup_in_progress_typentries(void)
+finalize_in_progress_typentries(void)
{
int i;
@@ -3168,11 +3173,11 @@ cleanup_in_progress_typentries(void)
void
AtEOXact_TypeCache(void)
{
- cleanup_in_progress_typentries();
+ finalize_in_progress_typentries();
}
void
AtEOSubXact_TypeCache(void)
{
- cleanup_in_progress_typentries();
+ finalize_in_progress_typentries();
}
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-20 17:36 Alexander Korotkov <[email protected]>
parent: Andrei Lepikhov <[email protected]>
0 siblings, 1 reply; 32+ messages in thread
From: Alexander Korotkov @ 2024-10-20 17:36 UTC (permalink / raw)
To: Andrei Lepikhov <[email protected]>; +Cc: Artur Zakirov <[email protected]>; Alexander Lakhin <[email protected]>; jian he <[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 Thu, Oct 17, 2024 at 12:41 PM Andrei Lepikhov <[email protected]> wrote:
> On 10/15/24 15:08, Alexander Korotkov wrote:
> > Yep, they didn't get updated. Fixed in the attached patchset.
> Let me wear Alexander Lakhin's mask for a moment and say that the code
> may cause a segfault:
>
> #0 0x000055e0da186000 in insert_rel_type_cache_if_needed (typentry=0x0)
> at typcache.c:3066
> b3066 if (typentry->typtype != TYPTYPE_COMPOSITE)
> (gdb) bt 20
> #0 0x000055e0da186000 in insert_rel_type_cache_if_needed (typentry=0x0)
> at typcache.c:3066
> #1 0x000055e0da18844f in cleanup_in_progress_typentries () at
> typcache.c:3172
> #2 0x000055e0da1883f9 in AtEOXact_TypeCache () at typcache.c:3181
> #3 0x000055e0d9a22e59 in AbortTransaction () at xact.c:2961
> #4 0x000055e0d9a1f75c in AbortCurrentTransactionInternal () at xact.c:3491
> #5 0x000055e0d9a1f6be in AbortCurrentTransaction () at xact.c:3445
> #6 0x000055e0d9f55f28 in PostgresMain (dbname=0x55e1057fb838
> "regression", username=0x55e1057fb818 "danolivo") at postgres.c:4508
> #7 0x000055e0d9f4e4a3 in BackendMain (startup_data=0x7ffeaf051310 "",
> startup_data_len=4) at backend_startup.c:107
> #8 0x000055e0d9e4bfee in postmaster_child_launch (child_type=B_BACKEND,
> startup_data=0x7ffeaf051310 "", startup_data_len=4,
> client_sock=0x7ffeaf051358) at launch_backend.c:274
> #9 0x000055e0d9e522a3 in BackendStartup (client_sock=0x7ffeaf051358) at
> postmaster.c:3420
> #10 0x000055e0d9e502f9 in ServerLoop () at postmaster.c:1653
> #11 0x000055e0d9e4f4fe in PostmasterMain (argc=3, argv=0x55e1057bb520)
> at postmaster.c:1351
> #12 0x000055e0d9cf4b2d in main (argc=3, argv=0x55e1057bb520) at main.c:197
>
> It can happen if something triggers an error in the middle of
> lookup_type_cache when in_progress_list[i] is already filled, but the
> typentry wasn't created.
> I think it can be easily shielded (see attached). Also, the name
> cleanup_in_progress_typentries causes a lot of ponderings, I guess it
> would be better to rename it likewise finalize_in_progress_typentries.
> Also, I added trivial comments to better understand what the function does.
>
> I think the first patch may already be committed, and this little burden
> may be avoided in future versions.
Thank you!
I've integrated your patch. But I think
finalize_in_progress_typentries() is more appropriate place to check
typentry for NULL. Also, I've revised the
finalize_in_progress_typentries() header comment. I'm going to push
these patches if no objections.
------
Regards,
Alexander Korotkov
Supabase
Attachments:
[application/octet-stream] v14-0002-Avoid-looping-over-all-type-cache-entries-in-Typ.patch (18.3K, ../../CAPpHfdtj5yXgPiPJBenTLaFvBbF7U+LVpN=Eho1=o01jU_KQaA@mail.gmail.com/2-v14-0002-Avoid-looping-over-all-type-cache-entries-in-Typ.patch)
download | inline diff:
From a4bbadce7df1777eea8b98fb72f1c9163ac8ce11 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Tue, 10 Sep 2024 23:25:04 +0300
Subject: [PATCH v14 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 | 356 +++++++++++++++++++++++++----
src/include/utils/typcache.h | 4 +
src/tools/pgindent/typedefs.list | 1 +
4 files changed, 327 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..5df965443ae 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,13 @@ 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;
ctl.keysize = sizeof(Oid);
ctl.entrysize = sizeof(TypeCacheEntry);
@@ -385,6 +407,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);
@@ -394,8 +423,32 @@ lookup_type_cache(Oid type_id, int flags)
/* Also make sure CacheMemoryContext exists */
if (!CacheMemoryContext)
CreateCacheMemoryContext();
+
+ /*
+ * 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;
}
+ 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 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 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,135 @@ 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
+ }
+}
+
+/*
+ * Add possibly missing RelIdToTypeId entries related to TypeCacheHas
+ * entries, marked as in-progress by lookup_type_cache(). It may happen
+ * in case of an error or interruption during the lookup_type_cache() call.
+ */
+static void
+finalize_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);
+ if (typentry)
+ insert_rel_type_cache_if_needed(typentry);
+ }
+
+ in_progress_list_len = 0;
+}
+
+void
+AtEOXact_TypeCache(void)
+{
+ finalize_in_progress_typentries();
+}
+
+void
+AtEOSubXact_TypeCache(void)
+{
+ finalize_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 57de1acff3a..9b3e7fd104b 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.5 (Apple Git-154)
[application/octet-stream] v14-0001-Update-header-comment-for-lookup_type_cache.patch (1.4K, ../../CAPpHfdtj5yXgPiPJBenTLaFvBbF7U+LVpN=Eho1=o01jU_KQaA@mail.gmail.com/3-v14-0001-Update-header-comment-for-lookup_type_cache.patch)
download | inline diff:
From d624b14c9a4bd93426a5e475d4503589927ef2ae Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Fri, 13 Sep 2024 02:10:04 +0300
Subject: [PATCH v14 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.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-20 17:47 Alexander Korotkov <[email protected]>
parent: jian he <[email protected]>
1 sibling, 1 reply; 32+ messages in thread
From: Alexander Korotkov @ 2024-10-20 17:47 UTC (permalink / raw)
To: jian he <[email protected]>; +Cc: Artur Zakirov <[email protected]>; Andrei Lepikhov <[email protected]>; 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 Tue, Oct 15, 2024 at 12:50 PM jian he <[email protected]> wrote:
>
> On Tue, Oct 15, 2024 at 4:09 PM Alexander Korotkov <[email protected]> wrote:
> >
> > Hi, Jian!
> >
> > Thank you for your review.
> >
> > On Tue, Oct 15, 2024 at 10:34 AM jian he <[email protected]> wrote:
> > > I don't fully understand all of it. but I did some tests anyway.
> > >
> > > static void
> > > cleanup_in_progress_typentries(void)
> > > {
> > > int i;
> > > if (in_progress_list_len > 1)
> > > elog(INFO, "%s:%d in_progress_list_len > 1", __FILE_NAME__, __LINE__);
> > > 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;
> > > }
> > >
> > > the regress still passed.
> > > I assume "elog(INFO, " won't interfere in cleanup_in_progress_typentries.
> > > So we lack tests for larger in_progress_list_len values or i missed something?
> >
> > Try to run test suite with -DCLOBBER_CACHE_ALWAYS.
> >
>
> build from source, DCLOBBER_CACHE_ALWAYS takes a very long time.
> So I gave up.
>
>
> in lookup_type_cache, we unconditionally do
> in_progress_list_len++;
> in_progress_list_len--;
Yes, this should work OK when no errors. On error or interruption,
finalize_in_progress_typentries() will clean the things up.
> "static int in_progress_list_len;"
> means in_progress_list_len value change is confined in
> src/backend/utils/cache/typcache.c.
Yep.
> based on above information, i am still confused with
> cleanup_in_progress_typentries, in_progress_list_len
> is there any simple sql example to demo
> cleanup_in_progress_typentries, in_progress_list_len> 0.
I don't think there is simple sql to reliably reproduce that. In
order to hit that, we must process invalidation messages in some
(short) moment of time during lookup_type_cache(). You can reproduce
that by setting a breakpoint in lookup_type_cache() and in parallel do
something to invalidate the type cache entry (for instance, ALTER
TABLE ... ADD COLUMN ... would invalidate the composite type). In
principle, we can reproduce that using injection points. However, I'm
not intended to do that as long as we have buildfarm members with
-DCLOBBER_CACHE_ALWAYS. FWIW, I will for sure run tests with
-DCLOBBER_CACHE_ALWAYS before committing this.
------
Regards,
Alexander Korotkov
Supabase
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-20 18:00 Alexander Korotkov <[email protected]>
parent: Alexander Korotkov <[email protected]>
0 siblings, 1 reply; 32+ messages in thread
From: Alexander Korotkov @ 2024-10-20 18:00 UTC (permalink / raw)
To: jian he <[email protected]>; +Cc: Artur Zakirov <[email protected]>; Andrei Lepikhov <[email protected]>; 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 Sun, Oct 20, 2024 at 8:47 PM Alexander Korotkov <[email protected]> wrote:
> On Tue, Oct 15, 2024 at 12:50 PM jian he <[email protected]> wrote:
> > build from source, DCLOBBER_CACHE_ALWAYS takes a very long time.
> > So I gave up.
> >
> >
> > in lookup_type_cache, we unconditionally do
> > in_progress_list_len++;
> > in_progress_list_len--;
>
> Yes, this should work OK when no errors. On error or interruption,
> finalize_in_progress_typentries() will clean the things up.
>
> > "static int in_progress_list_len;"
> > means in_progress_list_len value change is confined in
> > src/backend/utils/cache/typcache.c.
>
> Yep.
>
> > based on above information, i am still confused with
> > cleanup_in_progress_typentries, in_progress_list_len
> > is there any simple sql example to demo
> > cleanup_in_progress_typentries, in_progress_list_len> 0.
>
> I don't think there is simple sql to reliably reproduce that. In
> order to hit that, we must process invalidation messages in some
> (short) moment of time during lookup_type_cache(). You can reproduce
> that by setting a breakpoint in lookup_type_cache() and in parallel do
> something to invalidate the type cache entry (for instance, ALTER
> TABLE ... ADD COLUMN ... would invalidate the composite type).
Oops, concurrent invalidation message is not enough here. So,
-DCLOBBER_CACHE_ALWAYS is also not enough to reproduce the situation.
Injection-point test is required. I'm going to add this.
------
Regards,
Alexander Korotkov
Supabase
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-20 22:09 Alexander Korotkov <[email protected]>
parent: Alexander Korotkov <[email protected]>
0 siblings, 2 replies; 32+ messages in thread
From: Alexander Korotkov @ 2024-10-20 22:09 UTC (permalink / raw)
To: jian he <[email protected]>; +Cc: Artur Zakirov <[email protected]>; Andrei Lepikhov <[email protected]>; 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 Sun, Oct 20, 2024 at 9:00 PM Alexander Korotkov <[email protected]> wrote:
> On Sun, Oct 20, 2024 at 8:47 PM Alexander Korotkov <[email protected]> wrote:
> > On Tue, Oct 15, 2024 at 12:50 PM jian he <[email protected]> wrote:
> > > build from source, DCLOBBER_CACHE_ALWAYS takes a very long time.
> > > So I gave up.
> > >
> > >
> > > in lookup_type_cache, we unconditionally do
> > > in_progress_list_len++;
> > > in_progress_list_len--;
> >
> > Yes, this should work OK when no errors. On error or interruption,
> > finalize_in_progress_typentries() will clean the things up.
> >
> > > "static int in_progress_list_len;"
> > > means in_progress_list_len value change is confined in
> > > src/backend/utils/cache/typcache.c.
> >
> > Yep.
> >
> > > based on above information, i am still confused with
> > > cleanup_in_progress_typentries, in_progress_list_len
> > > is there any simple sql example to demo
> > > cleanup_in_progress_typentries, in_progress_list_len> 0.
> >
> > I don't think there is simple sql to reliably reproduce that. In
> > order to hit that, we must process invalidation messages in some
> > (short) moment of time during lookup_type_cache(). You can reproduce
> > that by setting a breakpoint in lookup_type_cache() and in parallel do
> > something to invalidate the type cache entry (for instance, ALTER
> > TABLE ... ADD COLUMN ... would invalidate the composite type).
>
> Oops, concurrent invalidation message is not enough here. So,
> -DCLOBBER_CACHE_ALWAYS is also not enough to reproduce the situation.
> Injection-point test is required. I'm going to add this.
Here you go. The test with injection point is implemented.
------
Regards,
Alexander Korotkov
Supabase
Attachments:
[application/octet-stream] v15-0001-Update-header-comment-for-lookup_type_cache.patch (1.4K, ../../CAPpHfdvK8XS-FvXSxcEr-=azS3SHJo3b0yrQCq4uOv4fsw0JGw@mail.gmail.com/2-v15-0001-Update-header-comment-for-lookup_type_cache.patch)
download | inline diff:
From d624b14c9a4bd93426a5e475d4503589927ef2ae Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Fri, 13 Sep 2024 02:10:04 +0300
Subject: [PATCH v15 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.5 (Apple Git-154)
[application/octet-stream] v15-0002-Avoid-looping-over-all-type-cache-entries-in-Typ.patch (24.7K, ../../CAPpHfdvK8XS-FvXSxcEr-=azS3SHJo3b0yrQCq4uOv4fsw0JGw@mail.gmail.com/3-v15-0002-Avoid-looping-over-all-type-cache-entries-in-Typ.patch)
download | inline diff:
From 9412092fc39d2989167bb2d48606e0c27a755a5b Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Tue, 10 Sep 2024 23:25:04 +0300
Subject: [PATCH v15 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.
There are many places in lookup_type_cache() where syscache invalidation,
user interruption, or even error could occur. In order to handle this, we
keep an array of in-progress type cache entries. In the case of
lookup_type_cache() interruption this array is processed to keep
RelIdToTypeIdCacheHash in a consistent state.
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 | 359 +++++++++++++++---
src/include/utils/typcache.h | 4 +
src/test/modules/Makefile | 4 +-
src/test/modules/meson.build | 1 +
src/test/modules/typcache/.gitignore | 4 +
src/test/modules/typcache/Makefile | 28 ++
.../expected/typcache_rel_type_cache.out | 34 ++
src/test/modules/typcache/meson.build | 16 +
.../typcache/sql/typcache_rel_type_cache.sql | 18 +
src/tools/pgindent/typedefs.list | 1 +
11 files changed, 433 insertions(+), 46 deletions(-)
create mode 100644 src/test/modules/typcache/.gitignore
create mode 100644 src/test/modules/typcache/Makefile
create mode 100644 src/test/modules/typcache/expected/typcache_rel_type_cache.out
create mode 100644 src/test/modules/typcache/meson.build
create mode 100644 src/test/modules/typcache/sql/typcache_rel_type_cache.sql
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..6fa525d08f0 100644
--- a/src/backend/utils/cache/typcache.c
+++ b/src/backend/utils/cache/typcache.c
@@ -66,6 +66,7 @@
#include "utils/builtins.h"
#include "utils/catcache.h"
#include "utils/fmgroids.h"
+#include "utils/injection_point.h"
#include "utils/inval.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
@@ -77,6 +78,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 +223,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 +348,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 +387,13 @@ 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;
ctl.keysize = sizeof(Oid);
ctl.entrysize = sizeof(TypeCacheEntry);
@@ -385,6 +408,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);
@@ -394,8 +424,32 @@ lookup_type_cache(Oid type_id, int flags)
/* Also make sure CacheMemoryContext exists */
if (!CacheMemoryContext)
CreateCacheMemoryContext();
+
+ /*
+ * 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;
}
+ 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 +950,13 @@ lookup_type_cache(Oid type_id, int flags)
load_domaintype_info(typentry);
}
+ INJECTION_POINT("typecache-before-rel-type-cache-insert");
+
+ Assert(in_progress_offset + 1 == in_progress_list_len);
+ in_progress_list_len--;
+
+ insert_rel_type_cache_if_needed(typentry);
+
return typentry;
}
@@ -2290,6 +2351,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 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 +2407,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 +2467,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 +2527,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 +2538,13 @@ TypeCacheTypCallback(Datum arg, int cacheid, uint32 hashvalue)
*/
typentry->flags &= ~(TCFLAGS_HAVE_PG_TYPE_DATA |
TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS);
+
+ /*
+ * Call 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 +3053,135 @@ 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
+ }
+}
+
+/*
+ * Add possibly missing RelIdToTypeId entries related to TypeCacheHas
+ * entries, marked as in-progress by lookup_type_cache(). It may happen
+ * in case of an error or interruption during the lookup_type_cache() call.
+ */
+static void
+finalize_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);
+ if (typentry)
+ insert_rel_type_cache_if_needed(typentry);
+ }
+
+ in_progress_list_len = 0;
+}
+
+void
+AtEOXact_TypeCache(void)
+{
+ finalize_in_progress_typentries();
+}
+
+void
+AtEOSubXact_TypeCache(void)
+{
+ finalize_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/test/modules/Makefile b/src/test/modules/Makefile
index 256799f520a..c0d3cf0e14b 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -43,9 +43,9 @@ SUBDIRS = \
ifeq ($(enable_injection_points),yes)
-SUBDIRS += injection_points gin
+SUBDIRS += injection_points gin typcache
else
-ALWAYS_SUBDIRS += injection_points gin
+ALWAYS_SUBDIRS += injection_points gin typcache
endif
ifeq ($(with_ssl),openssl)
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
index d8fe059d236..c829b619530 100644
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -36,6 +36,7 @@ subdir('test_rls_hooks')
subdir('test_shm_mq')
subdir('test_slru')
subdir('test_tidstore')
+subdir('typcache')
subdir('unsafe_tests')
subdir('worker_spi')
subdir('xid_wraparound')
diff --git a/src/test/modules/typcache/.gitignore b/src/test/modules/typcache/.gitignore
new file mode 100644
index 00000000000..5dcb3ff9723
--- /dev/null
+++ b/src/test/modules/typcache/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/typcache/Makefile b/src/test/modules/typcache/Makefile
new file mode 100644
index 00000000000..6ee46ec0891
--- /dev/null
+++ b/src/test/modules/typcache/Makefile
@@ -0,0 +1,28 @@
+# src/test/modules/typcache/Makefile
+
+EXTRA_INSTALL = src/test/modules/typcache_rel_type_cache.out.out
+
+REGRESS = typcache_rel_type_cache
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/typcache
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+
+# XXX: This test is conditional on enable_injection_points in the
+# parent Makefile, so we should never get here in the first place if
+# injection points are not enabled. But the buildfarm 'misc-check'
+# step doesn't pay attention to the if-condition in the parent
+# Makefile. To work around that, disable running the test here too.
+ifeq ($(enable_injection_points),yes)
+include $(top_srcdir)/contrib/contrib-global.mk
+else
+check:
+ @echo "injection points are disabled in this build"
+endif
+
+endif
diff --git a/src/test/modules/typcache/expected/typcache_rel_type_cache.out b/src/test/modules/typcache/expected/typcache_rel_type_cache.out
new file mode 100644
index 00000000000..b113e0bbd5d
--- /dev/null
+++ b/src/test/modules/typcache/expected/typcache_rel_type_cache.out
@@ -0,0 +1,34 @@
+--
+-- This test checks that lookup_type_cache() can correctly handle an
+-- interruption. We use the injection point to simulate an error but note
+-- that a similar situation could happen due to user query interruption.
+-- Despite the interruption, a map entry from the relation oid to type cache
+-- entry should be created. This is validated by subsequent modification of
+-- the table schema, then type casts which use new schema implying
+-- successful type cache invalidation by relation oid.
+--
+CREATE EXTENSION injection_points;
+CREATE TABLE t (i int);
+SELECT injection_points_attach('typecache-before-rel-type-cache-insert', 'error');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+SELECT '(1)'::t;
+ERROR: error triggered for injection point typecache-before-rel-type-cache-insert
+LINE 1: SELECT '(1)'::t;
+ ^
+SELECT injection_points_detach('typecache-before-rel-type-cache-insert');
+ injection_points_detach
+-------------------------
+
+(1 row)
+
+ALTER TABLE t ADD COLUMN j int;
+SELECT '(1,2)'::t;
+ t
+-------
+ (1,2)
+(1 row)
+
diff --git a/src/test/modules/typcache/meson.build b/src/test/modules/typcache/meson.build
new file mode 100644
index 00000000000..cb2e34c0d2b
--- /dev/null
+++ b/src/test/modules/typcache/meson.build
@@ -0,0 +1,16 @@
+# Copyright (c) 2022-2024, PostgreSQL Global Development Group
+
+if not get_option('injection_points')
+ subdir_done()
+endif
+
+tests += {
+ 'name': 'typcache',
+ 'sd': meson.current_source_dir(),
+ 'bd': meson.current_build_dir(),
+ 'regress': {
+ 'sql': [
+ 'typcache_rel_type_cache',
+ ],
+ },
+}
diff --git a/src/test/modules/typcache/sql/typcache_rel_type_cache.sql b/src/test/modules/typcache/sql/typcache_rel_type_cache.sql
new file mode 100644
index 00000000000..2c0a434d988
--- /dev/null
+++ b/src/test/modules/typcache/sql/typcache_rel_type_cache.sql
@@ -0,0 +1,18 @@
+--
+-- This test checks that lookup_type_cache() can correctly handle an
+-- interruption. We use the injection point to simulate an error but note
+-- that a similar situation could happen due to user query interruption.
+-- Despite the interruption, a map entry from the relation oid to type cache
+-- entry should be created. This is validated by subsequent modification of
+-- the table schema, then type casts which use new schema implying
+-- successful type cache invalidation by relation oid.
+--
+
+CREATE EXTENSION injection_points;
+
+CREATE TABLE t (i int);
+SELECT injection_points_attach('typecache-before-rel-type-cache-insert', 'error');
+SELECT '(1)'::t;
+SELECT injection_points_detach('typecache-before-rel-type-cache-insert');
+ALTER TABLE t ADD COLUMN j int;
+SELECT '(1,2)'::t;
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 57de1acff3a..9b3e7fd104b 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.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-20 23:32 Dagfinn Ilmari Mannsåker <[email protected]>
parent: Alexander Korotkov <[email protected]>
1 sibling, 1 reply; 32+ messages in thread
From: Dagfinn Ilmari Mannsåker @ 2024-10-20 23:32 UTC (permalink / raw)
To: Alexander Korotkov <[email protected]>; +Cc: jian he <[email protected]>; Artur Zakirov <[email protected]>; Andrei Lepikhov <[email protected]>; 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]>
Alexander Korotkov <[email protected]> writes:
> +static Oid *in_progress_list;
> +static int in_progress_list_len;
> +static int in_progress_list_maxlen;
Is there any particular reason not to use pg_list.h for this?
- ilmari
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-21 05:36 Andrei Lepikhov <[email protected]>
parent: Alexander Korotkov <[email protected]>
0 siblings, 0 replies; 32+ messages in thread
From: Andrei Lepikhov @ 2024-10-21 05:36 UTC (permalink / raw)
To: Alexander Korotkov <[email protected]>; +Cc: Artur Zakirov <[email protected]>; Alexander Lakhin <[email protected]>; jian he <[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 21/10/2024 00:36, Alexander Korotkov wrote:
> On Thu, Oct 17, 2024 at 12:41 PM Andrei Lepikhov <[email protected]> wrote:
>> I think the first patch may already be committed, and this little burden
>> may be avoided in future versions.
> I've integrated your patch. But I think
> finalize_in_progress_typentries() is more appropriate place to check
> typentry for NULL. Also, I've revised the
> finalize_in_progress_typentries() header comment. I'm going to push
> these patches if no objections.
I agree with your idea. Also, I think it would be more conventional not
to check the type entry for a NULL value but to test the 'found' value
instead.
And thanks for the injection points tests! I must have slipped my mind
about this option.
Now, the patch set looks good for me.
--
regards, Andrei Lepikhov
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-21 05:40 Andrei Lepikhov <[email protected]>
parent: Dagfinn Ilmari Mannsåker <[email protected]>
0 siblings, 1 reply; 32+ messages in thread
From: Andrei Lepikhov @ 2024-10-21 05:40 UTC (permalink / raw)
To: Dagfinn Ilmari Mannsåker <[email protected]>; Alexander Korotkov <[email protected]>; +Cc: jian he <[email protected]>; Artur Zakirov <[email protected]>; 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 21/10/2024 06:32, Dagfinn Ilmari Mannsåker wrote:
> Alexander Korotkov <[email protected]> writes:
>
>> +static Oid *in_progress_list;
>> +static int in_progress_list_len;
>> +static int in_progress_list_maxlen;
>
> Is there any particular reason not to use pg_list.h for this?
Sure. The type cache lookup has to be as much optimal as possible.
Using an array and relating sequential access to it, we avoid memory
allocations and deallocations 99.9% of the time. Also, quick access to
the single element (which we will have in real life almost all of the
time) is much faster than employing list machinery.
--
regards, Andrei Lepikhov
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-21 07:51 jian he <[email protected]>
parent: Alexander Korotkov <[email protected]>
1 sibling, 1 reply; 32+ messages in thread
From: jian he @ 2024-10-21 07:51 UTC (permalink / raw)
To: Alexander Korotkov <[email protected]>; +Cc: Artur Zakirov <[email protected]>; Andrei Lepikhov <[email protected]>; 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]>
thanks for the
INJECTION_POINT("typecache-before-rel-type-cache-insert");
Now I have better understanding of the whole changes.
+/*
+ * Add possibly missing RelIdToTypeId entries related to TypeCacheHas
+ * entries, marked as in-progress by lookup_type_cache(). It may happen
+ * in case of an error or interruption during the lookup_type_cache() call.
+ */
+static void
+finalize_in_progress_typentries(void)
comment typo. "TypeCacheHas" should be "TypeCacheHash"
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-21 08:10 Alexander Korotkov <[email protected]>
parent: Andrei Lepikhov <[email protected]>
0 siblings, 1 reply; 32+ messages in thread
From: Alexander Korotkov @ 2024-10-21 08:10 UTC (permalink / raw)
To: Andrei Lepikhov <[email protected]>; +Cc: Dagfinn Ilmari Mannsåker <[email protected]>; jian he <[email protected]>; Artur Zakirov <[email protected]>; 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, Oct 21, 2024 at 8:40 AM Andrei Lepikhov <[email protected]> wrote:
>
> On 21/10/2024 06:32, Dagfinn Ilmari Mannsåker wrote:
> > Alexander Korotkov <[email protected]> writes:
> >
> >> +static Oid *in_progress_list;
> >> +static int in_progress_list_len;
> >> +static int in_progress_list_maxlen;
> >
> > Is there any particular reason not to use pg_list.h for this?
> Sure. The type cache lookup has to be as much optimal as possible.
> Using an array and relating sequential access to it, we avoid memory
> allocations and deallocations 99.9% of the time. Also, quick access to
> the single element (which we will have in real life almost all of the
> time) is much faster than employing list machinery.
+1,
List with zero elements has to be NIL. That means continuous
allocations/deallocations.
------
Regards,
Alexander Korotkov
Supabase
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-21 08:11 Alexander Korotkov <[email protected]>
parent: jian he <[email protected]>
0 siblings, 0 replies; 32+ messages in thread
From: Alexander Korotkov @ 2024-10-21 08:11 UTC (permalink / raw)
To: jian he <[email protected]>; +Cc: Artur Zakirov <[email protected]>; Andrei Lepikhov <[email protected]>; 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, Oct 21, 2024 at 10:51 AM jian he <[email protected]> wrote:
>
> thanks for the
> INJECTION_POINT("typecache-before-rel-type-cache-insert");
> Now I have better understanding of the whole changes.
>
>
> +/*
> + * Add possibly missing RelIdToTypeId entries related to TypeCacheHas
> + * entries, marked as in-progress by lookup_type_cache(). It may happen
> + * in case of an error or interruption during the lookup_type_cache() call.
> + */
> +static void
> +finalize_in_progress_typentries(void)
> comment typo. "TypeCacheHas" should be "TypeCacheHash"
Thank you. This also has been spotted by Alexander Lakhin (off-list).
Fixed in the attached revision of the patchset.
------
Regards,
Alexander Korotkov
Supabase
Attachments:
[application/octet-stream] v16-0001-Update-header-comment-for-lookup_type_cache.patch (1.4K, ../../CAPpHfdsk+KfUTS2bUu+=eEJjPjz=mY_manKCYVMS6khvn_MStA@mail.gmail.com/2-v16-0001-Update-header-comment-for-lookup_type_cache.patch)
download | inline diff:
From d624b14c9a4bd93426a5e475d4503589927ef2ae Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Fri, 13 Sep 2024 02:10:04 +0300
Subject: [PATCH v16 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.5 (Apple Git-154)
[application/octet-stream] v16-0002-Avoid-looping-over-all-type-cache-entries-in-Typ.patch (24.7K, ../../CAPpHfdsk+KfUTS2bUu+=eEJjPjz=mY_manKCYVMS6khvn_MStA@mail.gmail.com/3-v16-0002-Avoid-looping-over-all-type-cache-entries-in-Typ.patch)
download | inline diff:
From e126267c3a6babbc5d37924947ba8f7ec9120cec Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Tue, 10 Sep 2024 23:25:04 +0300
Subject: [PATCH v16 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.
There are many places in lookup_type_cache() where syscache invalidation,
user interruption, or even error could occur. In order to handle this, we
keep an array of in-progress type cache entries. In the case of
lookup_type_cache() interruption this array is processed to keep
RelIdToTypeIdCacheHash in a consistent state.
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, Jian He, Alexander Lakhin
---
src/backend/access/transam/xact.c | 10 +
src/backend/utils/cache/typcache.c | 359 +++++++++++++++---
src/include/utils/typcache.h | 4 +
src/test/modules/Makefile | 4 +-
src/test/modules/meson.build | 1 +
src/test/modules/typcache/.gitignore | 4 +
src/test/modules/typcache/Makefile | 28 ++
.../expected/typcache_rel_type_cache.out | 34 ++
src/test/modules/typcache/meson.build | 16 +
.../typcache/sql/typcache_rel_type_cache.sql | 18 +
src/tools/pgindent/typedefs.list | 1 +
11 files changed, 433 insertions(+), 46 deletions(-)
create mode 100644 src/test/modules/typcache/.gitignore
create mode 100644 src/test/modules/typcache/Makefile
create mode 100644 src/test/modules/typcache/expected/typcache_rel_type_cache.out
create mode 100644 src/test/modules/typcache/meson.build
create mode 100644 src/test/modules/typcache/sql/typcache_rel_type_cache.sql
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..094d3ca00c1 100644
--- a/src/backend/utils/cache/typcache.c
+++ b/src/backend/utils/cache/typcache.c
@@ -66,6 +66,7 @@
#include "utils/builtins.h"
#include "utils/catcache.h"
#include "utils/fmgroids.h"
+#include "utils/injection_point.h"
#include "utils/inval.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
@@ -77,6 +78,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 +223,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 +348,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 +387,13 @@ 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;
ctl.keysize = sizeof(Oid);
ctl.entrysize = sizeof(TypeCacheEntry);
@@ -385,6 +408,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);
@@ -394,8 +424,32 @@ lookup_type_cache(Oid type_id, int flags)
/* Also make sure CacheMemoryContext exists */
if (!CacheMemoryContext)
CreateCacheMemoryContext();
+
+ /*
+ * 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;
}
+ 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 +950,13 @@ lookup_type_cache(Oid type_id, int flags)
load_domaintype_info(typentry);
}
+ INJECTION_POINT("typecache-before-rel-type-cache-insert");
+
+ Assert(in_progress_offset + 1 == in_progress_list_len);
+ in_progress_list_len--;
+
+ insert_rel_type_cache_if_needed(typentry);
+
return typentry;
}
@@ -2290,6 +2351,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 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 +2407,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 +2467,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 +2527,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 +2538,13 @@ TypeCacheTypCallback(Datum arg, int cacheid, uint32 hashvalue)
*/
typentry->flags &= ~(TCFLAGS_HAVE_PG_TYPE_DATA |
TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS);
+
+ /*
+ * Call 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 +3053,135 @@ 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
+ }
+}
+
+/*
+ * Add possibly missing RelIdToTypeId entries related to TypeCacheHash
+ * entries, marked as in-progress by lookup_type_cache(). It may happen
+ * in case of an error or interruption during the lookup_type_cache() call.
+ */
+static void
+finalize_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);
+ if (typentry)
+ insert_rel_type_cache_if_needed(typentry);
+ }
+
+ in_progress_list_len = 0;
+}
+
+void
+AtEOXact_TypeCache(void)
+{
+ finalize_in_progress_typentries();
+}
+
+void
+AtEOSubXact_TypeCache(void)
+{
+ finalize_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/test/modules/Makefile b/src/test/modules/Makefile
index 256799f520a..c0d3cf0e14b 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -43,9 +43,9 @@ SUBDIRS = \
ifeq ($(enable_injection_points),yes)
-SUBDIRS += injection_points gin
+SUBDIRS += injection_points gin typcache
else
-ALWAYS_SUBDIRS += injection_points gin
+ALWAYS_SUBDIRS += injection_points gin typcache
endif
ifeq ($(with_ssl),openssl)
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
index d8fe059d236..c829b619530 100644
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -36,6 +36,7 @@ subdir('test_rls_hooks')
subdir('test_shm_mq')
subdir('test_slru')
subdir('test_tidstore')
+subdir('typcache')
subdir('unsafe_tests')
subdir('worker_spi')
subdir('xid_wraparound')
diff --git a/src/test/modules/typcache/.gitignore b/src/test/modules/typcache/.gitignore
new file mode 100644
index 00000000000..5dcb3ff9723
--- /dev/null
+++ b/src/test/modules/typcache/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/typcache/Makefile b/src/test/modules/typcache/Makefile
new file mode 100644
index 00000000000..6ee46ec0891
--- /dev/null
+++ b/src/test/modules/typcache/Makefile
@@ -0,0 +1,28 @@
+# src/test/modules/typcache/Makefile
+
+EXTRA_INSTALL = src/test/modules/typcache_rel_type_cache.out.out
+
+REGRESS = typcache_rel_type_cache
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/typcache
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+
+# XXX: This test is conditional on enable_injection_points in the
+# parent Makefile, so we should never get here in the first place if
+# injection points are not enabled. But the buildfarm 'misc-check'
+# step doesn't pay attention to the if-condition in the parent
+# Makefile. To work around that, disable running the test here too.
+ifeq ($(enable_injection_points),yes)
+include $(top_srcdir)/contrib/contrib-global.mk
+else
+check:
+ @echo "injection points are disabled in this build"
+endif
+
+endif
diff --git a/src/test/modules/typcache/expected/typcache_rel_type_cache.out b/src/test/modules/typcache/expected/typcache_rel_type_cache.out
new file mode 100644
index 00000000000..b113e0bbd5d
--- /dev/null
+++ b/src/test/modules/typcache/expected/typcache_rel_type_cache.out
@@ -0,0 +1,34 @@
+--
+-- This test checks that lookup_type_cache() can correctly handle an
+-- interruption. We use the injection point to simulate an error but note
+-- that a similar situation could happen due to user query interruption.
+-- Despite the interruption, a map entry from the relation oid to type cache
+-- entry should be created. This is validated by subsequent modification of
+-- the table schema, then type casts which use new schema implying
+-- successful type cache invalidation by relation oid.
+--
+CREATE EXTENSION injection_points;
+CREATE TABLE t (i int);
+SELECT injection_points_attach('typecache-before-rel-type-cache-insert', 'error');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+SELECT '(1)'::t;
+ERROR: error triggered for injection point typecache-before-rel-type-cache-insert
+LINE 1: SELECT '(1)'::t;
+ ^
+SELECT injection_points_detach('typecache-before-rel-type-cache-insert');
+ injection_points_detach
+-------------------------
+
+(1 row)
+
+ALTER TABLE t ADD COLUMN j int;
+SELECT '(1,2)'::t;
+ t
+-------
+ (1,2)
+(1 row)
+
diff --git a/src/test/modules/typcache/meson.build b/src/test/modules/typcache/meson.build
new file mode 100644
index 00000000000..cb2e34c0d2b
--- /dev/null
+++ b/src/test/modules/typcache/meson.build
@@ -0,0 +1,16 @@
+# Copyright (c) 2022-2024, PostgreSQL Global Development Group
+
+if not get_option('injection_points')
+ subdir_done()
+endif
+
+tests += {
+ 'name': 'typcache',
+ 'sd': meson.current_source_dir(),
+ 'bd': meson.current_build_dir(),
+ 'regress': {
+ 'sql': [
+ 'typcache_rel_type_cache',
+ ],
+ },
+}
diff --git a/src/test/modules/typcache/sql/typcache_rel_type_cache.sql b/src/test/modules/typcache/sql/typcache_rel_type_cache.sql
new file mode 100644
index 00000000000..2c0a434d988
--- /dev/null
+++ b/src/test/modules/typcache/sql/typcache_rel_type_cache.sql
@@ -0,0 +1,18 @@
+--
+-- This test checks that lookup_type_cache() can correctly handle an
+-- interruption. We use the injection point to simulate an error but note
+-- that a similar situation could happen due to user query interruption.
+-- Despite the interruption, a map entry from the relation oid to type cache
+-- entry should be created. This is validated by subsequent modification of
+-- the table schema, then type casts which use new schema implying
+-- successful type cache invalidation by relation oid.
+--
+
+CREATE EXTENSION injection_points;
+
+CREATE TABLE t (i int);
+SELECT injection_points_attach('typecache-before-rel-type-cache-insert', 'error');
+SELECT '(1)'::t;
+SELECT injection_points_detach('typecache-before-rel-type-cache-insert');
+ALTER TABLE t ADD COLUMN j int;
+SELECT '(1,2)'::t;
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 57de1acff3a..9b3e7fd104b 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.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-21 10:16 Dagfinn Ilmari Mannsåker <[email protected]>
parent: Alexander Korotkov <[email protected]>
0 siblings, 1 reply; 32+ messages in thread
From: Dagfinn Ilmari Mannsåker @ 2024-10-21 10:16 UTC (permalink / raw)
To: Alexander Korotkov <[email protected]>; +Cc: Andrei Lepikhov <[email protected]>; jian he <[email protected]>; Artur Zakirov <[email protected]>; 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]>
Alexander Korotkov <[email protected]> writes:
> On Mon, Oct 21, 2024 at 8:40 AM Andrei Lepikhov <[email protected]> wrote:
>>
>> On 21/10/2024 06:32, Dagfinn Ilmari Mannsåker wrote:
>> > Alexander Korotkov <[email protected]> writes:
>> >
>> >> +static Oid *in_progress_list;
>> >> +static int in_progress_list_len;
>> >> +static int in_progress_list_maxlen;
>> >
>> > Is there any particular reason not to use pg_list.h for this?
>> Sure. The type cache lookup has to be as much optimal as possible.
>> Using an array and relating sequential access to it, we avoid memory
>> allocations and deallocations 99.9% of the time. Also, quick access to
>> the single element (which we will have in real life almost all of the
>> time) is much faster than employing list machinery.
Lists are actually dynamically resized arrays these days (see commit
1cff1b95ab6ddae32faa3efe0d95a820dbfdc164), not linked lists, so
accessing arbitrary elements is O(1), not O(n). Just like this patch,
the size is doubled (starting at 16) whenever array is full.
> +1,
> List with zero elements has to be NIL. That means continuous
> allocations/deallocations.
This however is a valid point (unless we keep a dummy zeroth element to
avoid it, which is even uglier than open-coding the array extension
logic), so objection withdrawn.
- ilmari
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-21 11:30 Alexander Korotkov <[email protected]>
parent: Dagfinn Ilmari Mannsåker <[email protected]>
0 siblings, 1 reply; 32+ messages in thread
From: Alexander Korotkov @ 2024-10-21 11:30 UTC (permalink / raw)
To: Dagfinn Ilmari Mannsåker <[email protected]>; +Cc: Andrei Lepikhov <[email protected]>; jian he <[email protected]>; Artur Zakirov <[email protected]>; 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, Oct 21, 2024 at 1:16 PM Dagfinn Ilmari Mannsåker
<[email protected]> wrote:
> Alexander Korotkov <[email protected]> writes:
>
> > On Mon, Oct 21, 2024 at 8:40 AM Andrei Lepikhov <[email protected]> wrote:
> >>
> >> On 21/10/2024 06:32, Dagfinn Ilmari Mannsåker wrote:
> >> > Alexander Korotkov <[email protected]> writes:
> >> >
> >> >> +static Oid *in_progress_list;
> >> >> +static int in_progress_list_len;
> >> >> +static int in_progress_list_maxlen;
> >> >
> >> > Is there any particular reason not to use pg_list.h for this?
> >> Sure. The type cache lookup has to be as much optimal as possible.
> >> Using an array and relating sequential access to it, we avoid memory
> >> allocations and deallocations 99.9% of the time. Also, quick access to
> >> the single element (which we will have in real life almost all of the
> >> time) is much faster than employing list machinery.
>
> Lists are actually dynamically resized arrays these days (see commit
> 1cff1b95ab6ddae32faa3efe0d95a820dbfdc164), not linked lists, so
> accessing arbitrary elements is O(1), not O(n). Just like this patch,
> the size is doubled (starting at 16) whenever array is full.
>
> > +1,
> > List with zero elements has to be NIL. That means continuous
> > allocations/deallocations.
>
> This however is a valid point (unless we keep a dummy zeroth element to
> avoid it, which is even uglier than open-coding the array extension
> logic), so objection withdrawn.
OK, thank you!
The attached revision fixes EXTRA_INSTALL in
src/test/modules/typcache/Makefile. Spotted off-list by Arthur
Zakirov.
------
Regards,
Alexander Korotkov
Supabase
Attachments:
[application/octet-stream] v17-0001-Update-header-comment-for-lookup_type_cache.patch (1.4K, ../../CAPpHfduqZFZYL63Yx92pfAyiS8ACXyXSsnV1Geb_ecJTTU=Pmg@mail.gmail.com/2-v17-0001-Update-header-comment-for-lookup_type_cache.patch)
download | inline diff:
From d624b14c9a4bd93426a5e475d4503589927ef2ae Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Fri, 13 Sep 2024 02:10:04 +0300
Subject: [PATCH v17 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.5 (Apple Git-154)
[application/octet-stream] v17-0002-Avoid-looping-over-all-type-cache-entries-in-Typ.patch (24.7K, ../../CAPpHfduqZFZYL63Yx92pfAyiS8ACXyXSsnV1Geb_ecJTTU=Pmg@mail.gmail.com/3-v17-0002-Avoid-looping-over-all-type-cache-entries-in-Typ.patch)
download | inline diff:
From 41da564395bd147939d425657caacfab572a1fce Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Tue, 10 Sep 2024 23:25:04 +0300
Subject: [PATCH v17 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.
There are many places in lookup_type_cache() where syscache invalidation,
user interruption, or even error could occur. In order to handle this, we
keep an array of in-progress type cache entries. In the case of
lookup_type_cache() interruption this array is processed to keep
RelIdToTypeIdCacheHash in a consistent state.
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, Jian He, Alexander Lakhin
Reviewed-by: Artur Zakirov
---
src/backend/access/transam/xact.c | 10 +
src/backend/utils/cache/typcache.c | 359 +++++++++++++++---
src/include/utils/typcache.h | 4 +
src/test/modules/Makefile | 4 +-
src/test/modules/meson.build | 1 +
src/test/modules/typcache/.gitignore | 4 +
src/test/modules/typcache/Makefile | 28 ++
.../expected/typcache_rel_type_cache.out | 34 ++
src/test/modules/typcache/meson.build | 16 +
.../typcache/sql/typcache_rel_type_cache.sql | 18 +
src/tools/pgindent/typedefs.list | 1 +
11 files changed, 433 insertions(+), 46 deletions(-)
create mode 100644 src/test/modules/typcache/.gitignore
create mode 100644 src/test/modules/typcache/Makefile
create mode 100644 src/test/modules/typcache/expected/typcache_rel_type_cache.out
create mode 100644 src/test/modules/typcache/meson.build
create mode 100644 src/test/modules/typcache/sql/typcache_rel_type_cache.sql
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..094d3ca00c1 100644
--- a/src/backend/utils/cache/typcache.c
+++ b/src/backend/utils/cache/typcache.c
@@ -66,6 +66,7 @@
#include "utils/builtins.h"
#include "utils/catcache.h"
#include "utils/fmgroids.h"
+#include "utils/injection_point.h"
#include "utils/inval.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
@@ -77,6 +78,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 +223,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 +348,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 +387,13 @@ 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;
ctl.keysize = sizeof(Oid);
ctl.entrysize = sizeof(TypeCacheEntry);
@@ -385,6 +408,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);
@@ -394,8 +424,32 @@ lookup_type_cache(Oid type_id, int flags)
/* Also make sure CacheMemoryContext exists */
if (!CacheMemoryContext)
CreateCacheMemoryContext();
+
+ /*
+ * 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;
}
+ 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 +950,13 @@ lookup_type_cache(Oid type_id, int flags)
load_domaintype_info(typentry);
}
+ INJECTION_POINT("typecache-before-rel-type-cache-insert");
+
+ Assert(in_progress_offset + 1 == in_progress_list_len);
+ in_progress_list_len--;
+
+ insert_rel_type_cache_if_needed(typentry);
+
return typentry;
}
@@ -2290,6 +2351,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 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 +2407,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 +2467,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 +2527,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 +2538,13 @@ TypeCacheTypCallback(Datum arg, int cacheid, uint32 hashvalue)
*/
typentry->flags &= ~(TCFLAGS_HAVE_PG_TYPE_DATA |
TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS);
+
+ /*
+ * Call 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 +3053,135 @@ 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
+ }
+}
+
+/*
+ * Add possibly missing RelIdToTypeId entries related to TypeCacheHash
+ * entries, marked as in-progress by lookup_type_cache(). It may happen
+ * in case of an error or interruption during the lookup_type_cache() call.
+ */
+static void
+finalize_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);
+ if (typentry)
+ insert_rel_type_cache_if_needed(typentry);
+ }
+
+ in_progress_list_len = 0;
+}
+
+void
+AtEOXact_TypeCache(void)
+{
+ finalize_in_progress_typentries();
+}
+
+void
+AtEOSubXact_TypeCache(void)
+{
+ finalize_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/test/modules/Makefile b/src/test/modules/Makefile
index 256799f520a..c0d3cf0e14b 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -43,9 +43,9 @@ SUBDIRS = \
ifeq ($(enable_injection_points),yes)
-SUBDIRS += injection_points gin
+SUBDIRS += injection_points gin typcache
else
-ALWAYS_SUBDIRS += injection_points gin
+ALWAYS_SUBDIRS += injection_points gin typcache
endif
ifeq ($(with_ssl),openssl)
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
index d8fe059d236..c829b619530 100644
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -36,6 +36,7 @@ subdir('test_rls_hooks')
subdir('test_shm_mq')
subdir('test_slru')
subdir('test_tidstore')
+subdir('typcache')
subdir('unsafe_tests')
subdir('worker_spi')
subdir('xid_wraparound')
diff --git a/src/test/modules/typcache/.gitignore b/src/test/modules/typcache/.gitignore
new file mode 100644
index 00000000000..5dcb3ff9723
--- /dev/null
+++ b/src/test/modules/typcache/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/typcache/Makefile b/src/test/modules/typcache/Makefile
new file mode 100644
index 00000000000..1f03de83890
--- /dev/null
+++ b/src/test/modules/typcache/Makefile
@@ -0,0 +1,28 @@
+# src/test/modules/typcache/Makefile
+
+EXTRA_INSTALL = src/test/modules/injection_points
+
+REGRESS = typcache_rel_type_cache
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/typcache
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+
+# XXX: This test is conditional on enable_injection_points in the
+# parent Makefile, so we should never get here in the first place if
+# injection points are not enabled. But the buildfarm 'misc-check'
+# step doesn't pay attention to the if-condition in the parent
+# Makefile. To work around that, disable running the test here too.
+ifeq ($(enable_injection_points),yes)
+include $(top_srcdir)/contrib/contrib-global.mk
+else
+check:
+ @echo "injection points are disabled in this build"
+endif
+
+endif
diff --git a/src/test/modules/typcache/expected/typcache_rel_type_cache.out b/src/test/modules/typcache/expected/typcache_rel_type_cache.out
new file mode 100644
index 00000000000..b113e0bbd5d
--- /dev/null
+++ b/src/test/modules/typcache/expected/typcache_rel_type_cache.out
@@ -0,0 +1,34 @@
+--
+-- This test checks that lookup_type_cache() can correctly handle an
+-- interruption. We use the injection point to simulate an error but note
+-- that a similar situation could happen due to user query interruption.
+-- Despite the interruption, a map entry from the relation oid to type cache
+-- entry should be created. This is validated by subsequent modification of
+-- the table schema, then type casts which use new schema implying
+-- successful type cache invalidation by relation oid.
+--
+CREATE EXTENSION injection_points;
+CREATE TABLE t (i int);
+SELECT injection_points_attach('typecache-before-rel-type-cache-insert', 'error');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+SELECT '(1)'::t;
+ERROR: error triggered for injection point typecache-before-rel-type-cache-insert
+LINE 1: SELECT '(1)'::t;
+ ^
+SELECT injection_points_detach('typecache-before-rel-type-cache-insert');
+ injection_points_detach
+-------------------------
+
+(1 row)
+
+ALTER TABLE t ADD COLUMN j int;
+SELECT '(1,2)'::t;
+ t
+-------
+ (1,2)
+(1 row)
+
diff --git a/src/test/modules/typcache/meson.build b/src/test/modules/typcache/meson.build
new file mode 100644
index 00000000000..cb2e34c0d2b
--- /dev/null
+++ b/src/test/modules/typcache/meson.build
@@ -0,0 +1,16 @@
+# Copyright (c) 2022-2024, PostgreSQL Global Development Group
+
+if not get_option('injection_points')
+ subdir_done()
+endif
+
+tests += {
+ 'name': 'typcache',
+ 'sd': meson.current_source_dir(),
+ 'bd': meson.current_build_dir(),
+ 'regress': {
+ 'sql': [
+ 'typcache_rel_type_cache',
+ ],
+ },
+}
diff --git a/src/test/modules/typcache/sql/typcache_rel_type_cache.sql b/src/test/modules/typcache/sql/typcache_rel_type_cache.sql
new file mode 100644
index 00000000000..2c0a434d988
--- /dev/null
+++ b/src/test/modules/typcache/sql/typcache_rel_type_cache.sql
@@ -0,0 +1,18 @@
+--
+-- This test checks that lookup_type_cache() can correctly handle an
+-- interruption. We use the injection point to simulate an error but note
+-- that a similar situation could happen due to user query interruption.
+-- Despite the interruption, a map entry from the relation oid to type cache
+-- entry should be created. This is validated by subsequent modification of
+-- the table schema, then type casts which use new schema implying
+-- successful type cache invalidation by relation oid.
+--
+
+CREATE EXTENSION injection_points;
+
+CREATE TABLE t (i int);
+SELECT injection_points_attach('typecache-before-rel-type-cache-insert', 'error');
+SELECT '(1)'::t;
+SELECT injection_points_detach('typecache-before-rel-type-cache-insert');
+ALTER TABLE t ADD COLUMN j int;
+SELECT '(1,2)'::t;
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 57de1acff3a..9b3e7fd104b 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.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-22 07:34 Alexander Korotkov <[email protected]>
parent: Alexander Korotkov <[email protected]>
0 siblings, 1 reply; 32+ messages in thread
From: Alexander Korotkov @ 2024-10-22 07:34 UTC (permalink / raw)
To: Dagfinn Ilmari Mannsåker <[email protected]>; +Cc: Andrei Lepikhov <[email protected]>; jian he <[email protected]>; Artur Zakirov <[email protected]>; 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, Oct 21, 2024 at 2:30 PM Alexander Korotkov <[email protected]> wrote:
>
> On Mon, Oct 21, 2024 at 1:16 PM Dagfinn Ilmari Mannsåker
> <[email protected]> wrote:
> > Alexander Korotkov <[email protected]> writes:
> >
> > > On Mon, Oct 21, 2024 at 8:40 AM Andrei Lepikhov <[email protected]> wrote:
> > >>
> > >> On 21/10/2024 06:32, Dagfinn Ilmari Mannsåker wrote:
> > >> > Alexander Korotkov <[email protected]> writes:
> > >> >
> > >> >> +static Oid *in_progress_list;
> > >> >> +static int in_progress_list_len;
> > >> >> +static int in_progress_list_maxlen;
> > >> >
> > >> > Is there any particular reason not to use pg_list.h for this?
> > >> Sure. The type cache lookup has to be as much optimal as possible.
> > >> Using an array and relating sequential access to it, we avoid memory
> > >> allocations and deallocations 99.9% of the time. Also, quick access to
> > >> the single element (which we will have in real life almost all of the
> > >> time) is much faster than employing list machinery.
> >
> > Lists are actually dynamically resized arrays these days (see commit
> > 1cff1b95ab6ddae32faa3efe0d95a820dbfdc164), not linked lists, so
> > accessing arbitrary elements is O(1), not O(n). Just like this patch,
> > the size is doubled (starting at 16) whenever array is full.
> >
> > > +1,
> > > List with zero elements has to be NIL. That means continuous
> > > allocations/deallocations.
> >
> > This however is a valid point (unless we keep a dummy zeroth element to
> > avoid it, which is even uglier than open-coding the array extension
> > logic), so objection withdrawn.
>
> OK, thank you!
>
> The attached revision fixes EXTRA_INSTALL in
> src/test/modules/typcache/Makefile. Spotted off-list by Arthur
> Zakirov.
I've re-checked that regression tests pass with
-DCLOBBER_CACHE_ALWAYS. Also did some grammar corrections for
comments and commit message. I'm going to push this if no objections.
------
Regards,
Alexander Korotkov
Supabase
Attachments:
[application/octet-stream] v18-0001-Update-header-comment-for-lookup_type_cache.patch (1.4K, ../../CAPpHfdtHjPuLPoLGAqeUnttxy-PooJNB4piiZraLXK2C8e8vwg@mail.gmail.com/2-v18-0001-Update-header-comment-for-lookup_type_cache.patch)
download | inline diff:
From ab098661c407355c07aacb7821221bfcbf10637b Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Tue, 22 Oct 2024 10:30:40 +0300
Subject: [PATCH v18 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.5 (Apple Git-154)
[application/octet-stream] v18-0002-Avoid-looping-over-all-type-cache-entries-in-Typ.patch (24.7K, ../../CAPpHfdtHjPuLPoLGAqeUnttxy-PooJNB4piiZraLXK2C8e8vwg@mail.gmail.com/3-v18-0002-Avoid-looping-over-all-type-cache-entries-in-Typ.patch)
download | inline diff:
From aba5740d647bc16aa4bbe5cd82d59bd7b8df7ee3 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Tue, 22 Oct 2024 10:30:46 +0300
Subject: [PATCH v18 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.
There are many places in lookup_type_cache() where syscache invalidation,
user interruption, or even error could occur. In order to handle this, we
keep an array of in-progress type cache entries. In the case of
lookup_type_cache() interruption this array is processed to keep
RelIdToTypeIdCacheHash in a consistent state.
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, Jian He, Alexander Lakhin
Reviewed-by: Artur Zakirov
---
src/backend/access/transam/xact.c | 10 +
src/backend/utils/cache/typcache.c | 359 +++++++++++++++---
src/include/utils/typcache.h | 4 +
src/test/modules/Makefile | 4 +-
src/test/modules/meson.build | 1 +
src/test/modules/typcache/.gitignore | 4 +
src/test/modules/typcache/Makefile | 28 ++
.../expected/typcache_rel_type_cache.out | 34 ++
src/test/modules/typcache/meson.build | 16 +
.../typcache/sql/typcache_rel_type_cache.sql | 18 +
src/tools/pgindent/typedefs.list | 1 +
11 files changed, 433 insertions(+), 46 deletions(-)
create mode 100644 src/test/modules/typcache/.gitignore
create mode 100644 src/test/modules/typcache/Makefile
create mode 100644 src/test/modules/typcache/expected/typcache_rel_type_cache.out
create mode 100644 src/test/modules/typcache/meson.build
create mode 100644 src/test/modules/typcache/sql/typcache_rel_type_cache.sql
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..2037e2f1c16 100644
--- a/src/backend/utils/cache/typcache.c
+++ b/src/backend/utils/cache/typcache.c
@@ -66,6 +66,7 @@
#include "utils/builtins.h"
#include "utils/catcache.h"
#include "utils/fmgroids.h"
+#include "utils/injection_point.h"
#include "utils/inval.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
@@ -77,6 +78,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 +223,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 +348,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 +387,13 @@ 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;
ctl.keysize = sizeof(Oid);
ctl.entrysize = sizeof(TypeCacheEntry);
@@ -385,6 +408,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);
@@ -394,8 +424,32 @@ lookup_type_cache(Oid type_id, int flags)
/* Also make sure CacheMemoryContext exists */
if (!CacheMemoryContext)
CreateCacheMemoryContext();
+
+ /*
+ * 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;
}
+ 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 +950,13 @@ lookup_type_cache(Oid type_id, int flags)
load_domaintype_info(typentry);
}
+ INJECTION_POINT("typecache-before-rel-type-cache-insert");
+
+ Assert(in_progress_offset + 1 == in_progress_list_len);
+ in_progress_list_len--;
+
+ insert_rel_type_cache_if_needed(typentry);
+
return typentry;
}
@@ -2290,6 +2351,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 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 +2407,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 +2467,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 +2527,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 +2538,13 @@ TypeCacheTypCallback(Datum arg, int cacheid, uint32 hashvalue)
*/
typentry->flags &= ~(TCFLAGS_HAVE_PG_TYPE_DATA |
TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS);
+
+ /*
+ * Call 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 +3053,135 @@ 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,
+ * 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
+ }
+}
+
+/*
+ * Add possibly missing RelIdToTypeId entries related to TypeCacheHash
+ * entries, marked as in-progress by lookup_type_cache(). It may happen
+ * in case of an error or interruption during the lookup_type_cache() call.
+ */
+static void
+finalize_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);
+ if (typentry)
+ insert_rel_type_cache_if_needed(typentry);
+ }
+
+ in_progress_list_len = 0;
+}
+
+void
+AtEOXact_TypeCache(void)
+{
+ finalize_in_progress_typentries();
+}
+
+void
+AtEOSubXact_TypeCache(void)
+{
+ finalize_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/test/modules/Makefile b/src/test/modules/Makefile
index 256799f520a..c0d3cf0e14b 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -43,9 +43,9 @@ SUBDIRS = \
ifeq ($(enable_injection_points),yes)
-SUBDIRS += injection_points gin
+SUBDIRS += injection_points gin typcache
else
-ALWAYS_SUBDIRS += injection_points gin
+ALWAYS_SUBDIRS += injection_points gin typcache
endif
ifeq ($(with_ssl),openssl)
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
index d8fe059d236..c829b619530 100644
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -36,6 +36,7 @@ subdir('test_rls_hooks')
subdir('test_shm_mq')
subdir('test_slru')
subdir('test_tidstore')
+subdir('typcache')
subdir('unsafe_tests')
subdir('worker_spi')
subdir('xid_wraparound')
diff --git a/src/test/modules/typcache/.gitignore b/src/test/modules/typcache/.gitignore
new file mode 100644
index 00000000000..5dcb3ff9723
--- /dev/null
+++ b/src/test/modules/typcache/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/typcache/Makefile b/src/test/modules/typcache/Makefile
new file mode 100644
index 00000000000..1f03de83890
--- /dev/null
+++ b/src/test/modules/typcache/Makefile
@@ -0,0 +1,28 @@
+# src/test/modules/typcache/Makefile
+
+EXTRA_INSTALL = src/test/modules/injection_points
+
+REGRESS = typcache_rel_type_cache
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/typcache
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+
+# XXX: This test is conditional on enable_injection_points in the
+# parent Makefile, so we should never get here in the first place if
+# injection points are not enabled. But the buildfarm 'misc-check'
+# step doesn't pay attention to the if-condition in the parent
+# Makefile. To work around that, disable running the test here too.
+ifeq ($(enable_injection_points),yes)
+include $(top_srcdir)/contrib/contrib-global.mk
+else
+check:
+ @echo "injection points are disabled in this build"
+endif
+
+endif
diff --git a/src/test/modules/typcache/expected/typcache_rel_type_cache.out b/src/test/modules/typcache/expected/typcache_rel_type_cache.out
new file mode 100644
index 00000000000..b113e0bbd5d
--- /dev/null
+++ b/src/test/modules/typcache/expected/typcache_rel_type_cache.out
@@ -0,0 +1,34 @@
+--
+-- This test checks that lookup_type_cache() can correctly handle an
+-- interruption. We use the injection point to simulate an error but note
+-- that a similar situation could happen due to user query interruption.
+-- Despite the interruption, a map entry from the relation oid to type cache
+-- entry should be created. This is validated by subsequent modification of
+-- the table schema, then type casts which use new schema implying
+-- successful type cache invalidation by relation oid.
+--
+CREATE EXTENSION injection_points;
+CREATE TABLE t (i int);
+SELECT injection_points_attach('typecache-before-rel-type-cache-insert', 'error');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+SELECT '(1)'::t;
+ERROR: error triggered for injection point typecache-before-rel-type-cache-insert
+LINE 1: SELECT '(1)'::t;
+ ^
+SELECT injection_points_detach('typecache-before-rel-type-cache-insert');
+ injection_points_detach
+-------------------------
+
+(1 row)
+
+ALTER TABLE t ADD COLUMN j int;
+SELECT '(1,2)'::t;
+ t
+-------
+ (1,2)
+(1 row)
+
diff --git a/src/test/modules/typcache/meson.build b/src/test/modules/typcache/meson.build
new file mode 100644
index 00000000000..cb2e34c0d2b
--- /dev/null
+++ b/src/test/modules/typcache/meson.build
@@ -0,0 +1,16 @@
+# Copyright (c) 2022-2024, PostgreSQL Global Development Group
+
+if not get_option('injection_points')
+ subdir_done()
+endif
+
+tests += {
+ 'name': 'typcache',
+ 'sd': meson.current_source_dir(),
+ 'bd': meson.current_build_dir(),
+ 'regress': {
+ 'sql': [
+ 'typcache_rel_type_cache',
+ ],
+ },
+}
diff --git a/src/test/modules/typcache/sql/typcache_rel_type_cache.sql b/src/test/modules/typcache/sql/typcache_rel_type_cache.sql
new file mode 100644
index 00000000000..2c0a434d988
--- /dev/null
+++ b/src/test/modules/typcache/sql/typcache_rel_type_cache.sql
@@ -0,0 +1,18 @@
+--
+-- This test checks that lookup_type_cache() can correctly handle an
+-- interruption. We use the injection point to simulate an error but note
+-- that a similar situation could happen due to user query interruption.
+-- Despite the interruption, a map entry from the relation oid to type cache
+-- entry should be created. This is validated by subsequent modification of
+-- the table schema, then type casts which use new schema implying
+-- successful type cache invalidation by relation oid.
+--
+
+CREATE EXTENSION injection_points;
+
+CREATE TABLE t (i int);
+SELECT injection_points_attach('typecache-before-rel-type-cache-insert', 'error');
+SELECT '(1)'::t;
+SELECT injection_points_detach('typecache-before-rel-type-cache-insert');
+ALTER TABLE t ADD COLUMN j int;
+SELECT '(1,2)'::t;
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 57de1acff3a..9b3e7fd104b 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.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-22 15:09 Pavel Borisov <[email protected]>
parent: Alexander Korotkov <[email protected]>
0 siblings, 1 reply; 32+ messages in thread
From: Pavel Borisov @ 2024-10-22 15:09 UTC (permalink / raw)
To: Alexander Korotkov <[email protected]>; +Cc: Dagfinn Ilmari Mannsåker <[email protected]>; Andrei Lepikhov <[email protected]>; jian he <[email protected]>; Artur Zakirov <[email protected]>; Alexander Lakhin <[email protected]>; Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]>
Hi, Alexander!
On Tue, 22 Oct 2024 at 11:34, Alexander Korotkov <[email protected]>
wrote:
> On Mon, Oct 21, 2024 at 2:30 PM Alexander Korotkov <[email protected]>
> wrote:
> >
> > On Mon, Oct 21, 2024 at 1:16 PM Dagfinn Ilmari Mannsåker
> > <[email protected]> wrote:
> > > Alexander Korotkov <[email protected]> writes:
> > >
> > > > On Mon, Oct 21, 2024 at 8:40 AM Andrei Lepikhov <[email protected]>
> wrote:
> > > >>
> > > >> On 21/10/2024 06:32, Dagfinn Ilmari Mannsåker wrote:
> > > >> > Alexander Korotkov <[email protected]> writes:
> > > >> >
> > > >> >> +static Oid *in_progress_list;
> > > >> >> +static int in_progress_list_len;
> > > >> >> +static int in_progress_list_maxlen;
> > > >> >
> > > >> > Is there any particular reason not to use pg_list.h for this?
> > > >> Sure. The type cache lookup has to be as much optimal as possible.
> > > >> Using an array and relating sequential access to it, we avoid memory
> > > >> allocations and deallocations 99.9% of the time. Also, quick access
> to
> > > >> the single element (which we will have in real life almost all of
> the
> > > >> time) is much faster than employing list machinery.
> > >
> > > Lists are actually dynamically resized arrays these days (see commit
> > > 1cff1b95ab6ddae32faa3efe0d95a820dbfdc164), not linked lists, so
> > > accessing arbitrary elements is O(1), not O(n). Just like this patch,
> > > the size is doubled (starting at 16) whenever array is full.
> > >
> > > > +1,
> > > > List with zero elements has to be NIL. That means continuous
> > > > allocations/deallocations.
> > >
> > > This however is a valid point (unless we keep a dummy zeroth element to
> > > avoid it, which is even uglier than open-coding the array extension
> > > logic), so objection withdrawn.
> >
> > OK, thank you!
> >
> > The attached revision fixes EXTRA_INSTALL in
> > src/test/modules/typcache/Makefile. Spotted off-list by Arthur
> > Zakirov.
>
> I've re-checked that regression tests pass with
> -DCLOBBER_CACHE_ALWAYS. Also did some grammar corrections for
> comments and commit message. I'm going to push this if no objections.
>
Thank you for working on this patch!
Looked through the patchset once more.
Patch 0001 (minor): "in the last" -> "after everything else" or "after
other TypeCacheEntry contents"
Patch 0002 looks ready to me.
Regards,
Pavel Borisov
Supabase
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-22 17:33 Alexander Korotkov <[email protected]>
parent: Pavel Borisov <[email protected]>
0 siblings, 2 replies; 32+ messages in thread
From: Alexander Korotkov @ 2024-10-22 17:33 UTC (permalink / raw)
To: Pavel Borisov <[email protected]>; +Cc: Dagfinn Ilmari Mannsåker <[email protected]>; Andrei Lepikhov <[email protected]>; jian he <[email protected]>; Artur Zakirov <[email protected]>; Alexander Lakhin <[email protected]>; Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]>
On Tue, Oct 22, 2024 at 6:10 PM Pavel Borisov <[email protected]> wrote:
> On Tue, 22 Oct 2024 at 11:34, Alexander Korotkov <[email protected]> wrote:
>>
>> On Mon, Oct 21, 2024 at 2:30 PM Alexander Korotkov <[email protected]> wrote:
>> >
>> > On Mon, Oct 21, 2024 at 1:16 PM Dagfinn Ilmari Mannsåker
>> > <[email protected]> wrote:
>> > > Alexander Korotkov <[email protected]> writes:
>> > >
>> > > > On Mon, Oct 21, 2024 at 8:40 AM Andrei Lepikhov <[email protected]> wrote:
>> > > >>
>> > > >> On 21/10/2024 06:32, Dagfinn Ilmari Mannsåker wrote:
>> > > >> > Alexander Korotkov <[email protected]> writes:
>> > > >> >
>> > > >> >> +static Oid *in_progress_list;
>> > > >> >> +static int in_progress_list_len;
>> > > >> >> +static int in_progress_list_maxlen;
>> > > >> >
>> > > >> > Is there any particular reason not to use pg_list.h for this?
>> > > >> Sure. The type cache lookup has to be as much optimal as possible.
>> > > >> Using an array and relating sequential access to it, we avoid memory
>> > > >> allocations and deallocations 99.9% of the time. Also, quick access to
>> > > >> the single element (which we will have in real life almost all of the
>> > > >> time) is much faster than employing list machinery.
>> > >
>> > > Lists are actually dynamically resized arrays these days (see commit
>> > > 1cff1b95ab6ddae32faa3efe0d95a820dbfdc164), not linked lists, so
>> > > accessing arbitrary elements is O(1), not O(n). Just like this patch,
>> > > the size is doubled (starting at 16) whenever array is full.
>> > >
>> > > > +1,
>> > > > List with zero elements has to be NIL. That means continuous
>> > > > allocations/deallocations.
>> > >
>> > > This however is a valid point (unless we keep a dummy zeroth element to
>> > > avoid it, which is even uglier than open-coding the array extension
>> > > logic), so objection withdrawn.
>> >
>> > OK, thank you!
>> >
>> > The attached revision fixes EXTRA_INSTALL in
>> > src/test/modules/typcache/Makefile. Spotted off-list by Arthur
>> > Zakirov.
>>
>> I've re-checked that regression tests pass with
>> -DCLOBBER_CACHE_ALWAYS. Also did some grammar corrections for
>> comments and commit message. I'm going to push this if no objections.
>
> Thank you for working on this patch!
> Looked through the patchset once more.
>
> Patch 0001 (minor): "in the last" -> "after everything else" or "after other TypeCacheEntry contents"
>
> Patch 0002 looks ready to me.
Thank you, Pavel! 0001 revised according to your suggestion.
------
Regards,
Alexander Korotkov
Supabase
Attachments:
[application/octet-stream] v19-0001-Update-header-comment-for-lookup_type_cache.patch (1.6K, ../../CAPpHfdv=WVCbzdpoTxvXzUZZEqu_pDk2YZBnEGpUCrzuHxPN6w@mail.gmail.com/2-v19-0001-Update-header-comment-for-lookup_type_cache.patch)
download | inline diff:
From 5f9c300e039854949cbd1337200b227018426c05 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Tue, 22 Oct 2024 10:30:40 +0300
Subject: [PATCH v19 1/2] Update header comment for lookup_type_cache()
Describe the way we handle concurrent invalidation messages.
Discussion: https://postgr.es/m/CAPpHfdsQhwUrnB3of862j9RgHoJM--eRbifvBMvtQxpC57dxCA%40mail.gmail.com
Reviewed-by: Andrei Lepikhov, Artur Zakirov, Pavel Borisov
---
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..f142624ad2e 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 after all other
+ * TypeCacheEntry items for TYPTYPE_COMPOSITE. So, tupdesc can't get
+ * invalidated during the lookup_type_cache() call.
*/
TypeCacheEntry *
lookup_type_cache(Oid type_id, int flags)
--
2.39.5 (Apple Git-154)
[application/octet-stream] v19-0002-Avoid-looping-over-all-type-cache-entries-in-Typ.patch (24.7K, ../../CAPpHfdv=WVCbzdpoTxvXzUZZEqu_pDk2YZBnEGpUCrzuHxPN6w@mail.gmail.com/3-v19-0002-Avoid-looping-over-all-type-cache-entries-in-Typ.patch)
download | inline diff:
From af2a07a7cc52f3335c939e0ed84bcca70a50b885 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Tue, 22 Oct 2024 10:30:46 +0300
Subject: [PATCH v19 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.
There are many places in lookup_type_cache() where syscache invalidation,
user interruption, or even error could occur. In order to handle this, we
keep an array of in-progress type cache entries. In the case of
lookup_type_cache() interruption this array is processed to keep
RelIdToTypeIdCacheHash in a consistent state.
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, Jian He, Alexander Lakhin
Reviewed-by: Artur Zakirov
---
src/backend/access/transam/xact.c | 10 +
src/backend/utils/cache/typcache.c | 359 +++++++++++++++---
src/include/utils/typcache.h | 4 +
src/test/modules/Makefile | 4 +-
src/test/modules/meson.build | 1 +
src/test/modules/typcache/.gitignore | 4 +
src/test/modules/typcache/Makefile | 28 ++
.../expected/typcache_rel_type_cache.out | 34 ++
src/test/modules/typcache/meson.build | 16 +
.../typcache/sql/typcache_rel_type_cache.sql | 18 +
src/tools/pgindent/typedefs.list | 1 +
11 files changed, 433 insertions(+), 46 deletions(-)
create mode 100644 src/test/modules/typcache/.gitignore
create mode 100644 src/test/modules/typcache/Makefile
create mode 100644 src/test/modules/typcache/expected/typcache_rel_type_cache.out
create mode 100644 src/test/modules/typcache/meson.build
create mode 100644 src/test/modules/typcache/sql/typcache_rel_type_cache.sql
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 f142624ad2e..1972bd1944b 100644
--- a/src/backend/utils/cache/typcache.c
+++ b/src/backend/utils/cache/typcache.c
@@ -66,6 +66,7 @@
#include "utils/builtins.h"
#include "utils/catcache.h"
#include "utils/fmgroids.h"
+#include "utils/injection_point.h"
#include "utils/inval.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
@@ -77,6 +78,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 +223,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 +348,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 +387,13 @@ 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;
ctl.keysize = sizeof(Oid);
ctl.entrysize = sizeof(TypeCacheEntry);
@@ -385,6 +408,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);
@@ -394,8 +424,32 @@ lookup_type_cache(Oid type_id, int flags)
/* Also make sure CacheMemoryContext exists */
if (!CacheMemoryContext)
CreateCacheMemoryContext();
+
+ /*
+ * 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;
}
+ 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 +950,13 @@ lookup_type_cache(Oid type_id, int flags)
load_domaintype_info(typentry);
}
+ INJECTION_POINT("typecache-before-rel-type-cache-insert");
+
+ Assert(in_progress_offset + 1 == in_progress_list_len);
+ in_progress_list_len--;
+
+ insert_rel_type_cache_if_needed(typentry);
+
return typentry;
}
@@ -2290,6 +2351,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 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 +2407,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 +2467,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 +2527,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 +2538,13 @@ TypeCacheTypCallback(Datum arg, int cacheid, uint32 hashvalue)
*/
typentry->flags &= ~(TCFLAGS_HAVE_PG_TYPE_DATA |
TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS);
+
+ /*
+ * Call 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 +3053,135 @@ 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,
+ * 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
+ }
+}
+
+/*
+ * Add possibly missing RelIdToTypeId entries related to TypeCacheHash
+ * entries, marked as in-progress by lookup_type_cache(). It may happen
+ * in case of an error or interruption during the lookup_type_cache() call.
+ */
+static void
+finalize_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);
+ if (typentry)
+ insert_rel_type_cache_if_needed(typentry);
+ }
+
+ in_progress_list_len = 0;
+}
+
+void
+AtEOXact_TypeCache(void)
+{
+ finalize_in_progress_typentries();
+}
+
+void
+AtEOSubXact_TypeCache(void)
+{
+ finalize_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/test/modules/Makefile b/src/test/modules/Makefile
index 256799f520a..c0d3cf0e14b 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -43,9 +43,9 @@ SUBDIRS = \
ifeq ($(enable_injection_points),yes)
-SUBDIRS += injection_points gin
+SUBDIRS += injection_points gin typcache
else
-ALWAYS_SUBDIRS += injection_points gin
+ALWAYS_SUBDIRS += injection_points gin typcache
endif
ifeq ($(with_ssl),openssl)
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
index d8fe059d236..c829b619530 100644
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -36,6 +36,7 @@ subdir('test_rls_hooks')
subdir('test_shm_mq')
subdir('test_slru')
subdir('test_tidstore')
+subdir('typcache')
subdir('unsafe_tests')
subdir('worker_spi')
subdir('xid_wraparound')
diff --git a/src/test/modules/typcache/.gitignore b/src/test/modules/typcache/.gitignore
new file mode 100644
index 00000000000..5dcb3ff9723
--- /dev/null
+++ b/src/test/modules/typcache/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/typcache/Makefile b/src/test/modules/typcache/Makefile
new file mode 100644
index 00000000000..1f03de83890
--- /dev/null
+++ b/src/test/modules/typcache/Makefile
@@ -0,0 +1,28 @@
+# src/test/modules/typcache/Makefile
+
+EXTRA_INSTALL = src/test/modules/injection_points
+
+REGRESS = typcache_rel_type_cache
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/typcache
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+
+# XXX: This test is conditional on enable_injection_points in the
+# parent Makefile, so we should never get here in the first place if
+# injection points are not enabled. But the buildfarm 'misc-check'
+# step doesn't pay attention to the if-condition in the parent
+# Makefile. To work around that, disable running the test here too.
+ifeq ($(enable_injection_points),yes)
+include $(top_srcdir)/contrib/contrib-global.mk
+else
+check:
+ @echo "injection points are disabled in this build"
+endif
+
+endif
diff --git a/src/test/modules/typcache/expected/typcache_rel_type_cache.out b/src/test/modules/typcache/expected/typcache_rel_type_cache.out
new file mode 100644
index 00000000000..b113e0bbd5d
--- /dev/null
+++ b/src/test/modules/typcache/expected/typcache_rel_type_cache.out
@@ -0,0 +1,34 @@
+--
+-- This test checks that lookup_type_cache() can correctly handle an
+-- interruption. We use the injection point to simulate an error but note
+-- that a similar situation could happen due to user query interruption.
+-- Despite the interruption, a map entry from the relation oid to type cache
+-- entry should be created. This is validated by subsequent modification of
+-- the table schema, then type casts which use new schema implying
+-- successful type cache invalidation by relation oid.
+--
+CREATE EXTENSION injection_points;
+CREATE TABLE t (i int);
+SELECT injection_points_attach('typecache-before-rel-type-cache-insert', 'error');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+SELECT '(1)'::t;
+ERROR: error triggered for injection point typecache-before-rel-type-cache-insert
+LINE 1: SELECT '(1)'::t;
+ ^
+SELECT injection_points_detach('typecache-before-rel-type-cache-insert');
+ injection_points_detach
+-------------------------
+
+(1 row)
+
+ALTER TABLE t ADD COLUMN j int;
+SELECT '(1,2)'::t;
+ t
+-------
+ (1,2)
+(1 row)
+
diff --git a/src/test/modules/typcache/meson.build b/src/test/modules/typcache/meson.build
new file mode 100644
index 00000000000..cb2e34c0d2b
--- /dev/null
+++ b/src/test/modules/typcache/meson.build
@@ -0,0 +1,16 @@
+# Copyright (c) 2022-2024, PostgreSQL Global Development Group
+
+if not get_option('injection_points')
+ subdir_done()
+endif
+
+tests += {
+ 'name': 'typcache',
+ 'sd': meson.current_source_dir(),
+ 'bd': meson.current_build_dir(),
+ 'regress': {
+ 'sql': [
+ 'typcache_rel_type_cache',
+ ],
+ },
+}
diff --git a/src/test/modules/typcache/sql/typcache_rel_type_cache.sql b/src/test/modules/typcache/sql/typcache_rel_type_cache.sql
new file mode 100644
index 00000000000..2c0a434d988
--- /dev/null
+++ b/src/test/modules/typcache/sql/typcache_rel_type_cache.sql
@@ -0,0 +1,18 @@
+--
+-- This test checks that lookup_type_cache() can correctly handle an
+-- interruption. We use the injection point to simulate an error but note
+-- that a similar situation could happen due to user query interruption.
+-- Despite the interruption, a map entry from the relation oid to type cache
+-- entry should be created. This is validated by subsequent modification of
+-- the table schema, then type casts which use new schema implying
+-- successful type cache invalidation by relation oid.
+--
+
+CREATE EXTENSION injection_points;
+
+CREATE TABLE t (i int);
+SELECT injection_points_attach('typecache-before-rel-type-cache-insert', 'error');
+SELECT '(1)'::t;
+SELECT injection_points_detach('typecache-before-rel-type-cache-insert');
+ALTER TABLE t ADD COLUMN j int;
+SELECT '(1,2)'::t;
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 57de1acff3a..9b3e7fd104b 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.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-25 08:35 Andres Freund <[email protected]>
parent: Alexander Korotkov <[email protected]>
1 sibling, 1 reply; 32+ messages in thread
From: Andres Freund @ 2024-10-25 08:35 UTC (permalink / raw)
To: Alexander Korotkov <[email protected]>; +Cc: Pavel Borisov <[email protected]>; Dagfinn Ilmari Mannsåker <[email protected]>; Andrei Lepikhov <[email protected]>; jian he <[email protected]>; Artur Zakirov <[email protected]>; Alexander Lakhin <[email protected]>; Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]>
Hi,
On 2024-10-22 20:33:24 +0300, Alexander Korotkov wrote:
> Thank you, Pavel! 0001 revised according to your suggestion.
Starting with this commit CI fails.
https://cirrus-ci.com/task/6668851469877248
https://api.cirrus-ci.com/v1/artifact/task/6668851469877248/testrun/build/testrun/regress-running/re...
diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/inherit.out /tmp/cirrus-ci-build/build/testrun/regress-running/regress/results/inherit.out
--- /tmp/cirrus-ci-build/src/test/regress/expected/inherit.out 2024-10-24 11:38:43.829712000 +0000
+++ /tmp/cirrus-ci-build/build/testrun/regress-running/regress/results/inherit.out 2024-10-24 11:44:57.154238000 +0000
@@ -1338,14 +1338,9 @@
ERROR: cannot drop inherited constraint "f1_pos" of relation "p1_c1"
alter table p1 drop constraint f1_pos;
\d p1_c1
- Table "public.p1_c1"
- Column | Type | Collation | Nullable | Default
---------+---------+-----------+----------+---------
- f1 | integer | | |
-Check constraints:
- "f1_pos" CHECK (f1 > 0)
-Inherits: p1
-
+ERROR: error triggered for injection point typecache-before-rel-type-cache-insert
+LINE 4: ORDER BY 1;
+ ^
drop table p1 cascade;
NOTICE: drop cascades to table p1_c1
create table p1(f1 int constraint f1_pos CHECK (f1 > 0));
Greetings,
Andres
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-25 09:48 Alexander Korotkov <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 1 reply; 32+ messages in thread
From: Alexander Korotkov @ 2024-10-25 09:48 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Pavel Borisov <[email protected]>; Dagfinn Ilmari Mannsåker <[email protected]>; Andrei Lepikhov <[email protected]>; jian he <[email protected]>; Artur Zakirov <[email protected]>; Alexander Lakhin <[email protected]>; Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]>
On Fri, Oct 25, 2024 at 11:35 AM Andres Freund <[email protected]> wrote:
> On 2024-10-22 20:33:24 +0300, Alexander Korotkov wrote:
> > Thank you, Pavel! 0001 revised according to your suggestion.
>
> Starting with this commit CI fails.
>
> https://cirrus-ci.com/task/6668851469877248
> https://api.cirrus-ci.com/v1/artifact/task/6668851469877248/testrun/build/testrun/regress-running/re...
>
> diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/inherit.out /tmp/cirrus-ci-build/build/testrun/regress-running/regress/results/inherit.out
> --- /tmp/cirrus-ci-build/src/test/regress/expected/inherit.out 2024-10-24 11:38:43.829712000 +0000
> +++ /tmp/cirrus-ci-build/build/testrun/regress-running/regress/results/inherit.out 2024-10-24 11:44:57.154238000 +0000
> @@ -1338,14 +1338,9 @@
> ERROR: cannot drop inherited constraint "f1_pos" of relation "p1_c1"
> alter table p1 drop constraint f1_pos;
> \d p1_c1
> - Table "public.p1_c1"
> - Column | Type | Collation | Nullable | Default
> ---------+---------+-----------+----------+---------
> - f1 | integer | | |
> -Check constraints:
> - "f1_pos" CHECK (f1 > 0)
> -Inherits: p1
> -
> +ERROR: error triggered for injection point typecache-before-rel-type-cache-insert
> +LINE 4: ORDER BY 1;
> + ^
> drop table p1 cascade;
> NOTICE: drop cascades to table p1_c1
> create table p1(f1 int constraint f1_pos CHECK (f1 > 0));
Thank you for reporting this.
Looks weird that injection point, which isn't used in these tests, got
triggered here.
I'm looking into this.
------
Regards,
Alexander Korotkov
Supabase
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2024-10-25 10:13 Alexander Korotkov <[email protected]>
parent: Alexander Korotkov <[email protected]>
0 siblings, 0 replies; 32+ messages in thread
From: Alexander Korotkov @ 2024-10-25 10:13 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Pavel Borisov <[email protected]>; Dagfinn Ilmari Mannsåker <[email protected]>; Andrei Lepikhov <[email protected]>; jian he <[email protected]>; Artur Zakirov <[email protected]>; Alexander Lakhin <[email protected]>; Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]>
On Fri, Oct 25, 2024 at 12:48 PM Alexander Korotkov
<[email protected]> wrote:
> On Fri, Oct 25, 2024 at 11:35 AM Andres Freund <[email protected]> wrote:
> > On 2024-10-22 20:33:24 +0300, Alexander Korotkov wrote:
> > > Thank you, Pavel! 0001 revised according to your suggestion.
> >
> > Starting with this commit CI fails.
> >
> > https://cirrus-ci.com/task/6668851469877248
> > https://api.cirrus-ci.com/v1/artifact/task/6668851469877248/testrun/build/testrun/regress-running/re...
> >
> > diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/inherit.out /tmp/cirrus-ci-build/build/testrun/regress-running/regress/results/inherit.out
> > --- /tmp/cirrus-ci-build/src/test/regress/expected/inherit.out 2024-10-24 11:38:43.829712000 +0000
> > +++ /tmp/cirrus-ci-build/build/testrun/regress-running/regress/results/inherit.out 2024-10-24 11:44:57.154238000 +0000
> > @@ -1338,14 +1338,9 @@
> > ERROR: cannot drop inherited constraint "f1_pos" of relation "p1_c1"
> > alter table p1 drop constraint f1_pos;
> > \d p1_c1
> > - Table "public.p1_c1"
> > - Column | Type | Collation | Nullable | Default
> > ---------+---------+-----------+----------+---------
> > - f1 | integer | | |
> > -Check constraints:
> > - "f1_pos" CHECK (f1 > 0)
> > -Inherits: p1
> > -
> > +ERROR: error triggered for injection point typecache-before-rel-type-cache-insert
> > +LINE 4: ORDER BY 1;
> > + ^
> > drop table p1 cascade;
> > NOTICE: drop cascades to table p1_c1
> > create table p1(f1 int constraint f1_pos CHECK (f1 > 0));
>
> Thank you for reporting this.
> Looks weird that injection point, which isn't used in these tests, got
> triggered here.
> I'm looking into this.
Oh, I forgot to make injection points in typcache_rel_type_cache.sql
local. Thus, it affects concurrent tests. Must be fixed in
aa1e898dea.
------
Regards,
Alexander Korotkov
Supabase
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2025-04-11 20:32 Noah Misch <[email protected]>
parent: Alexander Korotkov <[email protected]>
1 sibling, 1 reply; 32+ messages in thread
From: Noah Misch @ 2025-04-11 20:32 UTC (permalink / raw)
To: Alexander Korotkov <[email protected]>; +Cc: Pavel Borisov <[email protected]>; Dagfinn Ilmari Mannsåker <[email protected]>; Andrei Lepikhov <[email protected]>; jian he <[email protected]>; Artur Zakirov <[email protected]>; Alexander Lakhin <[email protected]>; Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]>
On Tue, Oct 22, 2024 at 08:33:24PM +0300, Alexander Korotkov wrote:
> On Tue, Oct 22, 2024 at 6:10 PM Pavel Borisov <[email protected]> wrote:
> > On Tue, 22 Oct 2024 at 11:34, Alexander Korotkov <[email protected]> wrote:
> >> I'm going to push this if no objections.
(This became commit b85a9d0.)
> + /* Call delete_rel_type_cache() if we actually cleared something */
> + if (hadTupDescOrOpclass)
> + delete_rel_type_cache_if_needed(typentry);
I think the intent was to maintain the invariant that a RelIdToTypeIdCacheHash
entry exists if and only if certain kinds of data appear in the TypeCacheHash
entry. However, TypeCacheOpcCallback() clears TCFLAGS_OPERATOR_FLAGS without
maintaining RelIdToTypeIdCacheHash. Is it right to do that?
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2025-04-11 21:43 Alexander Korotkov <[email protected]>
parent: Noah Misch <[email protected]>
0 siblings, 1 reply; 32+ messages in thread
From: Alexander Korotkov @ 2025-04-11 21:43 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Pavel Borisov <[email protected]>; Dagfinn Ilmari Mannsåker <[email protected]>; Andrei Lepikhov <[email protected]>; jian he <[email protected]>; Artur Zakirov <[email protected]>; Alexander Lakhin <[email protected]>; Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]>
On Fri, Apr 11, 2025 at 11:32 PM Noah Misch <[email protected]> wrote:
>
> On Tue, Oct 22, 2024 at 08:33:24PM +0300, Alexander Korotkov wrote:
> > On Tue, Oct 22, 2024 at 6:10 PM Pavel Borisov <[email protected]> wrote:
> > > On Tue, 22 Oct 2024 at 11:34, Alexander Korotkov <[email protected]> wrote:
> > >> I'm going to push this if no objections.
>
> (This became commit b85a9d0.)
>
> > + /* Call delete_rel_type_cache() if we actually cleared something */
> > + if (hadTupDescOrOpclass)
> > + delete_rel_type_cache_if_needed(typentry);
>
> I think the intent was to maintain the invariant that a RelIdToTypeIdCacheHash
> entry exists if and only if certain kinds of data appear in the TypeCacheHash
> entry. However, TypeCacheOpcCallback() clears TCFLAGS_OPERATOR_FLAGS without
> maintaining RelIdToTypeIdCacheHash. Is it right to do that?
Thank you for the question. I'll recheck this in next couple of days.
------
Regards,
Alexander Korotkov
Supabase
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2025-04-21 01:54 Alexander Korotkov <[email protected]>
parent: Alexander Korotkov <[email protected]>
0 siblings, 1 reply; 32+ messages in thread
From: Alexander Korotkov @ 2025-04-21 01:54 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Pavel Borisov <[email protected]>; Dagfinn Ilmari Mannsåker <[email protected]>; Andrei Lepikhov <[email protected]>; jian he <[email protected]>; Artur Zakirov <[email protected]>; Alexander Lakhin <[email protected]>; Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]>
Hi, Noah!
On Sat, Apr 12, 2025 at 12:43 AM Alexander Korotkov
<[email protected]> wrote:
>
> On Fri, Apr 11, 2025 at 11:32 PM Noah Misch <[email protected]> wrote:
> >
> > On Tue, Oct 22, 2024 at 08:33:24PM +0300, Alexander Korotkov wrote:
> > > On Tue, Oct 22, 2024 at 6:10 PM Pavel Borisov <[email protected]> wrote:
> > > > On Tue, 22 Oct 2024 at 11:34, Alexander Korotkov <[email protected]> wrote:
> > > >> I'm going to push this if no objections.
> >
> > (This became commit b85a9d0.)
> >
> > > + /* Call delete_rel_type_cache() if we actually cleared something */
> > > + if (hadTupDescOrOpclass)
> > > + delete_rel_type_cache_if_needed(typentry);
> >
> > I think the intent was to maintain the invariant that a RelIdToTypeIdCacheHash
> > entry exists if and only if certain kinds of data appear in the TypeCacheHash
> > entry. However, TypeCacheOpcCallback() clears TCFLAGS_OPERATOR_FLAGS without
> > maintaining RelIdToTypeIdCacheHash. Is it right to do that?
>
> Thank you for the question. I'll recheck this in next couple of days.
Sorry for the delay. Generally, your finding is correct. But, I
didn't manage to reproduce the situation, where existing code leads to
real error. In order to have it, we must have typcache entry without
TCFLAGS_HAVE_PG_TYPE_DATA and tupDesc, but with some of
TCFLAGS_OPERATOR_FLAGS. Reseting TCFLAGS_HAVE_PG_TYPE_DATA for a
composite type doesn't seem to be possible without resetting the rest
at the same time.
Nevertheless, I think it would be fragile to leave the current code
"as is". If even there is no case of real error (or it's just me
didn't manage to find it), it could appear after further changes of
type cache code. So, the fix is attached.
------
Regards,
Alexander Korotkov
Supabase
Attachments:
[application/octet-stream] v1-0001-Maintain-RelIdToTypeIdCacheHash-in-TypeCacheOpcCa.patch (2.5K, ../../CAPpHfdv5x5VpiBRuAGYUhyT417chXN2G9Dt+=6m+Odi8A5_R=Q@mail.gmail.com/2-v1-0001-Maintain-RelIdToTypeIdCacheHash-in-TypeCacheOpcCa.patch)
download | inline diff:
From f44dac0d623783aa7bb3ab03eb9c91bb76d1ae87 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <[email protected]>
Date: Mon, 21 Apr 2025 01:40:32 +0300
Subject: [PATCH v1] Maintain RelIdToTypeIdCacheHash in TypeCacheOpcCallback()
b85a9d046efd introduced a new RelIdToTypeIdCacheHash, whose entries should
exist for typecache entries with TCFLAGS_HAVE_PG_TYPE_DATA flag set or any
of TCFLAGS_OPERATOR_FLAGS set or tupDesc set. However, TypeCacheOpcCallback(),
which resets TCFLAGS_OPERATOR_FLAGS, was forgotten to update
RelIdToTypeIdCacheHash.
This commit adds a delete_rel_type_cache_if_needed() call to the
TypeCacheOpcCallback() function to maintain RelIdToTypeIdCacheHash after
resetting TCFLAGS_OPERATOR_FLAGS.
Also, this commit fixes the name of the delete_rel_type_cache_if_needed()
function in its mentions in the comments.
Reported-by: Noah Misch
Discussion: https://postgr.es/m/20250411203241.e9.nmisch%40google.com
---
src/backend/utils/cache/typcache.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c
index ae65a1cce06..560f5595fda 100644
--- a/src/backend/utils/cache/typcache.c
+++ b/src/backend/utils/cache/typcache.c
@@ -2395,7 +2395,7 @@ InvalidateCompositeTypeCacheEntry(TypeCacheEntry *typentry)
/* Reset equality/comparison/hashing validity information */
typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS;
- /* Call delete_rel_type_cache() if we actually cleared something */
+ /* Call delete_rel_type_cache_if_needed() if we actually cleared something */
if (hadTupDescOrOpclass)
delete_rel_type_cache_if_needed(typentry);
}
@@ -2542,7 +2542,7 @@ TypeCacheTypCallback(Datum arg, int cacheid, uint32 hashvalue)
TCFLAGS_CHECKED_DOMAIN_CONSTRAINTS);
/*
- * Call delete_rel_type_cache() if we cleaned
+ * Call delete_rel_type_cache_if_needed() if we cleaned
* TCFLAGS_HAVE_PG_TYPE_DATA flag previously.
*/
if (hadPgTypeData)
@@ -2576,8 +2576,17 @@ TypeCacheOpcCallback(Datum arg, int cacheid, uint32 hashvalue)
hash_seq_init(&status, TypeCacheHash);
while ((typentry = (TypeCacheEntry *) hash_seq_search(&status)) != NULL)
{
+ bool hadOpclass = (typentry->flags & TCFLAGS_OPERATOR_FLAGS);
+
/* Reset equality/comparison/hashing validity information */
typentry->flags &= ~TCFLAGS_OPERATOR_FLAGS;
+
+ /*
+ * Call delete_rel_type_cache_if_needed() if we actually cleared
+ * something
+ */
+ if (hadOpclass)
+ delete_rel_type_cache_if_needed(typentry);
}
}
--
2.39.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2025-04-29 00:56 Noah Misch <[email protected]>
parent: Alexander Korotkov <[email protected]>
0 siblings, 1 reply; 32+ messages in thread
From: Noah Misch @ 2025-04-29 00:56 UTC (permalink / raw)
To: Alexander Korotkov <[email protected]>; +Cc: Pavel Borisov <[email protected]>; Dagfinn Ilmari Mannsåker <[email protected]>; Andrei Lepikhov <[email protected]>; jian he <[email protected]>; Artur Zakirov <[email protected]>; Alexander Lakhin <[email protected]>; Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]>
On Mon, Apr 21, 2025 at 04:54:08AM +0300, Alexander Korotkov wrote:
> On Sat, Apr 12, 2025 at 12:43 AM Alexander Korotkov <[email protected]> wrote:
> > On Fri, Apr 11, 2025 at 11:32 PM Noah Misch <[email protected]> wrote:
> > > On Tue, Oct 22, 2024 at 08:33:24PM +0300, Alexander Korotkov wrote:
> > > > On Tue, Oct 22, 2024 at 6:10 PM Pavel Borisov <[email protected]> wrote:
> > > > > On Tue, 22 Oct 2024 at 11:34, Alexander Korotkov <[email protected]> wrote:
> > > > >> I'm going to push this if no objections.
> > >
> > > (This became commit b85a9d0.)
> > >
> > > > + /* Call delete_rel_type_cache() if we actually cleared something */
> > > > + if (hadTupDescOrOpclass)
> > > > + delete_rel_type_cache_if_needed(typentry);
> > >
> > > I think the intent was to maintain the invariant that a RelIdToTypeIdCacheHash
> > > entry exists if and only if certain kinds of data appear in the TypeCacheHash
> > > entry. However, TypeCacheOpcCallback() clears TCFLAGS_OPERATOR_FLAGS without
> > > maintaining RelIdToTypeIdCacheHash. Is it right to do that?
> Sorry for the delay. Generally, your finding is correct. But, I
> didn't manage to reproduce the situation, where existing code leads to
> real error. In order to have it, we must have typcache entry without
> TCFLAGS_HAVE_PG_TYPE_DATA and tupDesc, but with some of
> TCFLAGS_OPERATOR_FLAGS.
That makes sense.
> Reseting TCFLAGS_HAVE_PG_TYPE_DATA for a
> composite type doesn't seem to be possible without resetting the rest
> at the same time.
>
> Nevertheless, I think it would be fragile to leave the current code
> "as is". If even there is no case of real error (or it's just me
> didn't manage to find it), it could appear after further changes of
> type cache code. So, the fix is attached.
This change looks appropriate. Thanks.
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: type cache cleanup improvements
@ 2025-04-29 10:00 Alexander Korotkov <[email protected]>
parent: Noah Misch <[email protected]>
0 siblings, 0 replies; 32+ messages in thread
From: Alexander Korotkov @ 2025-04-29 10:00 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Pavel Borisov <[email protected]>; Dagfinn Ilmari Mannsåker <[email protected]>; Andrei Lepikhov <[email protected]>; jian he <[email protected]>; Artur Zakirov <[email protected]>; Alexander Lakhin <[email protected]>; Teodor Sigaev <[email protected]>; pgsql-hackers; Aleksander Alekseev <[email protected]>; Tom Lane <[email protected]>; Michael Paquier <[email protected]>
Hi, Noah!
On Tue, Apr 29, 2025 at 3:56 AM Noah Misch <[email protected]> wrote:
> On Mon, Apr 21, 2025 at 04:54:08AM +0300, Alexander Korotkov wrote:
> > On Sat, Apr 12, 2025 at 12:43 AM Alexander Korotkov <[email protected]> wrote:
> > > On Fri, Apr 11, 2025 at 11:32 PM Noah Misch <[email protected]> wrote:
> > > > On Tue, Oct 22, 2024 at 08:33:24PM +0300, Alexander Korotkov wrote:
> > > > > On Tue, Oct 22, 2024 at 6:10 PM Pavel Borisov <[email protected]> wrote:
> > > > > > On Tue, 22 Oct 2024 at 11:34, Alexander Korotkov <[email protected]> wrote:
> > > > > >> I'm going to push this if no objections.
> > > >
> > > > (This became commit b85a9d0.)
> > > >
> > > > > + /* Call delete_rel_type_cache() if we actually cleared something */
> > > > > + if (hadTupDescOrOpclass)
> > > > > + delete_rel_type_cache_if_needed(typentry);
> > > >
> > > > I think the intent was to maintain the invariant that a RelIdToTypeIdCacheHash
> > > > entry exists if and only if certain kinds of data appear in the TypeCacheHash
> > > > entry. However, TypeCacheOpcCallback() clears TCFLAGS_OPERATOR_FLAGS without
> > > > maintaining RelIdToTypeIdCacheHash. Is it right to do that?
>
> > Sorry for the delay. Generally, your finding is correct. But, I
> > didn't manage to reproduce the situation, where existing code leads to
> > real error. In order to have it, we must have typcache entry without
> > TCFLAGS_HAVE_PG_TYPE_DATA and tupDesc, but with some of
> > TCFLAGS_OPERATOR_FLAGS.
>
> That makes sense.
>
> > Reseting TCFLAGS_HAVE_PG_TYPE_DATA for a
> > composite type doesn't seem to be possible without resetting the rest
> > at the same time.
> >
> > Nevertheless, I think it would be fragile to leave the current code
> > "as is". If even there is no case of real error (or it's just me
> > didn't manage to find it), it could appear after further changes of
> > type cache code. So, the fix is attached.
>
> This change looks appropriate. Thanks.
Thank you for your feedback!
------
Regards,
Alexander Korotkov
Supabase
^ permalink raw reply [nested|flat] 32+ messages in thread
end of thread, other threads:[~2025-04-29 10:00 UTC | newest]
Thread overview: 32+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-09-26 21:05 [PATCH v2 5/8] jit: explain: remove backend lifetime module count from function name. Andres Freund <[email protected]>
2024-10-10 15:54 Re: type cache cleanup improvements Artur Zakirov <[email protected]>
2024-10-13 12:08 ` Re: type cache cleanup improvements Alexander Korotkov <[email protected]>
2024-10-15 07:34 ` Re: type cache cleanup improvements jian he <[email protected]>
2024-10-15 08:08 ` Re: type cache cleanup improvements Alexander Korotkov <[email protected]>
2024-10-15 08:45 ` Re: type cache cleanup improvements Artur Zakirov <[email protected]>
2024-10-15 09:50 ` Re: type cache cleanup improvements jian he <[email protected]>
2024-10-15 13:16 ` Re: type cache cleanup improvements Artur Zakirov <[email protected]>
2024-10-20 17:47 ` Re: type cache cleanup improvements Alexander Korotkov <[email protected]>
2024-10-20 18:00 ` Re: type cache cleanup improvements Alexander Korotkov <[email protected]>
2024-10-20 22:09 ` Re: type cache cleanup improvements Alexander Korotkov <[email protected]>
2024-10-20 23:32 ` Re: type cache cleanup improvements Dagfinn Ilmari Mannsåker <[email protected]>
2024-10-21 05:40 ` Re: type cache cleanup improvements Andrei Lepikhov <[email protected]>
2024-10-21 08:10 ` Re: type cache cleanup improvements Alexander Korotkov <[email protected]>
2024-10-21 10:16 ` Re: type cache cleanup improvements Dagfinn Ilmari Mannsåker <[email protected]>
2024-10-21 11:30 ` Re: type cache cleanup improvements Alexander Korotkov <[email protected]>
2024-10-22 07:34 ` Re: type cache cleanup improvements Alexander Korotkov <[email protected]>
2024-10-22 15:09 ` Re: type cache cleanup improvements Pavel Borisov <[email protected]>
2024-10-22 17:33 ` Re: type cache cleanup improvements Alexander Korotkov <[email protected]>
2024-10-25 08:35 ` Re: type cache cleanup improvements Andres Freund <[email protected]>
2024-10-25 09:48 ` Re: type cache cleanup improvements Alexander Korotkov <[email protected]>
2024-10-25 10:13 ` Re: type cache cleanup improvements Alexander Korotkov <[email protected]>
2025-04-11 20:32 ` Re: type cache cleanup improvements Noah Misch <[email protected]>
2025-04-11 21:43 ` Re: type cache cleanup improvements Alexander Korotkov <[email protected]>
2025-04-21 01:54 ` Re: type cache cleanup improvements Alexander Korotkov <[email protected]>
2025-04-29 00:56 ` Re: type cache cleanup improvements Noah Misch <[email protected]>
2025-04-29 10:00 ` Re: type cache cleanup improvements Alexander Korotkov <[email protected]>
2024-10-21 07:51 ` Re: type cache cleanup improvements jian he <[email protected]>
2024-10-21 08:11 ` Re: type cache cleanup improvements Alexander Korotkov <[email protected]>
2024-10-17 09:41 ` Re: type cache cleanup improvements Andrei Lepikhov <[email protected]>
2024-10-20 17:36 ` Re: type cache cleanup improvements Alexander Korotkov <[email protected]>
2024-10-21 05:36 ` Re: type cache cleanup improvements Andrei Lepikhov <[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