Tender Wang <[email protected]> writes:
> Tom Lane <[email protected]> 于2026年10月25日周日 08:56写道：
>> I experimented with writing logic that would allow merging locks of
>> different strengths, but I'm not really convinced that it's worth the
>> trouble.
> I haven't included the other improvement you suggested—performing SJE
> when lock strengths differ and retaining the stronger lock type, along
> with an appropriate rule for choosing the waitPolicy.
> I think it would be better to consider that separately from this cleanup
> patch.
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.
> During testing, I also noticed another possible missed optimization.
> For example, adding a restriction such as:
> postgres=# EXPLAIN (VERBOSE, COSTS OFF)
> SELECT p.a, q.a
> FROM t1 p
> JOIN t1 q ON p.a = q.a
> WHERE p.a = 18;
> QUERY PLAN
> -------------------------------
> Nested Loop
> Output: p.a, q.a
> -> Seq Scan on public.t1 p
> Output: p.a, p.b
> Filter: (p.a = 18)
> -> Seq Scan on public.t1 q
> Output: q.a, q.b
> Filter: (q.a = 18)
> (8 rows)
> to the self-join query prevents SJE, whereas removing that restriction
> allows SJE to occur.
> It appears that the equivalence class containing the constant absorbs
> the equality between p.a and q.a, so
> generate_join_implied_equalities() returns NIL and SJE gives up.
Yeah, I believe that explanation.
> Do you think this case is worth improving, perhaps as a separate patch?
Hm. Certainly the plan looks a little stupid. Now we know, in a
case like this, that each scan will produce just one row --- but
if they're implemented as seqscans not indexscans then they could
still be expensive. (On the third hand, there must be a unique index
on t1.a, or there would be no opportunity for SJE. So I guess your
example here is with toy-size tables that the planner doesn't think
justify an indexscan?)
Right offhand I'm thinking there is no meaningful performance win to
be had, so fixing this seems mostly cosmetic. If it's trivial to
do, or if you can refute my feeling that it's not a performance win,
then let's look closer.
regards, tom lane
diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index 7aad91fd34e..f903c48ef51 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -1287,26 +1287,94 @@ remove_self_join_rel(PlannerInfo *root,
Assert(found_relid);
/*
- * If the removed relation has a row mark, transfer it to the remaining
- * one.
+ * If the removed relation has a row mark, we must get rid of it, making
+ * sure that the kept relation has at least as strong a mark. (We assume
+ * it's okay to just adjust root->rowMarks; the planner won't consult the
+ * source list root->parse->rowMarks again.)
*
- * 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.
+ * remove_self_joins_one_group() already rejected cases in which one rel
+ * has a locking rowmark and the other doesn't. But here we must handle
+ * cases where two different locking strengths were given. It's okay to
+ * retain only the stronger lock strength, because the results will match
+ * what would have happened if we didn't do SJE and the scan requiring the
+ * stronger lock reached each tuple first. (We do not want to consider
+ * the possibility that the scan requiring the weaker lock reaches a tuple
+ * first, because implementing the query like that would risk lock-upgrade
+ * deadlock errors.) We keep the waitPolicy associated with that lock
+ * strength too, since if it is LockWaitError we should error out on
+ * non-lockable rows, while if it is LockWaitSkip then non-lockable rows
+ * would be dropped from the equijoin result anyway, and if it is
+ * LockWaitBlock then we will wait to acquire the row lock and then the
+ * second scan would have accepted the row as already locked.
+ *
+ * If we merge two rowmarks with equal strength, take the Max() of their
+ * waitPolicies, corresponding to what the parser's applyLockingClause()
+ * does and with the same reasoning.
+ *
+ * Unlike applyLockingClause(), we don't take the Max() of waitPolicies
+ * associated with different lock strengths. That's a bit debatable
+ * perhaps, but it is not our job here to make semantic policy decisions.
+ * We should just implement what the parser tells us, to the best of our
+ * ability.
+ *
+ * Note: this code is dependent on the merged rels not being foreign
+ * tables. If they were, we couldn't be sure to clean up what the FDW
+ * might have done in response to GetForeignRowMarkType calls; plus the
+ * markType might depend on factors we aren't accounting for.
+ *
+ * Nothing to do if there's no rmark; kmark, if any, is fine (it must be a
+ * non-locking mark).
*/
if (rmark)
{
+ /* Shouldn't have inheritance children yet */
+ Assert(rmark->rti == rmark->prti);
+ Assert(!rmark->isParent);
+
if (kmark)
{
- Assert(kmark->markType == rmark->markType);
+ Assert(kmark->rti == kmark->prti);
+ Assert(!kmark->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);
}
else
{
- /* Shouldn't have inheritance children yet. */
- Assert(rmark->rti == rmark->prti);
-
+ /*
+ * Just relabel rmark to attach it to the kept rel. The
+ * associated tlist row-identity Var's varno will get renumbered
+ * through normal processing, so we needn't touch it here.
+ */
rmark->rti = rmark->prti = toKeep->relid;
}
}
@@ -1827,10 +1895,15 @@ 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 one or both rels have rowmarks, we cannot remove the join
+ * unless the rowmarks have similar semantics. In particular, an
+ * RTE with a locking rowmark potentially represents a different
+ * set of row values than an RTE that doesn't have one.
+ *
+ * So first, search the rowMarks list for relevant PlanRowMarks.
+ * There should be no more than one for each RTI; we make an
+ * incomplete Assert check for that. kmark will be for the kept
+ * rel, rmark for the removed one.
*/
foreach(lc, root->rowMarks)
{
@@ -1850,8 +1923,25 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids)
if (kmark && rmark)
break;
}
- if (kmark && rmark && kmark->markType != rmark->markType)
- continue;
+ if (kmark && rmark)
+ {
+ /* Can't merge if only one mark implies row locking */
+ if (RowMarkRequiresRowShareLock(kmark->markType) !=
+ RowMarkRequiresRowShareLock(rmark->markType))
+ continue;
+ }
+ else if (kmark)
+ {
+ /* Can only merge if single mark is non-locking */
+ if (RowMarkRequiresRowShareLock(kmark->markType))
+ continue;
+ }
+ else if (rmark)
+ {
+ /* Can only merge if single mark is non-locking */
+ if (RowMarkRequiresRowShareLock(rmark->markType))
+ continue;
+ }
/*
* We only deal with base rels here, so their relids bitset
