pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedminor MERGE cleanups
3+ messages / 2 participants
[nested] [flat]
Commits from this thread:
2022-04-20 e70813fbc4 set_deparse_plan: Reuse variable to appease Coverity
2022-04-20 a87e759569 Move ModifyTableContext->lockmode to UpdateContext
2022-04-20 3dcc6bf406 ExecModifyTable: use context.planSlot instead of planSlot
* minor MERGE cleanups
@ 2022-04-19 13:45 Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 3+ messages in thread
From: Alvaro Herrera @ 2022-04-19 13:45 UTC (permalink / raw)
To: Pg Hackers <pgsql-hackers@lists.postgresql.org>
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
* Re: minor MERGE cleanups
@ 2022-04-20 01:29 Michael Paquier <michael@paquier.xyz>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 3+ messages in thread
From: Michael Paquier @ 2022-04-20 01:29 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, Apr 19, 2022 at 03:45:22PM +0200, Alvaro Herrera wrote:
> I expect these fixups in new code should be uncontroversial.
The whole set looks rather sane to me.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: minor MERGE cleanups
@ 2022-04-20 11:45 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 3+ messages in thread
From: Alvaro Herrera @ 2022-04-20 11:45 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
On 2022-Apr-20, Michael Paquier wrote:
> On Tue, Apr 19, 2022 at 03:45:22PM +0200, Alvaro Herrera wrote:
> > I expect these fixups in new code should be uncontroversial.
>
> The whole set looks rather sane to me.
Thank you, I have pushed them.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2022-04-20 11:45 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-04-19 13:45 minor MERGE cleanups Alvaro Herrera <alvherre@alvh.no-ip.org>
2022-04-20 01:29 ` Michael Paquier <michael@paquier.xyz>
2022-04-20 11:45 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
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