pg.ddx.io pgsql-hackers@postgresql.org mailing list archive help / color / mirror / Atom feed From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: minor MERGE cleanups
Date: Tue, 19 Apr 2022 15:45:22 +0200
Message-ID: <202204191345.qerjy3kxi3eb@alvherre.pgsql> (raw )
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/
Attachments: download all (3 files, .tar.gz)
0001-reuse-local-variable-to-silence-coverity.patch (text/x-diff, 859B) download
From 30a3bfadac90fabba239533a754dfe4efb85a5d8 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@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
0002-use-context.planSlot-instead-of-having-a-separate-pl.patch (text/x-diff, 3.7K) download
From 309e6f9717890713dde6b2a62cf20dcafdc176d0 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@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
0003-Move-ModifyTableContext-lockmode-to-UpdateContext.patch (text/x-diff, 2.7K) download
From 710bec98f6be0629973c715ff5440782b25ca36d Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@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
view thread (3+ messages) latest in thread
Message-ID: <202204191345.qerjy3kxi3eb@alvherre.pgsql>
Permalink: ../202204191345.qerjy3kxi3eb@alvherre.pgsql/
Also on: postgresql.org/message-id/202204191345.qerjy3kxi3eb@alvherre.pgsql
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-hackers@postgresql.org
Cc: alvherre@alvh.no-ip.org, pgsql-hackers@lists.postgresql.org
Subject: Re: minor MERGE cleanups
In-Reply-To: <202204191345.qerjy3kxi3eb@alvherre.pgsql>
* 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