On 26/2/2024 12:44, Tender Wang wrote:
Andrei Lepikhov <a.lepik...@postgrespro.ru
<mailto:a.lepik...@postgrespro.ru>> 于2024年2月26日周一 10:57写道：
On 25/2/2024 20:32, Tender Wang wrote:
> I think in prepare_probe_slot(), should called datumCopy as the
attached
> patch does.
>
> Any thoughts? Thanks.
Thanks for the report.
I think it is better to invent a Runtime Memory Context; likewise,
it is
already designed in IndexScan and derivatives. Here, you just allocate
the value in some upper memory context.
Also, I'm curious why such a trivial error hasn't been found for a
long time
Make sense. I found MemoizeState already has a MemoryContext, so I used it.
I update the patch.
This approach is better for me. In the next version of this patch, I
included a test case. I am still unsure about the context chosen and the
stability of the test case. Richard, you recently fixed some Memoize
issues, could you look at this problem and patch?
--
regards,
Andrei Lepikhov
Postgres Professional
From e32900e50730bccfde26355609d6b0b3e970f0a8 Mon Sep 17 00:00:00 2001
From: "tender.wang" <tender.w...@openpie.com>
Date: Mon, 26 Feb 2024 13:38:30 +0800
Subject: [PATCH] Store Memoize probeslot values in the hash table memory
context.
Values of probeslot evaluates in expression memory context which can be reset
on hash comparison when we still need it for the equality comparison.
So we copy the result to probeslot from ExecEvalExpr.
---
src/backend/executor/nodeMemoize.c | 23 ++++++++++++++++-------
src/test/regress/expected/memoize.out | 26 ++++++++++++++++++++++++++
src/test/regress/sql/memoize.sql | 14 ++++++++++++++
3 files changed, 56 insertions(+), 7 deletions(-)
diff --git a/src/backend/executor/nodeMemoize.c
b/src/backend/executor/nodeMemoize.c
index 18870f10e1..6402943772 100644
--- a/src/backend/executor/nodeMemoize.c
+++ b/src/backend/executor/nodeMemoize.c
@@ -312,17 +312,26 @@ prepare_probe_slot(MemoizeState *mstate, MemoizeKey *key)
if (key == NULL)
{
ExprContext *econtext = mstate->ss.ps.ps_ExprContext;
+ Datum value;
+ bool isnull;
+ TupleDesc tup = pslot->tts_tupleDescriptor;
+ Form_pg_attribute att;
MemoryContext oldcontext;
- oldcontext =
MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
-
/* Set the probeslot's values based on the current parameter
values */
for (int i = 0; i < numKeys; i++)
- pslot->tts_values[i] =
ExecEvalExpr(mstate->param_exprs[i],
-
econtext,
-
&pslot->tts_isnull[i]);
-
- MemoryContextSwitchTo(oldcontext);
+ {
+ att = TupleDescAttr(tup, i);
+ value =
ExecEvalExprSwitchContext(mstate->param_exprs[i],
+
econtext,
+
&isnull);
+ /* Copy the value to avoid freed after resetting
ExprContext */
+ oldcontext =
MemoryContextSwitchTo(mstate->tableContext);
+ pslot->tts_values[i] = datumCopy(value, att->attbyval,
att->attlen);
+ MemoryContextSwitchTo(oldcontext);
+
+ pslot->tts_isnull[i] = isnull;
+ }
}
else
{
diff --git a/src/test/regress/expected/memoize.out
b/src/test/regress/expected/memoize.out
index cf6886a288..9842b238c6 100644
--- a/src/test/regress/expected/memoize.out
+++ b/src/test/regress/expected/memoize.out
@@ -196,6 +196,32 @@ SELECT * FROM flt f1 INNER JOIN flt f2 ON f1.f >= f2.f;',
false);
(10 rows)
DROP TABLE flt;
+CREATE TABLE expr_key (x numeric, t text);
+INSERT INTO expr_key (x,t) SELECT d1::numeric, d1::text FROM (
+ SELECT round((d/pi())::numeric,7) AS d1 FROM generate_series(1,20) AS d);
+-- Having duplicates we must see hits in the Memoize node
+INSERT INTO expr_key SELECT * FROM expr_key;
+CREATE INDEX expr_key_idx_x_t ON expr_key (x,t);
+VACUUM ANALYZE expr_key;
+SELECT explain_memoize('
+SELECT * FROM expr_key t1 INNER JOIN expr_key t2
+ON t1.x=t2.t::numeric AND t1.t::numeric=t2.x;', true);
+ explain_memoize
+-------------------------------------------------------------------------------------------
+ Nested Loop (actual rows=80 loops=N)
+ -> Index Only Scan using expr_key_idx_x_t on expr_key t1 (actual rows=40
loops=N)
+ Heap Fetches: N
+ -> Memoize (actual rows=2 loops=N)
+ Cache Key: t1.x, (t1.t)::numeric
+ Cache Mode: logical
+ Hits: N Misses: N Evictions: Zero Overflows: 0 Memory Usage: NkB
+ -> Index Only Scan using expr_key_idx_x_t on expr_key t2 (actual
rows=2 loops=N)
+ Index Cond: (x = (t1.t)::numeric)
+ Filter: (t1.x = (t)::numeric)
+ Heap Fetches: N
+(11 rows)
+
+DROP TABLE expr_key;
-- Exercise Memoize in binary mode with a large fixed width type and a
-- varlena type.
CREATE TABLE strtest (n name, t text);
diff --git a/src/test/regress/sql/memoize.sql b/src/test/regress/sql/memoize.sql
index 1f4ab0ba3b..f2f7643571 100644
--- a/src/test/regress/sql/memoize.sql
+++ b/src/test/regress/sql/memoize.sql
@@ -103,6 +103,20 @@ SELECT * FROM flt f1 INNER JOIN flt f2 ON f1.f >= f2.f;',
false);
DROP TABLE flt;
+CREATE TABLE expr_key (x numeric, t text);
+INSERT INTO expr_key (x,t) SELECT d1::numeric, d1::text FROM (
+ SELECT round((d/pi())::numeric,7) AS d1 FROM generate_series(1,20) AS d);
+-- Having duplicates we must see hits in the Memoize node
+INSERT INTO expr_key SELECT * FROM expr_key;
+CREATE INDEX expr_key_idx_x_t ON expr_key (x,t);
+VACUUM ANALYZE expr_key;
+
+SELECT explain_memoize('
+SELECT * FROM expr_key t1 INNER JOIN expr_key t2
+ON t1.x=t2.t::numeric AND t1.t::numeric=t2.x;', true);
+
+DROP TABLE expr_key;
+
-- Exercise Memoize in binary mode with a large fixed width type and a
-- varlena type.
CREATE TABLE strtest (n name, t text);
--
2.43.2
