MDEV-26123: Always mark spatial index reads as locking#5269
MDEV-26123: Always mark spatial index reads as locking#5269mariadb-TafzeelShams wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request simplifies the logic in ha_innobase::index_read to always mark spatial index reads as locking (will_lock = true) to address MDEV-26123, and adds a corresponding test case. Feedback points out that if a transaction is explicitly started as read-only, setting will_lock = true can lead to assertion failures or crashes. It is recommended to check if the transaction is read-only and return HA_ERR_READ_ONLY_TRANSACTION instead, along with adding a test case to verify this behavior.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (index->is_spatial()) { | ||
| m_prebuilt->trx->will_lock = true; | ||
| } |
There was a problem hiding this comment.
If the transaction is explicitly started as read-only (i.e., m_prebuilt->trx->read_only is true), setting will_lock = true and subsequently attempting to acquire predicate locks on the spatial index will lead to assertion failures (e.g., ut_ad(!trx->read_only) in lock_rec_lock_slow()) in debug builds, or undefined behavior/crashes in release builds.
We should check if m_prebuilt->trx->read_only is true and return HA_ERR_READ_ONLY_TRANSACTION in that case to safely prevent locking in read-only transactions.
if (index->is_spatial()) {
if (m_prebuilt->trx->read_only) {
DBUG_RETURN(HA_ERR_READ_ONLY_TRANSACTION);
}
m_prebuilt->trx->will_lock = true;
}There was a problem hiding this comment.
Below testcase works even without the patch.
Where trx->will_lock==true along with trx->read_only==true
Even though our transaction is explicitly set as read only, for spatial index we need to prevent parent path from other write transactions by acquiring locks.
SET SESSION TRANSACTION READ ONLY;
SELECT COUNT(*) FROM t1 WHERE category = (SELECT category FROM t1 WHERE ST_EQUALS(p, POINT(1,2)));
| SELECT COUNT(*) FROM t1 WHERE ST_Contains(t1.p, (SELECT p FROM t1 WHERE category = 10)); | ||
|
|
||
| DROP TABLE t1; |
There was a problem hiding this comment.
It would be beneficial to add a test case to verify that spatial index reads in an explicit READ ONLY transaction correctly return ER_READ_ONLY_TRANSACTION instead of causing any issues or assertions.
SELECT COUNT(*) FROM t1 WHERE ST_Contains(t1.p, (SELECT p FROM t1 WHERE category = 10));
--echo # Verify that explicit READ ONLY transaction returns ER_READ_ONLY_TRANSACTION
START TRANSACTION READ ONLY;
--error ER_READ_ONLY_TRANSACTION
SELECT COUNT(*) FROM t1 WHERE ST_Contains(t1.p, (SELECT p FROM t1 WHERE category = 10));
COMMIT;
DROP TABLE t1;
| SELECT COUNT(*) FROM t1 WHERE ST_Contains(t1.p, (SELECT p FROM t1 WHERE category = 10)); | ||
| COUNT(*) | ||
| 1 | ||
| DROP TABLE t1; |
There was a problem hiding this comment.
Update the result file to match the new test case for explicit READ ONLY transactions.
SELECT COUNT(*) FROM t1 WHERE ST_Contains(t1.p, (SELECT p FROM t1 WHERE category = 10));
COUNT(*)
1
# Verify that explicit READ ONLY transaction returns ER_READ_ONLY_TRANSACTION
START TRANSACTION READ ONLY;
SELECT COUNT(*) FROM t1 WHERE ST_Contains(t1.p, (SELECT p FROM t1 WHERE category = 10));
ERROR 1792 (25006): Cannot execute statement in a READ ONLY transaction
COMMIT;
DROP TABLE t1;
bb08748 to
d8d8f8a
Compare
A SELECT query could fail with ER_READ_ONLY_TRANSACTION when search on spatial index is done after search on non-spatial index. R-tree index search always require page locks. Previously, trx->will_lock was only set for spatial indexes when the transaction had not yet been started. If a transaction was already active due to a prior non-spatial index access, index_read() for spatial index returned HA_ERR_READ_ONLY_TRANSACTION instead of marking the transaction as locking. Fix: - ha_innobase::index_read() : Set trx->will_lock unconditionally for spatial index, ensuring that R-tree searches can acquire the required page locks regardless of transaction state.
d8d8f8a to
0b6edbe
Compare
Description
MDEV-26123: Always mark spatial index reads as locking
A SELECT query could fail with ER_READ_ONLY_TRANSACTION when search
on spatial index is done after search on non-spatial index.
R-tree index search always require page locks. Previously,
trx->will_lock was only set for spatial indexes when the transaction
had not yet been started. If a transaction was already active due to
a prior non-spatial index access, index_read() for spatial index
returned HA_ERR_READ_ONLY_TRANSACTION instead of marking the transaction
as locking.
Fix:
spatial index, ensuring that R-tree searches can acquire the required
page locks regardless of transaction state.
Release Notes
Always mark Spatial Index reads as locking
How can this PR be tested?
Added testcase inside
mysql-test/suite/innodb_gis/t/rtree_search.testto cover the changes.Basing the PR against the correct MariaDB version
mainbranch.PR quality check