There's a complain from Coverity about outer_plan being referenced while
possibly NULL, which can be silenced by using an existing local
variable. 0001 does that.
0002 and 0003 are unrelated: in the former, we avoid creating a separate
local variable planSlot when we can just refer to the eponymous member
of ModifyTableContext. In the latter, we reduce the scope where
'lockmode' is defined by moving it from ModifyTableContext to
UpdateContext, which means we can save initializing it in a few spots;
this makes the code more natural.
I expect these fixups in new code should be uncontroversial.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
>From 30a3bfadac90fabba239533a754dfe4efb85a5d8 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Tue, 19 Apr 2022 13:57:24 +0200
Subject: [PATCH 1/3] reuse local variable, to silence coverity
---
src/backend/utils/adt/ruleutils.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 7e08d7fe6c..5d49f564a2 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -4996,7 +4996,7 @@ set_deparse_plan(deparse_namespace *dpns, Plan *plan)
if (IsA(plan, ModifyTable))
{
if (((ModifyTable *) plan)->operation == CMD_MERGE)
- dpns->inner_tlist = dpns->outer_plan->targetlist;
+ dpns->inner_tlist = dpns->outer_tlist;
else
dpns->inner_tlist = ((ModifyTable *) plan)->exclRelTlist;
}
--
2.30.2
>From 309e6f9717890713dde6b2a62cf20dcafdc176d0 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Tue, 19 Apr 2022 13:57:08 +0200
Subject: [PATCH 2/3] use context.planSlot instead of having a separate
planSlot
---
src/backend/executor/nodeModifyTable.c | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 171575cd73..0de6abd5bb 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -3452,7 +3452,6 @@ ExecModifyTable(PlanState *pstate)
ResultRelInfo *resultRelInfo;
PlanState *subplanstate;
TupleTableSlot *slot;
- TupleTableSlot *planSlot;
TupleTableSlot *oldSlot;
ItemPointerData tuple_ctid;
HeapTupleData oldtupdata;
@@ -3525,10 +3524,10 @@ ExecModifyTable(PlanState *pstate)
if (pstate->ps_ExprContext)
ResetExprContext(pstate->ps_ExprContext);
- planSlot = ExecProcNode(subplanstate);
+ context.planSlot = ExecProcNode(subplanstate);
/* No more tuples to process? */
- if (TupIsNull(planSlot))
+ if (TupIsNull(context.planSlot))
break;
/*
@@ -3542,7 +3541,7 @@ ExecModifyTable(PlanState *pstate)
bool isNull;
Oid resultoid;
- datum = ExecGetJunkAttribute(planSlot, node->mt_resultOidAttno,
+ datum = ExecGetJunkAttribute(context.planSlot, node->mt_resultOidAttno,
&isNull);
if (isNull)
{
@@ -3556,9 +3555,8 @@ ExecModifyTable(PlanState *pstate)
*/
if (operation == CMD_MERGE)
{
- EvalPlanQualSetSlot(&node->mt_epqstate, planSlot);
+ EvalPlanQualSetSlot(&node->mt_epqstate, context.planSlot);
- context.planSlot = planSlot;
context.lockmode = 0;
ExecMerge(&context, node->resultRelInfo, NULL, node->canSetTag);
@@ -3589,13 +3587,13 @@ ExecModifyTable(PlanState *pstate)
* ExecProcessReturning by IterateDirectModify, so no need to
* provide it here.
*/
- slot = ExecProcessReturning(resultRelInfo, NULL, planSlot);
+ slot = ExecProcessReturning(resultRelInfo, NULL, context.planSlot);
return slot;
}
- EvalPlanQualSetSlot(&node->mt_epqstate, planSlot);
- slot = planSlot;
+ EvalPlanQualSetSlot(&node->mt_epqstate, context.planSlot);
+ slot = context.planSlot;
tupleid = NULL;
oldtuple = NULL;
@@ -3637,9 +3635,8 @@ ExecModifyTable(PlanState *pstate)
{
if (operation == CMD_MERGE)
{
- EvalPlanQualSetSlot(&node->mt_epqstate, planSlot);
+ EvalPlanQualSetSlot(&node->mt_epqstate, context.planSlot);
- context.planSlot = planSlot;
context.lockmode = 0;
ExecMerge(&context, node->resultRelInfo, NULL, node->canSetTag);
@@ -3698,7 +3695,6 @@ ExecModifyTable(PlanState *pstate)
}
/* complete context setup */
- context.planSlot = planSlot;
context.lockmode = 0;
switch (operation)
@@ -3707,7 +3703,7 @@ ExecModifyTable(PlanState *pstate)
/* Initialize projection info if first time for this table */
if (unlikely(!resultRelInfo->ri_projectNewInfoValid))
ExecInitInsertProjection(node, resultRelInfo);
- slot = ExecGetInsertNewTuple(resultRelInfo, planSlot);
+ slot = ExecGetInsertNewTuple(resultRelInfo, context.planSlot);
slot = ExecInsert(&context, resultRelInfo, slot,
node->canSetTag, NULL, NULL);
break;
@@ -3737,7 +3733,7 @@ ExecModifyTable(PlanState *pstate)
oldSlot))
elog(ERROR, "failed to fetch tuple being updated");
}
- slot = internalGetUpdateNewTuple(resultRelInfo, planSlot,
+ slot = internalGetUpdateNewTuple(resultRelInfo, context.planSlot,
oldSlot, NULL);
context.GetUpdateNewTuple = internalGetUpdateNewTuple;
context.relaction = NULL;
--
2.30.2
>From 710bec98f6be0629973c715ff5440782b25ca36d Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Tue, 19 Apr 2022 15:37:23 +0200
Subject: [PATCH 3/3] Move ModifyTableContext->lockmode to UpdateContext
This avoids some pointless initialization, and better contains the
variable to exist in the scope where it is really used.
---
src/backend/executor/nodeModifyTable.c | 23 ++++++++---------------
1 file changed, 8 insertions(+), 15 deletions(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 0de6abd5bb..982acfdad9 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -116,12 +116,6 @@ typedef struct ModifyTableContext
* cross-partition UPDATE
*/
TupleTableSlot *cpUpdateReturningSlot;
-
- /*
- * Lock mode to acquire on the latest tuple version before performing
- * EvalPlanQual on it
- */
- LockTupleMode lockmode;
} ModifyTableContext;
/*
@@ -132,6 +126,12 @@ typedef struct UpdateContext
bool updated; /* did UPDATE actually occur? */
bool updateIndexes; /* index update required? */
bool crossPartUpdate; /* was it a cross-partition update? */
+
+ /*
+ * Lock mode to acquire on the latest tuple version before performing
+ * EvalPlanQual on it
+ */
+ LockTupleMode lockmode;
} UpdateContext;
@@ -1971,7 +1971,7 @@ lreplace:;
estate->es_snapshot,
estate->es_crosscheck_snapshot,
true /* wait for commit */ ,
- &context->tmfd, &context->lockmode,
+ &context->tmfd, &updateCxt->lockmode,
&updateCxt->updateIndexes);
if (result == TM_Ok)
updateCxt->updated = true;
@@ -2251,7 +2251,7 @@ redo_act:
result = table_tuple_lock(resultRelationDesc, tupleid,
estate->es_snapshot,
inputslot, estate->es_output_cid,
- context->lockmode, LockWaitBlock,
+ updateCxt.lockmode, LockWaitBlock,
TUPLE_LOCK_FLAG_FIND_LAST_VERSION,
&context->tmfd);
@@ -3557,8 +3557,6 @@ ExecModifyTable(PlanState *pstate)
{
EvalPlanQualSetSlot(&node->mt_epqstate, context.planSlot);
- context.lockmode = 0;
-
ExecMerge(&context, node->resultRelInfo, NULL, node->canSetTag);
continue; /* no RETURNING support yet */
}
@@ -3637,8 +3635,6 @@ ExecModifyTable(PlanState *pstate)
{
EvalPlanQualSetSlot(&node->mt_epqstate, context.planSlot);
- context.lockmode = 0;
-
ExecMerge(&context, node->resultRelInfo, NULL, node->canSetTag);
continue; /* no RETURNING support yet */
}
@@ -3694,9 +3690,6 @@ ExecModifyTable(PlanState *pstate)
}
}
- /* complete context setup */
- context.lockmode = 0;
-
switch (operation)
{
case CMD_INSERT:
--
2.30.2
