Is a clearer memory lifespan for outerTuple and innerTuple useful?

Started by Andy Fanabout 2 years ago5 messages
#1Andy Fan
zhihuifan1213@163.com
1 attachment(s)

Hi,

When I am working on "shared detoast value"[0]/messages/by-id/87ttoyihgm.fsf@163.com, where I want to avoid
detoast the same datum over and over again, I have to decide which
memory context should be used to hold the detoast value. later I
found I have to use different MemoryContexts for the OuterTuple and
innerTuple since OuterTuple usually have a longer lifespan.

I found the following code in nodeMergeJoin.c which has pretty similar
situation, just that it uses ExprContext rather than MemoryContext.

MergeJoinState *
ExecInitMergeJoin(MergeJoin *node, EState *estate, int eflags)

/*
* we need two additional econtexts in which we can compute the join
* expressions from the left and right input tuples. The node's regular
* econtext won't do because it gets reset too often.
*/
mergestate->mj_OuterEContext = CreateExprContext(estate);
mergestate->mj_InnerEContext = CreateExprContext(estate);

IIUC, we needs a MemoryContext rather than ExprContext in fact. In the
attachment, I just use two MemoryContext instead of the two ExprContexts
which should be less memory and more precise semantics, and works
fine. shall we go in this direction? I attached the 2 MemoryContext in
JoinState rather than MergeJoinState, which is for the "shared detoast
value"[0]/messages/by-id/87ttoyihgm.fsf@163.com more or less.

[0]: /messages/by-id/87ttoyihgm.fsf@163.com

Attachments:

v1-0001-Use-MemoryContext-instead-of-ExprContext-for-node.patchtext/x-diffDownload
From cadd94f8ff2398ca430b43494b3eb71225b017c3 Mon Sep 17 00:00:00 2001
From: "yizhi.fzh" <yizhi.fzh@alibaba-inc.com>
Date: Fri, 15 Dec 2023 15:28:36 +0800
Subject: [PATCH v1] Use MemoryContext instead of ExprContext for
 nodeMergejoin.c

---
 src/backend/executor/nodeMergejoin.c | 27 ++++++++++++++-------------
 src/backend/executor/nodeNestloop.c  |  1 +
 src/include/nodes/execnodes.h        |  4 ++--
 3 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/src/backend/executor/nodeMergejoin.c b/src/backend/executor/nodeMergejoin.c
