public inbox for [email protected]
help / color / mirror / Atom feedFrom: Amit Langote <[email protected]>
To: Dean Rasheed <[email protected]>
Cc: Tender Wang <[email protected]>
Cc: Bh W <[email protected]>
Cc: Laurenz Albe <[email protected]>
Cc: [email protected]
Subject: Re: BUG #19355: Attempt to insert data unexpectedly during concurrent update
Date: Thu, 15 Jan 2026 20:39:01 +0900
Message-ID: <CA+HiwqGSogk5__9rFp9F6TGDpyM11CTVQHy5GRptGZC3A4DwwA@mail.gmail.com> (raw)
In-Reply-To: <CA+HiwqFC8OssEd-sAixUiZetiMGz8BGbbn6GJC8krUQx2J=huQ@mail.gmail.com>
References: <[email protected]>
<[email protected]>
<CANjBVm3AMm=9-mB7=uqJjaWN-+RrA8+w-gXhZn6L7VHu7K_Urw@mail.gmail.com>
<CAEZATCVcV4cfqGnmPGVxwjsPh7rxsTD510=KOGFyi2_FMgqWaQ@mail.gmail.com>
<CA+HiwqE8gNK7Utqo2EAKh24KExWfypt06m1EFNXBd5TFDO2f6w@mail.gmail.com>
<CAHewXNn8ApcYfZKjZ1R8Z7yHRoi+C+mu5r9LbZUbb6691d6Rrw@mail.gmail.com>
<CAEZATCVr+=F=iTbi3XyABSEr5Cqw=ptm2PyGc75SaQ1JYrPnsg@mail.gmail.com>
<CA+HiwqGCfYgT1DnnK836qRX63okKNeNR=CQmshC6=aATCw6VVA@mail.gmail.com>
<CAEZATCVwnjNYNSLLTQCXgwhgGEascKHu+hEtDVn7PF4ZCZvm8g@mail.gmail.com>
<CA+HiwqFC8OssEd-sAixUiZetiMGz8BGbbn6GJC8krUQx2J=huQ@mail.gmail.com>
On Thu, Jan 8, 2026 at 10:16 PM Amit Langote <[email protected]> wrote:
> On Wed, Jan 7, 2026 at 9:52 PM Dean Rasheed <[email protected]> wrote:
> > On Wed, 7 Jan 2026 at 09:37, Amit Langote <[email protected]> wrote:
> > >
> > > Yes, I think the approach in your patch seems better for the reason
> > > you mentioned, at least for back-patching sanity.
> > >
> > > I intended all of these relid sets to account for prunable RELATION RTEs only.
> >
> > Yes, I think that makes sense.
> >
> > > Thanks Tender and Bernice for the additional analysis. I prefer Dean's
> > > fix-the-executor approach for back-patching. Bernice, are there other
> > > related issues you're aware of beyond this rowmark bug? Want to make
> > > sure Dean's patch covers them too.
> >
> > It looks to me as though either approach would work, so I'm happy for
> > you to decide which approach fits best with your design.
>
> Ok, I thought about this some more.
>
> I admit I'd be lying if I said I didn't have doubts about my original
> design. It might be better for PlannedStmt.unprunableRelids and
> es_unpruned_relids to cover *all* RTEs except those that are prunable
> (decided by the planner) or pruned (decided during executor startup).
> That way, code checking these sets wouldn't need to also verify the
> RTE kind, as your patch currently does.
>
> If we were to change the design, we could remove
> PlannerGlobal.allRelids entirely on master, since it would no longer
> need to be selectively populated -- unprunableRelids could just be
> computed directly from the full rtable. But that would mean we'd be
> leaving v18 with an unused field in PlannerGlobal. That's not great,
> but having different designs in the two branches isn't ideal either.
>
> So I'll go with your minimal fix for the back-patch. We can revisit
> the design cleanup on master later if desired.
>
> > > Thanks for the patch! Do you intend to commit and back-patch this
> > > yourself, or would you like me to handle it?
> >
> > It's your code, and you're more familiar with it than me, so I'm happy
> > to leave it to you :-)
>
> Surely.
Here's Dean patch with a commit message added. I plan to commit it
barring objections.
--
Thanks, Amit Langote
Attachments:
[application/octet-stream] v1-0001-Fix-rowmark-handling-for-non-relation-RTEs-during.patch (7.6K, 2-v1-0001-Fix-rowmark-handling-for-non-relation-RTEs-during.patch)
download | inline diff:
From 2ab058afde9517a12a80f59821439a9c2a63a8f9 Mon Sep 17 00:00:00 2001
From: Amit Langote <[email protected]>
Date: Thu, 15 Jan 2026 19:00:11 +0900
Subject: [PATCH v1] Fix rowmark handling for non-relation RTEs during executor
init
Commit cbc127917e introduced tracking of unpruned relids to skip
processing of pruned partitions. PlannedStmt.unprunableRelids is
computed as the difference between PlannerGlobal.allRelids and
prunableRelids, but allRelids only contains RTE_RELATION entries.
This means non-relation RTEs (VALUES, subqueries, CTEs, etc.) are
never included in unprunableRelids, and consequently not in
es_unpruned_relids at runtime.
As a result, rowmarks attached to non-relation RTEs were incorrectly
skipped during executor initialization. This affects any DML statement
that has rowmarks on such RTEs, including MERGE with a VALUES or
subquery source, and UPDATE/DELETE with joins against subqueries or
CTEs. When a concurrent update triggers an EPQ recheck, the missing
rowmark leads to incorrect results.
Fix by restricting the es_unpruned_relids membership check to
RTE_RELATION entries only, since partition pruning only applies to
actual relations. Rowmarks for other RTE kinds are now always
processed.
Bug: #19355
Reported-by: Bihua Wang <[email protected]>
Diagnosed-by: Dean Rasheed <[email protected]>
Diagnosed-by: Tender Wang <[email protected]>
Author: Dean Rasheed <[email protected]>
Discussion: https://postgr.es/m/[email protected]
Backpatch-through: 18
---
src/backend/executor/execMain.c | 14 ++++++++-----
src/backend/executor/nodeLockRows.c | 10 +++++++---
src/backend/executor/nodeModifyTable.c | 10 +++++++---
.../isolation/expected/eval-plan-qual.out | 20 +++++++++++++++++++
src/test/isolation/specs/eval-plan-qual.spec | 13 ++++++++++++
5 files changed, 56 insertions(+), 11 deletions(-)
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index ca14cdabdd0..bfd3ebc601e 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -880,21 +880,25 @@ InitPlan(QueryDesc *queryDesc, int eflags)
foreach(l, plannedstmt->rowMarks)
{
PlanRowMark *rc = (PlanRowMark *) lfirst(l);
+ RangeTblEntry *rte = exec_rt_fetch(rc->rti, estate);
Oid relid;
Relation relation;
ExecRowMark *erm;
+ /* ignore "parent" rowmarks; they are irrelevant at runtime */
+ if (rc->isParent)
+ continue;
+
/*
- * Ignore "parent" rowmarks, because they are irrelevant at
- * runtime. Also ignore the rowmarks belonging to child tables
- * that have been pruned in ExecDoInitialPruning().
+ * Also ignore rowmarks belonging to child tables that have been
+ * pruned in ExecDoInitialPruning().
*/
- if (rc->isParent ||
+ if (rte->rtekind == RTE_RELATION &&
!bms_is_member(rc->rti, estate->es_unpruned_relids))
continue;
/* get relation's OID (will produce InvalidOid if subquery) */
- relid = exec_rt_fetch(rc->rti, estate)->relid;
+ relid = rte->relid;
/* open relation, if we need to access it for this mark type */
switch (rc->markType)
diff --git a/src/backend/executor/nodeLockRows.c b/src/backend/executor/nodeLockRows.c
index 67bee922a3c..8d865470780 100644
--- a/src/backend/executor/nodeLockRows.c
+++ b/src/backend/executor/nodeLockRows.c
@@ -344,15 +344,19 @@ ExecInitLockRows(LockRows *node, EState *estate, int eflags)
foreach(lc, node->rowMarks)
{
PlanRowMark *rc = lfirst_node(PlanRowMark, lc);
+ RangeTblEntry *rte = exec_rt_fetch(rc->rti, estate);
ExecRowMark *erm;
ExecAuxRowMark *aerm;
+ /* ignore "parent" rowmarks; they are irrelevant at runtime */
+ if (rc->isParent)
+ continue;
+
/*
- * Ignore "parent" rowmarks, because they are irrelevant at runtime.
- * Also ignore the rowmarks belonging to child tables that have been
+ * Also ignore rowmarks belonging to child tables that have been
* pruned in ExecDoInitialPruning().
*/
- if (rc->isParent ||
+ if (rte->rtekind == RTE_RELATION &&
!bms_is_member(rc->rti, estate->es_unpruned_relids))
continue;
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 46ff6da8289..7d7411a7056 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -5092,15 +5092,19 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
foreach(l, node->rowMarks)
{
PlanRowMark *rc = lfirst_node(PlanRowMark, l);
+ RangeTblEntry *rte = exec_rt_fetch(rc->rti, estate);
ExecRowMark *erm;
ExecAuxRowMark *aerm;
+ /* ignore "parent" rowmarks; they are irrelevant at runtime */
+ if (rc->isParent)
+ continue;
+
/*
- * Ignore "parent" rowmarks, because they are irrelevant at runtime.
- * Also ignore the rowmarks belonging to child tables that have been
+ * Also ignore rowmarks belonging to child tables that have been
* pruned in ExecDoInitialPruning().
*/
- if (rc->isParent ||
+ if (rte->rtekind == RTE_RELATION &&
!bms_is_member(rc->rti, estate->es_unpruned_relids))
continue;
diff --git a/src/test/isolation/expected/eval-plan-qual.out b/src/test/isolation/expected/eval-plan-qual.out
index 05fffe0d570..a122047fd2a 100644
--- a/src/test/isolation/expected/eval-plan-qual.out
+++ b/src/test/isolation/expected/eval-plan-qual.out
@@ -1473,3 +1473,23 @@ step s2pp4: DELETE FROM another_parttbl WHERE a = (SELECT 1); <waiting ...>
step c1: COMMIT;
step s2pp4: <... completed>
step c2: COMMIT;
+
+starting permutation: updateformergevalues mergevalues c1 c2 read
+step updateformergevalues: UPDATE accounts SET balance = balance + 100;
+step mergevalues:
+ MERGE INTO accounts
+ USING (VALUES ('checking', 610), ('savings', 620)) v(accountid, balance)
+ ON v.accountid = accounts.accountid
+ WHEN MATCHED THEN UPDATE SET balance = v.balance
+ WHEN NOT MATCHED THEN INSERT VALUES ('unmatched', -1);
+ <waiting ...>
+step c1: COMMIT;
+step mergevalues: <... completed>
+step c2: COMMIT;
+step read: SELECT * FROM accounts ORDER BY accountid;
+accountid|balance|balance2
+---------+-------+--------
+checking | 610| 1220
+savings | 620| 1240
+(2 rows)
+
diff --git a/src/test/isolation/specs/eval-plan-qual.spec b/src/test/isolation/specs/eval-plan-qual.spec
index 80e1e6bb307..fb57fb237dd 100644
--- a/src/test/isolation/specs/eval-plan-qual.spec
+++ b/src/test/isolation/specs/eval-plan-qual.spec
@@ -206,6 +206,8 @@ step sys1 {
step s1pp1 { UPDATE another_parttbl SET b = b + 1 WHERE a = 1; }
+step updateformergevalues { UPDATE accounts SET balance = balance + 100; }
+
session s2
setup { BEGIN ISOLATION LEVEL READ COMMITTED; }
step wx2 { UPDATE accounts SET balance = balance + 450 WHERE accountid = 'checking' RETURNING balance; }
@@ -318,6 +320,14 @@ step s2pp2 { PREPARE epd AS DELETE FROM another_parttbl WHERE a = $1; }
step s2pp3 { EXECUTE epd(1); }
step s2pp4 { DELETE FROM another_parttbl WHERE a = (SELECT 1); }
+step mergevalues {
+ MERGE INTO accounts
+ USING (VALUES ('checking', 610), ('savings', 620)) v(accountid, balance)
+ ON v.accountid = accounts.accountid
+ WHEN MATCHED THEN UPDATE SET balance = v.balance
+ WHEN NOT MATCHED THEN INSERT VALUES ('unmatched', -1);
+}
+
session s3
setup { BEGIN ISOLATION LEVEL READ COMMITTED; }
step read { SELECT * FROM accounts ORDER BY accountid; }
@@ -425,3 +435,6 @@ permutation sys1 sysmerge2 c1 c2
# Exercise run-time partition pruning code in an EPQ recheck
permutation s1pp1 s2pp1 s2pp2 s2pp3 c1 c2
permutation s1pp1 s2pp4 c1 c2
+
+# test EPQ recheck in MERGE from VALUES_RTE, cf bug #19355
+permutation updateformergevalues mergevalues c1 c2 read
--
2.47.3
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: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: BUG #19355: Attempt to insert data unexpectedly during concurrent update
In-Reply-To: <CA+HiwqGSogk5__9rFp9F6TGDpyM11CTVQHy5GRptGZC3A4DwwA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox