rename es_epq_active to es_epqstate
Hi hackers,
While reviewing the ExecSeqScan optimizations patch[1]/messages/by-id/CA+HiwqF+oydVreYN3Xp7U6x_LKi9ZL+No2X6WUv8X_kN+yHSLA@mail.gmail.com, I found that
es_epq_active might not be well named, my intuition told me that this
is a boolean field because of the "active" suffix.
es_epq_active was introduced in 27cc7cd, in the original discussion[2]/messages/by-id/20190828030201.v5u76ty47mtw2efp@alap3.anarazel.de,
Tom and Andres discussed the field name "es_active_epq" a little bit,
let me quote some:
------- quoted content begin
Also I dislike the field name "es_active_epq", as what that suggests
to me is exactly backwards from the way you apparently are using it.
Maybe "es_parent_epq" instead? The comment for it is pretty opaque
as well, not to mention that it misspells EState.
I think what I was trying to signal was that EPQ is currently active
from the POV of executor nodes. Ought to have been es_epq_active, for
that, I guess. For me "if (estate->es_epq_active)" explains the meaning
of the check more than "if (estate->es_parent_epq)".
I went with es_epq_active, as I suggested in my earlier email, which
still seems accurate to me.
I really want to move consideration of es_ep_active to ExecInitNode()
time, rather than execution time. If we add an execScan helper, we can
have it set the corresponding executor node's ExecProcNode to
a) a function that performs qual checks and projection
b) a function that performs projection
c) the fetch method from the scan node
d) basically the current ExecScan, when es_epq_active
-------- quoted content end
ISTM Andres tend to use *es_epq_active* in a boolean way,
like `if (es_epq_active) then`, but in the code base, all its usages
follow pattern `if (es_epq_active == NULL) then`, so I propose to
change es_epq_active to es_epqstate.
[1]: /messages/by-id/CA+HiwqF+oydVreYN3Xp7U6x_LKi9ZL+No2X6WUv8X_kN+yHSLA@mail.gmail.com
[2]: /messages/by-id/20190828030201.v5u76ty47mtw2efp@alap3.anarazel.de
--
Regards
Junwang Zhao
Attachments:
v1-0001-rename-es_epq_active-to-es_epqstate.patchapplication/octet-stream; name=v1-0001-rename-es_epq_active-to-es_epqstate.patchDownload
From 30ad05f7c4d01e807de5979d19b192185eedbfcb Mon Sep 17 00:00:00 2001
From: Junwang Zhao <zhjwpku@gmail.com>
Date: Sat, 18 Jan 2025 00:52:59 +0000
Subject: [PATCH v1] rename es_epq_active to es_epqstate
es_epq_active was introduced in 27cc7cd, in the original discussion,
Andres mentioned he was *trying to signal that EPQ is currently
active from the POV of executor nodes* when choosing es_epq_active as
a field of EState. It seems to me we can rename es_epq_active to
es_epqstate and a non-null value would convey the same meaning.
Besides, a field name ending with "active" typically suggests a
boolean value.
---
src/backend/executor/execMain.c | 4 ++--
src/backend/executor/execScan.c | 8 ++++----
src/backend/executor/nodeAppend.c | 2 +-
src/backend/executor/nodeForeignscan.c | 14 +++++++-------
src/backend/executor/nodeIndexonlyscan.c | 6 +++---
src/backend/executor/nodeIndexscan.c | 6 +++---
src/backend/executor/nodeModifyTable.c | 2 +-
src/include/nodes/execnodes.h | 2 +-
8 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index fb8dba3ab2c..b21b708f3a2 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -925,7 +925,7 @@ InitPlan(QueryDesc *queryDesc, int eflags)
estate->es_tupleTable = NIL;
/* signal that this EState is not used for EPQ */
- estate->es_epq_active = NULL;
+ estate->es_epqstate = NULL;
/*
* Initialize private state information for each SubPlan. We must do this
@@ -2842,7 +2842,7 @@ EvalPlanQualStart(EPQState *epqstate, Plan *planTree)
oldcontext = MemoryContextSwitchTo(rcestate->es_query_cxt);
/* signal that this is an EState for executing EPQ */
- rcestate->es_epq_active = epqstate;
+ rcestate->es_epqstate = epqstate;
/*
* Child EPQ EStates share the parent's copy of unchanging state such as
diff --git a/src/backend/executor/execScan.c b/src/backend/executor/execScan.c
index 556a5d98e78..b6391a2539d 100644
--- a/src/backend/executor/execScan.c
+++ b/src/backend/executor/execScan.c
@@ -39,9 +39,9 @@ ExecScanFetch(ScanState *node,
CHECK_FOR_INTERRUPTS();
- if (estate->es_epq_active != NULL)
+ if (estate->es_epqstate != NULL)
{
- EPQState *epqstate = estate->es_epq_active;
+ EPQState *epqstate = estate->es_epqstate;
/*
* We are inside an EvalPlanQual recheck. Return the test tuple if
@@ -308,9 +308,9 @@ ExecScanReScan(ScanState *node)
* Rescan EvalPlanQual tuple(s) if we're inside an EvalPlanQual recheck.
* But don't lose the "blocked" status of blocked target relations.
*/
- if (estate->es_epq_active != NULL)
+ if (estate->es_epqstate != NULL)
{
- EPQState *epqstate = estate->es_epq_active;
+ EPQState *epqstate = estate->es_epqstate;
Index scanrelid = ((Scan *) node->ps.plan)->scanrelid;
if (scanrelid > 0)
diff --git a/src/backend/executor/nodeAppend.c b/src/backend/executor/nodeAppend.c
index 0bd0e4e54d3..1471678ee90 100644
--- a/src/backend/executor/nodeAppend.c
+++ b/src/backend/executor/nodeAppend.c
@@ -200,7 +200,7 @@ ExecInitAppend(Append *node, EState *estate, int eflags)
* as sync ones; don't do this when initializing an EvalPlanQual plan
* tree.
*/
- if (initNode->async_capable && estate->es_epq_active == NULL)
+ if (initNode->async_capable && estate->es_epqstate == NULL)
{
asyncplans = bms_add_member(asyncplans, j);
nasyncplans++;
diff --git a/src/backend/executor/nodeForeignscan.c b/src/backend/executor/nodeForeignscan.c
index 9c56c2f3acf..75fd8cf9390 100644
--- a/src/backend/executor/nodeForeignscan.c
+++ b/src/backend/executor/nodeForeignscan.c
@@ -53,7 +53,7 @@ ForeignNext(ForeignScanState *node)
* direct modifications cannot be re-evaluated, so shouldn't get here
* during EvalPlanQual processing
*/
- Assert(node->ss.ps.state->es_epq_active == NULL);
+ Assert(node->ss.ps.state->es_epqstate == NULL);
slot = node->fdwroutine->IterateDirectModify(node);
}
@@ -125,7 +125,7 @@ ExecForeignScan(PlanState *pstate)
* Ignore direct modifications when EvalPlanQual is active --- they are
* irrelevant for EvalPlanQual rechecking
*/
- if (estate->es_epq_active != NULL && plan->operation != CMD_SELECT)
+ if (estate->es_epqstate != NULL && plan->operation != CMD_SELECT)
return NULL;
return ExecScan(&node->ss,
@@ -230,7 +230,7 @@ ExecInitForeignScan(ForeignScan *node, EState *estate, int eflags)
* this has to be kept in sync with the code in ExecInitAppend().
*/
scanstate->ss.ps.async_capable = (((Plan *) node)->async_capable &&
- estate->es_epq_active == NULL);
+ estate->es_epqstate == NULL);
/*
* Initialize FDW-related state.
@@ -249,7 +249,7 @@ ExecInitForeignScan(ForeignScan *node, EState *estate, int eflags)
* EvalPlanQual processing, EvalPlanQual only initializes the subtree
* under the ModifyTable, and doesn't run ExecInitModifyTable.
*/
- if (node->resultRelation > 0 && estate->es_epq_active == NULL)
+ if (node->resultRelation > 0 && estate->es_epqstate == NULL)
{
if (estate->es_result_relations == NULL ||
estate->es_result_relations[node->resultRelation - 1] == NULL)
@@ -278,7 +278,7 @@ ExecInitForeignScan(ForeignScan *node, EState *estate, int eflags)
* so we need to ignore such ForeignScan nodes during EvalPlanQual
* processing. See also ExecForeignScan/ExecReScanForeignScan.
*/
- if (estate->es_epq_active == NULL)
+ if (estate->es_epqstate == NULL)
fdwroutine->BeginDirectModify(scanstate, eflags);
}
else
@@ -302,7 +302,7 @@ ExecEndForeignScan(ForeignScanState *node)
/* Let the FDW shut down */
if (plan->operation != CMD_SELECT)
{
- if (estate->es_epq_active == NULL)
+ if (estate->es_epqstate == NULL)
node->fdwroutine->EndDirectModify(node);
}
else
@@ -330,7 +330,7 @@ ExecReScanForeignScan(ForeignScanState *node)
* Ignore direct modifications when EvalPlanQual is active --- they are
* irrelevant for EvalPlanQual rechecking
*/
- if (estate->es_epq_active != NULL && plan->operation != CMD_SELECT)
+ if (estate->es_epqstate != NULL && plan->operation != CMD_SELECT)
return;
node->fdwroutine->ReScanForeignScan(node);
diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c
index e6635233155..ba9d7794eaf 100644
--- a/src/backend/executor/nodeIndexonlyscan.c
+++ b/src/backend/executor/nodeIndexonlyscan.c
@@ -433,7 +433,7 @@ void
ExecIndexOnlyMarkPos(IndexOnlyScanState *node)
{
EState *estate = node->ss.ps.state;
- EPQState *epqstate = estate->es_epq_active;
+ EPQState *epqstate = estate->es_epqstate;
if (epqstate != NULL)
{
@@ -470,9 +470,9 @@ void
ExecIndexOnlyRestrPos(IndexOnlyScanState *node)
{
EState *estate = node->ss.ps.state;
- EPQState *epqstate = estate->es_epq_active;
+ EPQState *epqstate = estate->es_epqstate;
- if (estate->es_epq_active != NULL)
+ if (estate->es_epqstate != NULL)
{
/* See comments in ExecIndexMarkPos */
Index scanrelid = ((Scan *) node->ss.ps.plan)->scanrelid;
diff --git a/src/backend/executor/nodeIndexscan.c b/src/backend/executor/nodeIndexscan.c
index 3b2275e8fe9..5cc02306e1f 100644
--- a/src/backend/executor/nodeIndexscan.c
+++ b/src/backend/executor/nodeIndexscan.c
@@ -813,7 +813,7 @@ void
ExecIndexMarkPos(IndexScanState *node)
{
EState *estate = node->ss.ps.state;
- EPQState *epqstate = estate->es_epq_active;
+ EPQState *epqstate = estate->es_epqstate;
if (epqstate != NULL)
{
@@ -850,9 +850,9 @@ void
ExecIndexRestrPos(IndexScanState *node)
{
EState *estate = node->ss.ps.state;
- EPQState *epqstate = estate->es_epq_active;
+ EPQState *epqstate = estate->es_epqstate;
- if (estate->es_epq_active != NULL)
+ if (estate->es_epqstate != NULL)
{
/* See comments in ExecIndexMarkPos */
Index scanrelid = ((Scan *) node->ss.ps.plan)->scanrelid;
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index bc82e035ba2..e082e4c0710 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -3998,7 +3998,7 @@ ExecModifyTable(PlanState *pstate)
* case it is within a CTE subplan. Hence this test must be here, not in
* ExecInitModifyTable.)
*/
- if (estate->es_epq_active != NULL)
+ if (estate->es_epqstate != NULL)
elog(ERROR, "ModifyTable should not be called during EvalPlanQual");
/*
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index d0f2dca5928..30db314d1b0 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -720,7 +720,7 @@ typedef struct EState
* need to perform EPQ related work, and to provide necessary information
* to do so.
*/
- struct EPQState *es_epq_active;
+ struct EPQState *es_epqstate;
bool es_use_parallel_mode; /* can we use parallel workers? */
--
2.39.5
Junwang Zhao <zhjwpku@gmail.com> writes:
ISTM Andres tend to use *es_epq_active* in a boolean way,
like `if (es_epq_active) then`, but in the code base, all its usages
follow pattern `if (es_epq_active == NULL) then`, so I propose to
change es_epq_active to es_epqstate.
While I didn't especially love "es_epq_active" at the time,
I don't see that "es_epqstate" is much of an improvement:
it's an extremely generic name that conveys little information.
And renaming it now, years later, seems to add little except
back-patching hazards. So I'd vote for leaving it alone.
regards, tom lane
On Sat, Jan 18, 2025 at 9:43 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Junwang Zhao <zhjwpku@gmail.com> writes:
ISTM Andres tend to use *es_epq_active* in a boolean way,
like `if (es_epq_active) then`, but in the code base, all its usages
follow pattern `if (es_epq_active == NULL) then`, so I propose to
change es_epq_active to es_epqstate.While I didn't especially love "es_epq_active" at the time,
I don't see that "es_epqstate" is much of an improvement:
it's an extremely generic name that conveys little information.
And renaming it now, years later, seems to add little except
back-patching hazards. So I'd vote for leaving it alone.regards, tom lane
Ok, let's keep it as is, thanks for the explanation.
--
Regards
Junwang Zhao