index 3cdab77dfc..d330e104f6 100644
--- a/src/backend/executor/nodeMergejoin.c
+++ b/src/backend/executor/nodeMergejoin.c
@@ -294,7 +294,7 @@ MJExamineQuals(List *mergeclauses,
 static MJEvalResult
 MJEvalOuterValues(MergeJoinState *mergestate)
 {
-	ExprContext *econtext = mergestate->mj_OuterEContext;
+	ExprContext *econtext = mergestate->js.ps.ps_ExprContext;
 	MJEvalResult result = MJEVAL_MATCHABLE;
 	int			i;
 	MemoryContext oldContext;
@@ -303,9 +303,9 @@ MJEvalOuterValues(MergeJoinState *mergestate)
 	if (TupIsNull(mergestate->mj_OuterTupleSlot))
 		return MJEVAL_ENDOFJOIN;
 
-	ResetExprContext(econtext);
+	MemoryContextReset(mergestate->js.outer_tuple_memory);
 
-	oldContext = MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
+	oldContext = MemoryContextSwitchTo(mergestate->js.outer_tuple_memory);
 
 	econtext->ecxt_outertuple = mergestate->mj_OuterTupleSlot;
 
@@ -341,7 +341,7 @@ MJEvalOuterValues(MergeJoinState *mergestate)
 static MJEvalResult
 MJEvalInnerValues(MergeJoinState *mergestate, TupleTableSlot *innerslot)
 {
-	ExprContext *econtext = mergestate->mj_InnerEContext;
+	ExprContext *econtext = mergestate->js.ps.ps_ExprContext;
 	MJEvalResult result = MJEVAL_MATCHABLE;
 	int			i;
 	MemoryContext oldContext;
@@ -352,7 +352,7 @@ MJEvalInnerValues(MergeJoinState *mergestate, TupleTableSlot *innerslot)
 
 	ResetExprContext(econtext);
 
-	oldContext = MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
+	oldContext = MemoryContextSwitchTo(mergestate->js.inner_tuple_memory);
 
 	econtext->ecxt_innertuple = innerslot;
 
@@ -1471,14 +1471,6 @@ ExecInitMergeJoin(MergeJoin *node, EState *estate, int eflags)
 	 */
 	ExecAssignExprContext(estate, &mergestate->js.ps);
 
-	/*
-	 * we need two additional econtexts in which we can compute the join
-	 * expressions from the left and right input tuples.  The node's regular
-	 * econtext won't do because it gets reset too often.
-	 */
-	mergestate->mj_OuterEContext = CreateExprContext(estate);
-	mergestate->mj_InnerEContext = CreateExprContext(estate);
-
 	/*
 	 * initialize child nodes
 	 *
@@ -1497,6 +1489,15 @@ ExecInitMergeJoin(MergeJoin *node, EState *estate, int eflags)
 											  (eflags | EXEC_FLAG_MARK));
 	innerDesc = ExecGetResultType(innerPlanState(mergestate));
 
+	mergestate->js.outer_tuple_memory = AllocSetContextCreate(
+		mergestate->js.ps.ps_ExprContext->ecxt_per_query_memory,
+		"OuterTupleCtx",
+		ALLOCSET_SMALL_SIZES);
+	mergestate->js.inner_tuple_memory = AllocSetContextCreate(
+		mergestate->js.ps.ps_ExprContext->ecxt_per_query_memory,
+		"InnerTupleCtx",
+		ALLOCSET_SMALL_SIZES);
+
 	/*
 	 * For certain types of inner child nodes, it is advantageous to issue
 	 * MARK every time we advance past an inner tuple we will never return to.
diff --git a/src/backend/executor/nodeNestloop.c b/src/backend/executor/nodeNestloop.c
index ebd1406843..5e10d01c4e 100644
--- a/src/backend/executor/nodeNestloop.c
+++ b/src/backend/executor/nodeNestloop.c
@@ -349,6 +349,7 @@ ExecInitNestLoop(NestLoop *node, EState *estate, int eflags)
 	NL1_printf("ExecInitNestLoop: %s\n",
 			   "node initialized");
 
+
 	return nlstate;
 }
 
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 5d7f17dee0..c0d4958ba2 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -2003,6 +2003,8 @@ typedef struct JoinState
 	bool		single_match;	/* True if we should skip to next outer tuple
 								 * after finding one inner match */
 	ExprState  *joinqual;		/* JOIN quals (in addition to ps.qual) */
+	MemoryContext	outer_tuple_memory;
+	MemoryContext	inner_tuple_memory;
 } JoinState;
 
 /* ----------------
@@ -2064,8 +2066,6 @@ typedef struct MergeJoinState
 	TupleTableSlot *mj_MarkedTupleSlot;
 	TupleTableSlot *mj_NullOuterTupleSlot;
 	TupleTableSlot *mj_NullInnerTupleSlot;
-	ExprContext *mj_OuterEContext;
-	ExprContext *mj_InnerEContext;
 } MergeJoinState;
 
 /* ----------------
-- 
2.34.1

#2Andy Fan
zhihuifan1213@163.com
In reply to: Andy Fan (#1)
Re: Is a clearer memory lifespan for outerTuple and innerTuple useful?

Andy Fan <zhihuifan1213@163.com> writes:

..., I attached the 2 MemoryContext in
JoinState rather than MergeJoinState, which is for the "shared detoast
value"[0] more or less.

After thinking more, if it is designed for "shared detoast value" patch
(happens on ExecInterpExpr stage), the inner_tuple_memory and
outer_tuple_memory should be attached to ExprContext rather than
JoinState since it is more natual to access ExprConext (compared with
JoinState) in ExecInterpExpr. I didn't attach a new version for this,
any feedback will be appreciated.

--
Best Regards
Andy Fan

#3Andy Fan
zhihuifan1213@163.com
In reply to: Andy Fan (#2)
1 attachment(s)
Re: Is a clearer memory lifespan for outerTuple and innerTuple useful?

Andy Fan <zhihuifan1213@163.com> writes:

Andy Fan <zhihuifan1213@163.com> writes:

..., I attached the 2 MemoryContext in
JoinState rather than MergeJoinState, which is for the "shared detoast
value"[0] more or less.

In order to delimit the scope of this discussion, I attached the 2
MemoryContext to MergeJoinState. Since the code was writen by Tom at
2005, so add Tom to the cc-list.

Attachments:

v2-0001-Use-MemoryContext-instead-of-ExprContext-for-node.patchtext/x-diffDownload
From 20068bdda00716da712fb3f1e554401e81924e19 Mon Sep 17 00:00:00 2001
From: "yizhi.fzh" <yizhi.fzh@alibaba-inc.com>
Date: Sun, 17 Dec 2023 21:41:34 +0800
Subject: [PATCH v2 1/1] Use MemoryContext instead of ExprContext for
 nodeMergejoin.c

MergeJoin needs to store the values in MergeJoinClause for a longer
lifespan, in the past it uses a dedicate ExprContext, however a
MemoryContext should be OK. This patch does this.
---
 src/backend/executor/nodeMergejoin.c | 29 +++++++++++++++++-----------
 src/include/nodes/execnodes.h        |  5 +++--
 2 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/src/backend/executor/nodeMergejoin.c b/src/backend/executor/nodeMergejoin.c
index 3cdab77dfc..4d45305482 100644
--- a/src/backend/executor/nodeMergejoin.c
+++ b/src/backend/executor/nodeMergejoin.c
@@ -294,7 +294,7 @@ MJExamineQuals(List *mergeclauses,
 static MJEvalResult
 MJEvalOuterValues(MergeJoinState *mergestate)
 {
-	ExprContext *econtext = mergestate->mj_OuterEContext;
+	ExprContext *econtext = mergestate->js.ps.ps_ExprContext;
 	MJEvalResult result = MJEVAL_MATCHABLE;
 	int			i;
 	MemoryContext oldContext;
@@ -303,9 +303,9 @@ MJEvalOuterValues(MergeJoinState *mergestate)
 	if (TupIsNull(mergestate->mj_OuterTupleSlot))
 		return MJEVAL_ENDOFJOIN;
 
-	ResetExprContext(econtext);
+	MemoryContextReset(mergestate->mj_outerTuple_memory);
 
-	oldContext = MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
+	oldContext = MemoryContextSwitchTo(mergestate->mj_outerTuple_memory);
 
 	econtext->ecxt_outertuple = mergestate->mj_OuterTupleSlot;
 
@@ -341,7 +341,7 @@ MJEvalOuterValues(MergeJoinState *mergestate)
 static MJEvalResult
 MJEvalInnerValues(MergeJoinState *mergestate, TupleTableSlot *innerslot)
 {
-	ExprContext *econtext = mergestate->mj_InnerEContext;
+	ExprContext *econtext = mergestate->js.ps.ps_ExprContext;
 	MJEvalResult result = MJEVAL_MATCHABLE;
 	int			i;
 	MemoryContext oldContext;
@@ -350,9 +350,9 @@ MJEvalInnerValues(MergeJoinState *mergestate, TupleTableSlot *innerslot)
 	if (TupIsNull(innerslot))
 		return MJEVAL_ENDOFJOIN;
 
-	ResetExprContext(econtext);
+	MemoryContextReset(mergestate->mj_innerTuple_memory);
 
-	oldContext = MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
+	oldContext = MemoryContextSwitchTo(mergestate->mj_innerTuple_memory);
 
 	econtext->ecxt_innertuple = innerslot;
 
@@ -1447,6 +1447,7 @@ ExecInitMergeJoin(MergeJoin *node, EState *estate, int eflags)
 	TupleDesc	outerDesc,
 				innerDesc;
 	const TupleTableSlotOps *innerOps;
+	ExprContext *econtext;
 
 	/* check for unsupported flags */
 	Assert(!(eflags & (EXEC_FLAG_BACKWARD | EXEC_FLAG_MARK)));
@@ -1471,13 +1472,19 @@ ExecInitMergeJoin(MergeJoin *node, EState *estate, int eflags)
 	 */
 	ExecAssignExprContext(estate, &mergestate->js.ps);
 
+	econtext = mergestate->js.ps.ps_ExprContext;
+
 	/*
-	 * we need two additional econtexts in which we can compute the join
-	 * expressions from the left and right input tuples.  The node's regular
-	 * econtext won't do because it gets reset too often.
+	 * we need two additional contexts in which we can compute the join
+	 * expressions from the left and right input tuples.  The econtext's
+	 * regular ecxt_per_tuple_memory won't do because it gets reset too often.
 	 */
-	mergestate->mj_OuterEContext = CreateExprContext(estate);
-	mergestate->mj_InnerEContext = CreateExprContext(estate);
+	mergestate->mj_outerTuple_memory = AllocSetContextCreate(econtext->ecxt_per_query_memory,
+															 "OuterTupleCtx",
+															 ALLOCSET_SMALL_SIZES);
+	mergestate->mj_innerTuple_memory = AllocSetContextCreate(econtext->ecxt_per_query_memory,
+															 "InnerTupleCtx",
+															 ALLOCSET_SMALL_SIZES);
 
 	/*
 	 * initialize child nodes
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 5d7f17dee0..e8af959c01 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -2064,8 +2064,9 @@ typedef struct MergeJoinState
 	TupleTableSlot *mj_MarkedTupleSlot;
 	TupleTableSlot *mj_NullOuterTupleSlot;
 	TupleTableSlot *mj_NullInnerTupleSlot;
-	ExprContext *mj_OuterEContext;
-	ExprContext *mj_InnerEContext;
+
+	MemoryContext mj_outerTuple_memory;
+	MemoryContext mj_innerTuple_memory;
 } MergeJoinState;
 
 /* ----------------
-- 
2.34.1

#4Nikita Malakhov
hukutoc@gmail.com
In reply to: Andy Fan (#3)
Re: Is a clearer memory lifespan for outerTuple and innerTuple useful?

Hi!

Maybe, the alternative way is using a separate kind of context, say name it
'ToastContext' for all custom data related to Toasted values? What do you
think?

On Sun, Dec 17, 2023 at 4:52 PM Andy Fan <zhihuifan1213@163.com> wrote:

Andy Fan <zhihuifan1213@163.com> writes:

Andy Fan <zhihuifan1213@163.com> writes:

..., I attached the 2 MemoryContext in
JoinState rather than MergeJoinState, which is for the "shared detoast
value"[0] more or less.

In order to delimit the scope of this discussion, I attached the 2
MemoryContext to MergeJoinState. Since the code was writen by Tom at
2005, so add Tom to the cc-list.

--
Best Regards
Andy Fan

--
Regards,
Nikita Malakhov
Postgres Professional
The Russian Postgres Company
https://postgrespro.ru/

#5Andy Fan
zhihuifan1213@163.com
In reply to: Nikita Malakhov (#4)
Re: Is a clearer memory lifespan for outerTuple and innerTuple useful?

Nikita Malakhov <hukutoc@gmail.com> writes:

Hi!

Maybe, the alternative way is using a separate kind of context, say name it
'ToastContext' for all custom data related to Toasted values? What do
you think?

That should be a candidate. The latest research makes me think the
'detoast_values' should have the same life cycles as tts_values, so the
memory should be managed by TupleTuleSlot (rather than ExprContext) and
be handled in ExecCopySlot / ExecClearSlot stuff.

In TupleTableSlot we already have a tts_mctx MemoryContext, reusing it
needs using 'pfree' to free the detoast values and but a dedicated
memory context pays more costs on the setup, but a more efficient
MemoryContextReset.

On Sun, Dec 17, 2023 at 4:52 PM Andy Fan <zhihuifan1213@163.com> wrote:

Andy Fan <zhihuifan1213@163.com> writes:

Andy Fan <zhihuifan1213@163.com> writes:

..., I attached the 2 MemoryContext in
JoinState rather than MergeJoinState, which is for the "shared detoast
value"[0] more or less.

In order to delimit the scope of this discussion, I attached the 2
MemoryContext to MergeJoinState. Since the code was writen by Tom at
2005, so add Tom to the cc-list.

However this patch can be discussed seperately.

--
Best Regards
Andy Fan