MDEV-38817 innodb.skip_locked_nowait fails: SELECT NOWAIT gets ER_LOCK_WAIT_TIMEOUT

[arcivanov]

Don't send OK packet early for locking reads (SELECT ... FOR UPDATE, SELECT ... LOCK IN SHARE MODE).

MDEV-38019 added an optimization in select_send::send_eof() that sends the OK packet to the client before the server finishes cleanup, allowing the client to process results sooner. The guard
!thd->transaction->stmt.is_trx_read_write() was intended to exclude write transactions, but mark_trx_read_write() is only called for DML (INSERT/UPDATE/DELETE), not for locking reads. This caused SELECT ... FOR UPDATE with autocommit to send OK before external_lock(F_UNLCK) committed and released locks, creating a race where another connection using NOWAIT could see still-held locks and immediately fail with ER_LOCK_WAIT_TIMEOUT.

Add a check for select_lock == st_select_lex::NONE to ensure the early OK path is only taken for truly non-locking SELECT statements.

[arcivanov]

@grooverdan

[svoj]

creating a race where another connection using NOWAIT could see still-held locks and
immediately fail with ER_LOCK_WAIT_TIMEOUT.

This sounds like a valid behaviour. Besides there're a lot more statements that'd most probably fail similarly, e.g. TRUNCATE TABLE ... NOWAIT after regular SELECT in another connection.

My thinking is MDEV-38019 invalidated many tests and it needs some systematic approach towards getting it fixed.

Not requesting any changes, just sharing a point to think about.

[gkodinov]

This is preliminary review. Thanks for your contribution. LGTM. Please wait for the final review.

[arcivanov]

This sounds like a valid behaviour.

I have no dog in this fight (I was just asked to look at it, it's not even my bug) but to me this sounds like a violation of happens-before ACID guarantees: if the state confirmation packet arrives on the client then the happens-before relationship between the DML and the reported state of that DML has to be consistent. The arrival of the packet to the client establishes that the operation is complete. The fact that there is a background cleanup that may have side-effects on the operation that has been reported to have been completed in any other concurrent system would be interpreted as a major bug.

Note that happens-before guarantee is only implied within the same connection/transaction that first DMLed and then SELECT NOWAIT, and not within other connections that issued SELECT NOWAIT "too early" according to the some shared wall clock.