[demo] Do not call lock_rec_convert_impl_to_expl if a table S-lock is…

[mariadb-YuchenPei]

… held

In lock_clust_rec_read_check_and_lock there's a test on lock_table_has(trx, index->table, LOCK_X) which if fails would result in a call to lock_rec_convert_impl_to_expl, which 1. has a comment "If an implicit x-lock exists on a record, convert it to an explicit one." that does not apply if a table S-lock is held and 2. does a few things such as looking up the trx_id of the record and find the trx in a global hash trx_sys. In this patch we do the same check for a table S-lock. This could potentially improve the performance of LOCK IN SHARE MODE (TODO: check)

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[mariadb-YuchenPei]

claude:

---

I'll review PR 5526. Let me fetch the PR and its diff.

I've reviewed the PR (draft/demo, single line added to lock_clust_rec_read_check_and_lock). Here's my assessment.

The change

 	if (lock_table_has(trx, index->table, LOCK_X)
+      || lock_table_has(trx, index->table, LOCK_S)
 	    || heap_no == PAGE_HEAP_NO_SUPREMUM) {
 	} else if (lock_rec_convert_impl_to_expl<true>(...) == trx
 	    && gap_mode == LOCK_REC_NOT_GAP) {

Correctness — I believe it's sound, and here's the justification worth putting in the code

The safety of skipping lock_rec_convert_impl_to_expl rests on one question: while we hold a table S-lock, can any other transaction hold an implicit X-lock on a record of this table?

No — and it's a clean argument from the table-lock compatibility matrix:

• To hold an implicit X-lock on a record, a transaction must have modified that record in its still-active transaction, which requires it to hold a table IX lock.
• A granted table S lock is incompatible with IX. So while our S-lock is granted and held, no other transaction can hold a granted IX-lock, hence none can be mid-modification, hence none can hold an implicit X-lock.
• The only implicit lock that could exist is our own, and that doesn't need converting for our own subsequent lock_rec_lock to be correct.

So there is nothing for lock_rec_convert_impl_to_expl to usefully convert when we hold table S — the same reasoning that already justifies the existing LOCK_X branch (an X table lock excludes every other table lock).

Strong supporting precedent: the sibling function lock_sec_rec_read_check_and_lock (lock0lock.cc:6544) already does exactly this — if (lock_table_has(trx, index->table, mode)) return DB_SUCCESS; — skipping its own impl-to-expl call when a covering table lock (including S) is held. Your patch just brings the clustered path in line with the secondary path. That's the single best argument that this is safe, and I'd cite it in the PR description.

One behavioral note: your form is slightly broader than the sec path. The sec path keys on mode (the requested read mode); yours skips whenever a table S-lock is held, regardless of read mode — so a FOR UPDATE record read (mode LOCK_X) that happens to hold a table S-lock (but not X) would also skip. It's still safe by the argument above, but if you want to mirror the existing code exactly and stay maximally conservative, use:

if (lock_table_has(trx, index->table, LOCK_X)
    || lock_table_has(trx, index->table, mode)   // mirrors lock_sec_rec_read_check_and_lock
    || heap_no == PAGE_HEAP_NO_SUPREMUM) {

For mode == LOCK_S that's identical to your patch; for mode == LOCK_X it collapses to the pre-existing X check (no new behavior). Easier to defend by direct analogy.

Performance — the win is real but conditional, and the benchmark has to target the right case

This is exactly the caveat from the earlier analysis, and it directly affects your "TODO: check":

• The patch removes lock_rec_convert_impl_to_expl, whose cost is dominated by the trx_sys.find() hash probe — but only when the record's DB_TRX_ID exceeds the reader's cached max_inactive_id. That's the case right after a bulk insert (all rows carry the same recent trx_id, max_inactive_id still low), which is the scenario in the demo.
• If instead the cheap trx_id <= max_inactive_id compare was firing, you're removing a compare-and-return and adding a second lock_table_has walk — so the net could be a wash or even slightly negative per row.

So to actually demonstrate the win, benchmark LOCK IN SHARE MODE over a freshly bulk-inserted table with a low max_inactive_id (few intervening transactions), and confirm with perf that trx_sys.find / lf_hash_search disappears from the share-mode profile. If you want I can sketch that microbenchmark.

Minor: for FOR UPDATE (holds table X) the first lock_table_has(LOCK_X) short-circuits, so the new S-check is never evaluated — no added cost there. Good.

Things to verify / nits

1. Scope is global, not full-scan-gated. This changes every clustered locking read that holds a table S-lock, not just the innodb_table_lock_on_full_scan path. I think that's fine (and desirable), but flag it as intentional in the PR — it's not scoped to MDEV-24813.
2. Run the locking/isolation suites before promoting from demo: innodb.lock_isolation, innodb.avoid_deadlock_with_blocked, innodb.lock_delete_updated, the PR3487 main.innodb_full_scan combinations, and anything exercising implicit-lock conversion. If the argument above is right, all should stay green; that's the empirical confirmation.
3. Add a one-line comment explaining why holding S is sufficient (the "S excludes IX ⇒ no foreign implicit X-lock" reasoning) — it's non-obvious to a future reader, whereas the LOCK_X case reads as self-evident.
4. Indentation: the added line uses spaces; surrounding InnoDB code uses tabs. Re-indent to match.

Net: as a demo the reasoning is correct and it has direct precedent in the secondary-index path; the open item is proving the perf benefit on a benchmark that hits the trx_sys.find escalation rather than the cheap-compare path.