From 84ff5a8849a5542f071974cb3996fdb2e30e5de8 Mon Sep 17 00:00:00 2001 From: Tender Wang 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