MDEV-36832 UPDATE...IN(SELECT) acquires spurious gap locks on FK index

[arcivanov]

UPDATE ticket SET is_valid=0 WHERE ticket_id IN (SELECT ticket_id FROM ticket WHERE booking_id=1) acquires S next-key and gap locks on the FK secondary index fk_ticket_2_booking, causing deadlocks between concurrent transactions operating on non-overlapping booking_id values. The expected behavior — matching UPDATE ... WHERE ticket_id IN (1,2) (explicit PK list) — is to only lock the PRIMARY key records being updated.

Root cause: the optimizer converts the IN(SELECT) to a semi-join, which creates two table references for the same table. The SQL layer then processes the statement as SQLCOM_UPDATE_MULTI. In ha_innobase::store_lock(), the read-only (subquery) table handle receives TL_READ, but the existing LOCK_NONE optimization for DML read tables was gated on isolation_level <= READ_COMMITTED and did not include SQLCOM_UPDATE_MULTI at all. In REPEATABLE READ (the default), the subquery's secondary index scan therefore acquired LOCK_S next-key locks, including gap locks spanning into adjacent key ranges.

Fix: add SQLCOM_UPDATE_MULTI to the LOCK_NONE (consistent read) condition in store_lock():

1. For TL_READ (binlog off or ROW format): use consistent read at all isolation levels. This is safe because:
   • The MVCC snapshot is stable within a transaction in REPEATABLE READ
   • Rows being modified still acquire X locks via the write-side table handle (F_WRLCK / LOCK_X)
   • SERIALIZABLE is handled by ::external_lock() upgrading LOCK_NONE back to LOCK_S
2. For TL_READ_NO_INSERT (statement-based binlog): extend the existing READ COMMITTED optimization to also cover SQLCOM_UPDATE_MULTI

The fix is deliberately limited to SQLCOM_UPDATE_MULTI (not SQLCOM_UPDATE) because single-table UPDATE with scalar subqueries on other tables traditionally uses S locks to block concurrent reads of the subquery table, and broadening the change there would alter observable behavior for existing applications.

Lock count before/after (semi-join UPDATE on booking_id=1):

• Before: 5 lock structs, 5 row locks (IS + S×3 on FK index + IX + X×2 on PK)
• After: 2 lock structs, 2 row locks (IX + X×2 on PK) — identical to explicit WHERE ticket_id IN (1,2)

[arcivanov]

@dr-m

[grooverdan]

@arcivanov , while waiting on a review from the the InnoDB team on this, do you have the time/interest in looking at MDEV-38817? I'm suspecting the test failure is actually the correct result, and should have been always occurring since 10.6 when MDEV-13115 was implemented.

[arcivanov]

@arcivanov , while waiting on a review from the the InnoDB team on this, do you have the time/interest in looking at MDEV-38817? I'm suspecting the test failure is actually the correct result, and should have been always occurring since 10.6 when MDEV-13115 was implemented.

I was unable to reproduce it locally but the early send_eof seems to not be appropriate for non-read-only transactions due to cleanup occurring after the packet is sent resulting in a potential race. Please see #4684.

[gkodinov]

This is preliminary review. Thank you for your contribution. Please stand by for the final review.

[dr-m]

As far as I can tell, this business of downgrading the requested lock type is a hack that should have been replaced years ago. A better solution would be to introduce a lock_type value TL_READ_NO_LOCK, which would be set at the appropriate places in the SQL query execution, and have InnoDB never downgrade any TL_READ or TL_READ_NO_INSERT.

My concern is that it could be unsafe to downgrade some locks in some fragments of a complex SQL statement. But there is only a single thd_sql_command(thd) value returned for the entire statement.

[arcivanov]

So what would be a satisfactory outcome for you as far as implementation/testing/validation etc that would allow this fix to move forward (the overall historic state of this code notwithstanding)? While I understand the frustration with code quality your comment does not seem actionable 🥲

[dr-m]

The actionable part of my comment was that instead of revising the complex conditions for downgrading the requested lock, such as choosing

			m_prebuilt->select_lock_type = LOCK_NONE;
			m_prebuilt->stored_select_lock_type = LOCK_NONE;

over

			m_prebuilt->select_lock_type = LOCK_S;
			m_prebuilt->stored_select_lock_type = LOCK_S;

for specific values of thd_sql_command(thd), a new lock_type=TL_READ_NO_LOCK needs to be introduced that would be requested when appropriate. At the same time, most or all of the sql_command checks should be removed from ha_innobase::store_lock().

I understand that implementing this is far from trivial. But I do not think that extending the current hack is in any way sustainable or acceptable.

[arcivanov]

for specific values of thd_sql_command(thd), a new lock_type=TL_READ_NO_LOCK needs to be introduced that would be requested when appropriate. At the same time, most or all of the sql_command checks should be removed from ha_innobase::store_lock().\n\nI understand that implementing this is far from trivial. But I do not think that extending the current hack is in any way sustainable or acceptable.

Fair enough, this is actionable. I have a feeling though that this type of a change wouldn't be allowed into the stable and wait time on fixing this in cloud prod would be in years (since it'll go into 13.1+). I understand this is a hack but it's a hack pattern that is already successfully employed in stable and I'm merely extending it.

That said I'm willing to take a stab at the proper implementation as well.

[dr-m]

If @vuvova thinks that your original change is OK, then we could perhaps go with it. However, I think that in that case we would have to fix the MDEV-37365 family of MDL bugs first, because currently our ability to run stress tests on FOREIGN KEY is being seriously hurt by the insufficient meta-data locking during DDL operations.

It would be really great if you could work on the proper implementation, targeting the main branch, in a separate pull request.

[vuvova]

• The change kind of makes sense to me. Old code handled and old comment explicitly mentioned "UPDATE ... = (SELECT ...)" case. It is difficult to explain to the user why sometimes there's an S lock and sometimes there is none (depending on how optimizer decided to execute the subquery)
• the new comment shouldn't mention a subquery, the fix has nothing to do with subqueries, mutli-update updates some tables and only read others — these others don't need an S lock. It doesn't matter whether multi-update used a join or the join was implicitly created by the optimizer from a subquery. Also, don't mention MDEV in the comment
• what about multi-delete? does it have the same issue?

[arcivanov]

Multi-delete in this scenario won't work as delete takes write locks on PK and FK no matter what.

[arcivanov]

Comment updated per your specification.

[vuvova]

why do you need that? the change below should be enough

[arcivanov]

Because, as the comment above states, the one below is only for READ_COMMITTED, and we need to cover REPEATABLE_READ (without binlog statement replication) as well:

|| (trx->isolation_level <= TRX_ISO_READ_COMMITTED
...
&& (
...
|| sql_command == SQLCOM_UPDATE_MULTI
)

[vuvova]

• The change kind of makes sense to me. Old code handled and old comment explicitly mentioned "UPDATE ... = (SELECT ...)" case. It is difficult to explain to the user why sometimes there's an S lock and sometimes there is none (depending on how optimizer decided to execute the subquery)
• the new comment shouldn't mention a subquery, the fix has nothing to do with subqueries, mutli-update updates some tables and only read others — these others don't need an S lock. It doesn't matter whether multi-update used a join or the join was implicitly created by the optimizer from a subquery. Also, don't mention MDEV in the comment
• what about multi-delete? does it have the same issue?

[gkodinov]

Sorry for the delay on our end, @arcivanov. @vuvova confirmed that he is aware that he needs to keep reviewing this. But he's currently a bit busy with other work. He also wanted me to set expectations a bit by providing some general timelines (based on https://mariadb.com/docs/release-notes/community-server/about/release-model):

According to our development cycle we work on bugs In the following periods 15.03 - 30.04, 15.06 - 30.07, 15.12 - 31.01 and 15.09 - 30.10. So, please, expect to get a review somewhere between these two dates and the goal is to have your PR merged before the second date.
Note that these dates are not set in stone and might fluctuate a bit, given the priorities and load of the individual developers. I've confirmed with Serg that we're in one such fluctuation currently.

Long story short: you should hopefully have your reply in a couple of days or so.

I will monitor the situation and keep everybody updated in the meanwhile.

[arcivanov]

Pushed an update fixing two issues I found while re-auditing this change.

1. SERIALIZABLE was weakened for autocommit statements.

The previous revision granted LOCK_NONE for the read-side tables of SQLCOM_UPDATE_MULTI at all isolation levels, relying on ::external_lock() to upgrade LOCK_NONE back to LOCK_S at SERIALIZABLE. That upgrade, however, is gated on OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN — its autocommit exemption assumes a read-only transaction, which a multi-table UPDATE is not. As a result, an autocommit UPDATE ... IN (SELECT ...) at SERIALIZABLE read its subquery via a consistent read with no S locks at all (confirmed with innodb_status_output_locks=ON: the trx held only IX + PK X locks and had an open read view; the same statement inside BEGIN held FK-index S next-key locks as expected).

Fixed by excluding SERIALIZABLE in the store_lock() condition itself (trx->isolation_level != TRX_ISO_SERIALIZABLE), so this path never hands out LOCK_NONE at SERIALIZABLE and does not depend on the later upgrade. For explicit transactions the resulting locks are identical to before (S locks either way); only the autocommit case changes, restoring the pre-change SERIALIZABLE behavior.

Added Test 7 to update_subquery_fk_gap_lock_isolation.test covering exactly this: an autocommit UPDATE...IN(SELECT) at SERIALIZABLE is stalled mid-scan on a PK X lock, and a probe INSERT whose FK index entry falls inside the already-scanned range must time out. The test fails on the previous revision and passes on this one. Two details in the test are load-bearing: the table is recreated rather than emptied with DELETE (delete-marked records turn the clustered index scan's eq_ref locks into next-key locks and the probe INSERT then queues behind the UPDATE's waiting next-key lock on the PK, masking what the test measures), and the probe row's FK entry must land inside the scanned range, since a mid-scan-stalled UPDATE has not yet locked the gap past its last scanned entry.

2. Restored the original mdev-14846 deadlock coverage.

That test guards the fix for evaluate_join_record() ignoring a deadlock-rollback error (asserting on trx->state). My previous revision rewrote it to a no-deadlock scenario, which silently dropped coverage of that error path. The test and result files are now restored verbatim; instead, its .opt file adds --log-bin --binlog-format=statement, under which the read-side tables receive TL_READ_NO_INSERT and deliberately keep their S locks (statement-based replication requires locking reads), so the original deadlock and its ER_LOCK_DEADLOCK expectation still hold unchanged.

Verified: both new tests, mdev-14846, innodb_lock_wait_timeout_1, lock_isolation, deadlock_detect (both combinations) and innodb-lock all pass.