Tom Lane <[email protected]> 于2026年10月26日周一 11:29写道： > > Agreed that there's separable issues there. Since you seem to be > interested in pursuing this, I attach the patch I had as far as > I'd taken it (I stopped upon realizing that preprocess_rowmarks > prevents a lot of the new code from being reached). You could > compare this to yours, which I've not looked at, and take the better > bits from each.
Thanks for sharing your WIP patch. After comparing it with mine, I confirmed your observation that preprocess_rowmarks() makes the cases where only one SJE candidate has a PlanRowMark unreachable. When row marks are needed, every non-target base relation receives one, and target relations are not SJE candidates. Thus, the two candidate relations must either both have row marks, or both lack them. The attached v3 is now a two-patch series: 0001 documents this invariant, adds assertions for it, and removes unreachable code that transfers an rmark when there is no kmark. 0002 is based on your WIP patch. It allows SJE for locking row marks of different strengths, while removing the unreachable single-rowmark handling. It retains the stronger lock and its associated waitPolicy; for equal lock strengths, it takes the greater waitPolicy, as applyLockingClause() does. -- Thanks, Tender Wang
From 8ed48fbee7f16834f9e06dc2bbcfb4aee38d28a0 Mon Sep 17 00:00:00 2001 From: Tender Wang <[email protected]> Date: Mon, 26 Oct 2026 10:59:30 +0800 Subject: [PATCH v3 1/2] Clarify row mark handling in self-join elimination When row marks are needed, preprocess_rowmarks() creates one for every non-target base relation. Since target relations are not candidates for self-join elimination, a pair of candidate relations must either both have row marks or both lack them. Therefore, the code that transfers the removed relation's row mark when the kept relation has none is unreachable. Remove that code, add assertions for the invariant, and expand the comments explaining why relations with different row mark types are not merged. --- src/backend/optimizer/plan/analyzejoins.c | 40 +++++++++-------------- 1 file changed, 16 insertions(+), 24 deletions(-) diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c index 7aad91fd34e..dcfe5097586 100644 --- a/src/backend/optimizer/plan/analyzejoins.c +++ b/src/backend/optimizer/plan/analyzejoins.c @@ -1287,28 +1287,13 @@ remove_self_join_rel(PlannerInfo *root, Assert(found_relid); /* - * If the removed relation has a row mark, transfer it to the remaining - * one. - * - * If both rels have row marks, just keep the one corresponding to the - * remaining relation because we verified earlier that they have the same - * strength. + * If the relations have row marks, discard the removed relation's mark. + * The caller has verified that both marks exist and have the same type. */ if (rmark) { - if (kmark) - { - Assert(kmark->markType == rmark->markType); - - root->rowMarks = list_delete_ptr(root->rowMarks, rmark); - } - else - { - /* Shouldn't have inheritance children yet. */ - Assert(rmark->rti == rmark->prti); - - rmark->rti = rmark->prti = toKeep->relid; - } + Assert(rmark->markType == kmark->markType); + root->rowMarks = list_delete_ptr(root->rowMarks, rmark); } } @@ -1827,10 +1812,16 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids) continue; /* - * Check Row Marks equivalence. We can't remove the join if the - * relations have row marks of different strength (e.g., one is - * locked FOR UPDATE, and another just has ROW_MARK_REFERENCE for - * EvalPlanQual rechecking). + * If row marks are needed, preprocess_rowmarks() creates one for + * every non-target base relation. Target relations aren't SJE + * candidates, so these relations must either both have row marks or + * both lack them. + * + * Don't merge relations with different row mark types. In + * particular, a locking row mark can fetch an updated version of a + * tuple, while a non-locking row mark preserves the version found by + * the original scan. We also conservatively reject different locking + * row mark types rather than try to combine their lock strengths. */ foreach(lc, root->rowMarks) { @@ -1850,7 +1841,8 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids) if (kmark && rmark) break; } - if (kmark && rmark && kmark->markType != rmark->markType) + Assert((kmark == NULL) == (rmark == NULL)); + if (kmark && kmark->markType != rmark->markType) continue; /* -- 2.43.0
From 84ff5a8849a5542f071974cb3996fdb2e30e5de8 Mon Sep 17 00:00:00 2001 From: Tender Wang <[email protected]> Date: Mon, 26 Oct 2026 16:32:26 +0800 Subject: [PATCH v3 2/2] Allow self-join elimination with different row lock strengths Once self-join elimination has proven that two relations identify the same row, their locking row marks can be combined. Retain the stronger lock and its wait policy when their strengths differ. When the strengths are equal, retain the stronger wait policy, following applyLockingClause(). This avoids rejecting SJE solely because two locking row marks request different lock strengths, and avoids the lock-upgrade risk of acquiring the weaker lock first. Based on a patch by Tom Lane. --- src/backend/optimizer/plan/analyzejoins.c | 66 +++++++++++++++++++---- src/test/regress/expected/join.out | 11 ++++ src/test/regress/sql/join.sql | 5 ++ 3 files changed, 73 insertions(+), 9 deletions(-) diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c index dcfe5097586..90304f76fa1 100644 --- a/src/backend/optimizer/plan/analyzejoins.c +++ b/src/backend/optimizer/plan/analyzejoins.c @@ -1287,12 +1287,58 @@ remove_self_join_rel(PlannerInfo *root, Assert(found_relid); /* - * If the relations have row marks, discard the removed relation's mark. - * The caller has verified that both marks exist and have the same type. + * If the relations have row marks, merge their locking requirements into + * the kept relation's mark, then discard the removed relation's mark. + * The caller has rejected cases in which only one mark requires row + * locking. + * + * When the marks have different lock strengths, retain the stronger lock + * and its waitPolicy. This produces the same result as an execution of + * the unoptimized query in which the scan requesting the stronger lock + * reaches each tuple first. Choosing that ordering also avoids the risk + * of lock-upgrade deadlocks that would exist if the weaker lock were + * acquired first. + * + * For equal lock strengths, take the greater waitPolicy, as + * applyLockingClause() does when combining locking clauses for one RTE. */ if (rmark) { - Assert(rmark->markType == kmark->markType); + Assert(kmark != NULL); + Assert(kmark->rti == kmark->prti); + Assert(rmark->rti == rmark->prti); + Assert(!kmark->isParent); + Assert(!rmark->isParent); + + /* + * Since SJE only considers regular tables (not foreign tables), + * we don't expect to see ROW_MARK_COPY marks. It's important to + * check this since the associated row-identity Vars in the tlist + * are different for COPY marks. + */ + Assert(kmark->markType != ROW_MARK_COPY); + Assert(rmark->markType != ROW_MARK_COPY); + + /* Merge the lock strengths and waitPolicies as described above */ + if (rmark->strength > kmark->strength) + { + kmark->strength = rmark->strength; + /* copy fields derived from strength */ + kmark->markType = rmark->markType; + kmark->allMarkTypes = rmark->allMarkTypes; + /* adopt rmark's waitPolicy too */ + kmark->waitPolicy = rmark->waitPolicy; + } + else if (rmark->strength == kmark->strength) + { + /* adopt the greater waitPolicy */ + kmark->waitPolicy = Max(kmark->waitPolicy, rmark->waitPolicy); + } + /* + * Now drop the rmark from root->rowMarks. Ideally we'd also get + * rid of the rmark's associated row-identity Var in the tlist. + * But that's just a ctid, so it doesn't seem worth the trouble. + */ root->rowMarks = list_delete_ptr(root->rowMarks, rmark); } } @@ -1817,11 +1863,10 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids) * candidates, so these relations must either both have row marks or * both lack them. * - * Don't merge relations with different row mark types. In - * particular, a locking row mark can fetch an updated version of a - * tuple, while a non-locking row mark preserves the version found by - * the original scan. We also conservatively reject different locking - * row mark types rather than try to combine their lock strengths. + * We cannot merge a locking mark with a non-locking mark, since they + * can represent different sets of row values. Different locking mark + * types are compatible, however: remove_self_join_rel() will retain + * the stronger lock requirement. */ foreach(lc, root->rowMarks) { @@ -1842,7 +1887,10 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids) break; } Assert((kmark == NULL) == (rmark == NULL)); - if (kmark && kmark->markType != rmark->markType) + if (kmark && + kmark->markType != rmark->markType && + (!RowMarkRequiresRowShareLock(kmark->markType) || + !RowMarkRequiresRowShareLock(rmark->markType))) continue; /* diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 1e6aa380257..7969b34785d 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -10337,6 +10337,17 @@ SELECT a1.a FROM sj a1,sj a2 WHERE (a1.a=a2.a) FOR UPDATE; Filter: (a IS NOT NULL) (3 rows) +-- Different locking strengths can be merged, keeping the stronger one +EXPLAIN (COSTS OFF) +SELECT a1.a FROM sj a1,sj a2 WHERE (a1.a = a2.a) + FOR SHARE OF a1 FOR UPDATE OF a2; + QUERY PLAN +--------------------------------- + LockRows + -> Seq Scan on sj a2 + Filter: (a IS NOT NULL) +(3 rows) + reset enable_hashjoin; reset enable_mergejoin; -- diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 6264f23e081..abdcae28029 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -4026,6 +4026,11 @@ FROM sje_agg a JOIN sje_agg a2 ON a.id = a2.id; EXPLAIN (COSTS OFF) SELECT a1.a FROM sj a1,sj a2 WHERE (a1.a=a2.a) FOR UPDATE; +-- Different locking strengths can be merged, keeping the stronger one +EXPLAIN (COSTS OFF) +SELECT a1.a FROM sj a1,sj a2 WHERE (a1.a = a2.a) + FOR SHARE OF a1 FOR UPDATE OF a2; + reset enable_hashjoin; reset enable_mergejoin; -- 2.43.0
