pg.ddx.io pgsql-bugs@postgresql.org mailing list archive help / color / mirror / Atom feed From: Andrew Dunstan <andrew@dunslane.net>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andres Freund <andres@anarazel.de>
Cc: exclusion@gmail.com
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17994: Invalidating relcache corrupts tupDesc inside ExecEvalFieldStoreDeForm()
Date: Sat, 1 Jul 2023 09:14:39 -0400
Message-ID: <3878b204-e9b4-6e09-9adb-7a37b8db34a6@dunslane.net> (raw )
In-Reply-To: <1800418.1688152085@sss.pgh.pa.us >
References: <17994-5c7100b51b4790e9@postgresql.org >
<1306569.1687978174@sss.pgh.pa.us >
<20230628194446.uu47f4vyfyjfl7dd@awork3.anarazel.de >
<1314841.1687981948@sss.pgh.pa.us >
<20230628205443.6gu5bmcck6t4pgyv@awork3.anarazel.de >
<e52f6028-7196-8545-9726-a17a50ea14a3@dunslane.net >
<e6cbff03-32c3-4048-78b6-54d2907ba852@dunslane.net >
<1509402.1688048784@sss.pgh.pa.us >
<7ae37634-7626-8977-4d89-20603f0d1dcf@dunslane.net >
<1583514.1688066735@sss.pgh.pa.us >
<326723ee-ee05-35bf-06fd-e4f3afc61654@dunslane.net >
<1610276.1688078516@sss.pgh.pa.us >
<5316ba5d-ab3b-4ae2-6f6b-f22760932d99@dunslane.net >
<1742513.1688129204@sss.pgh.pa.us >
<cf24e075-0809-b9b8-4c73-48eed105b9a9@dunslane.net >
<1749275.1688132604@sss.pgh.pa.us >
<e56b9c00-291b-dac0-6260-74137fdaffa5@dunslane.net >
<1800418.1688152085@sss.pgh.pa.us >
On 2023-06-30 Fr 15:08, Tom Lane wrote:
> Andrew Dunstan<andrew@dunslane.net> writes:
>> Me either. I think this might call for too much invention so I'm going
>> to revert to plan A. The invalidation code won't be very much, and it
>> should be a fairly rare event, so it doesn't need to be very clever.
> The problem with rarely-executed code is that it's also hard to test.
>
>
Your Plan B in the end proved less difficult than I thought, and
certainly seems more robust than having to tangle with invalidations. I
didn't try to do anything to wrap the values in a bytea, it didn't seem
necessary. Here's a patch - it's not terribly long or invasive. I
haven't tried backpatching it yet.
cheers
andrew
--
Andrew Dunstan
EDB:https://www.enterprisedb.com
Attachments:
[text/x-patch] fix-missing-value-corruption.patch (4.8K, ../3878b204-e9b4-6e09-9adb-7a37b8db34a6@dunslane.net/3-fix-missing-value-corruption.patch)
download | inline diff:
diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c
index cb2291ee36..acd801a07c 100644
--- a/src/backend/access/common/heaptuple.c
+++ b/src/backend/access/common/heaptuple.c
@@ -60,9 +60,12 @@
#include "access/heaptoast.h"
#include "access/sysattr.h"
#include "access/tupdesc_details.h"
+#include "common/hashfn.h"
#include "executor/tuptable.h"
+#include "utils/datum.h"
#include "utils/expandeddatum.h"
-
+#include "utils/hsearch.h"
+#include "utils/memutils.h"
/* Does att's datatype allow packing into the 1-byte-header varlena format? */
#define ATT_IS_PACKABLE(att) \
@@ -71,6 +74,68 @@
#define VARLENA_ATT_IS_PACKABLE(att) \
((att)->attstorage != TYPSTORAGE_PLAIN)
+/*
+ * Setup for cacheing pass-by-ref missing attributes in a way that survives
+ * tupleDesc destruction.
+ */
+
+typedef struct
+{
+ int len;
+ Datum value;
+} missing_cache_key;
+
+static HTAB *missing_cache = NULL;
+
+static uint32
+missing_hash(const void *key, Size keysize)
+{
+ const missing_cache_key *entry = (missing_cache_key *) key;
+
+ return hash_bytes((const unsigned char *) entry->value, entry->len);
+}
+
+static int
+missing_match(const void *key1, const void *key2, Size keysize)
+{
+ const missing_cache_key *entry1 = (missing_cache_key *) key1;
+ const missing_cache_key *entry2 = (missing_cache_key *) key2;
+
+ if (entry1->len != entry2->len)
+ return entry1->len > entry2->len ? 1 : -1;
+
+ return memcmp(DatumGetPointer(entry1->value),
+ DatumGetPointer(entry2->value),
+ entry1->len);
+}
+
+static void
+init_missing_cache()
+{
+ HASHCTL hash_ctl;
+
+ hash_ctl.keysize = sizeof(missing_cache_key);
+ hash_ctl.entrysize = sizeof(missing_cache_key);
+ hash_ctl.hcxt = TopTransactionContext;
+ hash_ctl.hash = missing_hash;
+ hash_ctl.match = missing_match;
+ missing_cache =
+ hash_create("Missing Values Cache",
+ 32,
+ &hash_ctl,
+ HASH_ELEM | HASH_CONTEXT | HASH_FUNCTION | HASH_COMPARE);
+}
+
+/* end of transaction routine to clear the missing values cache */
+void
+AtEOXact_MissCache(void)
+{
+ /*
+ * just set the cache pointer to NULL, the context cleanup will do the
+ * rest
+ */
+ missing_cache = NULL;
+}
/* ----------------------------------------------------------------
* misc support routines
@@ -102,8 +167,44 @@ getmissingattr(TupleDesc tupleDesc,
if (attrmiss->am_present)
{
+ missing_cache_key key;
+ missing_cache_key *entry;
+ bool found;
+ MemoryContext oldctx;
+ Datum copy;
+
*isnull = false;
- return attrmiss->am_value;
+
+ /* no need to cache by-value attributes */
+ if (att->attbyval)
+ return attrmiss->am_value;
+
+ /* set up cache if required */
+ if (missing_cache == NULL)
+ init_missing_cache();
+
+ /* check if there's a cache entry */
+ Assert(att->attlen > 0 || att->attlen == -1);
+ if (att->attlen > 0)
+ key.len = att->attlen;
+ else
+ key.len = VARSIZE_ANY(attrmiss->am_value);
+ key.value = attrmiss->am_value;
+
+ entry = hash_search(missing_cache, &key, HASH_ENTER, &found);
+
+ /* cache hit, so return it */
+ if (found)
+ return entry->value;
+
+ /* cache miss, so we need a transaction-stable copy of the datum */
+ oldctx = MemoryContextSwitchTo(TopTransactionContext);
+ copy = datumCopy(attrmiss->am_value, false, att->attlen);
+ MemoryContextSwitchTo(oldctx);
+
+ entry->value = copy;
+
+ return copy;
}
}
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 8daaa535ed..b2946b955e 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -21,6 +21,7 @@
#include <unistd.h>
#include "access/commit_ts.h"
+#include "access/htup_details.h"
#include "access/multixact.h"
#include "access/parallel.h"
#include "access/subtrans.h"
@@ -2362,6 +2363,7 @@ CommitTransaction(void)
AtEOXact_GUC(true, 1);
AtEOXact_SPI(true);
AtEOXact_Enum();
+ AtEOXact_MissCache();
AtEOXact_on_commit_actions(true);
AtEOXact_Namespace(true, is_parallel_worker);
AtEOXact_SMgr();
diff --git a/src/include/access/htup_details.h b/src/include/access/htup_details.h
index e01f4f35c8..027c4f5436 100644
--- a/src/include/access/htup_details.h
+++ b/src/include/access/htup_details.h
@@ -736,6 +736,7 @@ extern MinimalTuple minimal_tuple_from_heap_tuple(HeapTuple htup);
extern size_t varsize_any(void *p);
extern HeapTuple heap_expand_tuple(HeapTuple sourceTuple, TupleDesc tupleDesc);
extern MinimalTuple minimal_expand_tuple(HeapTuple sourceTuple, TupleDesc tupleDesc);
+extern void AtEOXact_MissCache(void);
#ifndef FRONTEND
/*
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 260854747b..91d57159db 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -3471,6 +3471,7 @@ mbstr_verifier
memoize_hash
memoize_iterator
metastring
+missing_cache_key
mix_data_t
mixedStruct
mode_t
view thread (33+ messages) latest in thread
Message-ID: <3878b204-e9b4-6e09-9adb-7a37b8db34a6@dunslane.net>
Permalink: ../3878b204-e9b4-6e09-9adb-7a37b8db34a6@dunslane.net/
Also on: postgresql.org/message-id/3878b204-e9b4-6e09-9adb-7a37b8db34a6@dunslane.net
copy link · copy postgr.es · copy git am
reply Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: andrew@dunslane.net, tgl@sss.pgh.pa.us, andres@anarazel.de, exclusion@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17994: Invalidating relcache corrupts tupDesc inside ExecEvalFieldStoreDeForm()
In-Reply-To: <3878b204-e9b4-6e09-9adb-7a37b8db34a6@dunslane.net>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox