Stale PRIMARY KEY values in a SPATIAL INDEX built by ALTER TABLE when the clustered index page is evicted during a log checkpoint

Claude AI generated description:

When ALTER TABLE builds a SPATIAL INDEX (ADD SPATIAL INDEX, or a table rebuild of a table that has one), row_merge_read_clustered_index() caches the spatial index entries built from one clustered index page in spatial_index_info and inserts them when it reaches the end of the page. The rows are built with ROW_COPY_POINTERS, so the PRIMARY KEY fields of the cached entries point into the clustered index page.

spatial_index_info::insert() commits the scan mini-transaction when a log checkpoint is needed in the middle of a batch:

if (log_sys.check_for_checkpoint()) {

	if (mtr_started) {

		...

		btr_pcur_store_position(pcur, scan_mtr);

		scan_mtr->commit();

		mtr_started = false;

	}

	log_free_check();

}

It then continues to insert the remaining cached entries, whose PRIMARY KEY fields still point into the now unlatched and unfixed clustered index page. If the page is evicted and the block is reused while the batch is being inserted, the R-tree records get garbage PRIMARY KEY values. In a debug build this fails an assertion; in a release build the SPATIAL INDEX silently gets entries that point to the wrong row or to no row, and the affected rows cannot be found through the SPATIAL INDEX.

The comment in row_merge_read_clustered_index() already describes the requirement ("We have to do insert the cached spatial index rows, since after the mtr_commit, the cluster index page could be updated, then the data in cached rows become invalid."), but the commit inside spatial_index_info::insert() does not honour it. spatial_index_info::add() only copies fields that point to ext.

Backtrace of the test case below on an unpatched 11.4.12 debug build:

mariadbd: storage/innobase/include/page0cur.inl:183: rec_t* page_cur_tuple_insert(page_cur_t*, const dtuple_t*, rec_offs**, mem_heap_t**, ulint, mtr_t*): Assertion `!rec || !cmp_dtuple_rec(tuple, rec, cursor->index, *offsets)' failed.

include/page0cur.inl:184(page_cur_tuple_insert(page_cur_t*, dtuple_t const*, unsigned short**, mem_block_info_t**, unsigned long, mtr_t*))

btr/btr0cur.cc:2490(btr_cur_optimistic_insert(...))

row/row0merge.cc:172(spatial_index_info::insert(unsigned long, btr_pcur_t*, bool&, mem_block_info_t*, mtr_t*))

row/row0merge.cc:1770(row_merge_spatial_rows(...))

row/row0merge.cc:2165(row_merge_read_clustered_index(...))

handler/handler0alter.cc:8968(ha_innobase::inplace_alter_table(TABLE*, Alter_inplace_info*))

sql/handler.h:4958(handler::ha_inplace_alter_table(TABLE*, Alter_inplace_info*))

sql/sql_table.cc:8127(mysql_inplace_alter_table(...))

The assertion fails because the record that was just inserted differs from the tuple: the tuple's PRIMARY KEY field points into a block that has meanwhile been reused (possibly for the very R-tree page that is being modified).

MySQL

MySQL copied these fields before committing the scan mini-transaction already before 8.0.17 ("Since the data of the tuple pk fields are pointers of cluster rows. After mtr committed, these pointer could be point to invalid data. Then, we need to copy all these data from cluster rows."), but only for the first spatial index. Bug #28758961 "ASSERT PAGE0CUR.IC:NNN:!REC || !CMP_DTUPLE_REC(TUPLE, REC, INDEX, *OFFSETS)" (MySQL 8.0.17, commit 6e9dcbf8560) extended the copy to all spatial indexes. Since MySQL 8.0.27 (WL#14283), RTree_inserter::deep_copy_tuples() in ddl0rtree.cc does the same. MariaDB never had this copy.

How to repeat

The test uses the existing DBUG injection point row_merge_instrument_log_check_flush to request a checkpoint, and the minimal buffer pool so that the clustered index page gets evicted while the batch is being inserted. Without the fix, the ALTER TABLE fails the assertion above. With innodb_buffer_pool_size=512M, the test passes also without the fix.

Whether the evicted block is reused before the batch has been inserted depends on timing, so the test is not fully deterministic without the fix. Without the fix it failed every time on 11.4.12 (already with 100,000 rows). On 10.11 it failed in 0 of 3 runs with 150,000 rows, 3 of 3 with 200,000 rows and 2 of 3 with 250,000 rows. With the fix it always passes. It takes about 36 seconds in a debug build, so it is a big test.

rtree_create_evict.opt:

--innodb-buffer-pool-size=6m

rtree_create_evict.test:

--source include/have_innodb.inc

--source include/have_innodb_16k.inc

--source include/have_debug.inc

--source include/have_sequence.inc

--source include/not_embedded.inc

--source include/big_test.inc

CREATE TABLE t1 (id INT PRIMARY KEY, g POINT NOT NULL) ENGINE=InnoDB;

INSERT INTO t1 SELECT seq,

  POINT(FLOOR(RAND(seq) * 1000000), FLOOR(RAND(seq + 7777777) * 1000000))

FROM seq_1_to_200000;

SET @save_dbug=@@SESSION.debug_dbug;

SET debug_dbug='+d,row_merge_instrument_log_check_flush';

ALTER TABLE t1 ADD SPATIAL INDEX(g);

SET debug_dbug=@save_dbug;

CHECK TABLE t1;

DROP TABLE t1;

Suggested fix

Copy all fields after the MBR when caching a tuple, so that the cached tuples never point to the clustered index page. This also covers the existing case of fields that point to ext. Unlike MySQL, which copies only before committing the scan mini-transaction, this copies every tuple, at the cost of some memory and copying per batch.

--- a/storage/innobase/row/row0merge.cc

+++ b/storage/innobase/row/row0merge.cc

@@ -80,18 +80,13 @@ class spatial_index_info {

     dtuple_t *dtuple= row_build_index_entry(row, ext, index, heap);

     ut_ad(dtuple);

     ut_ad(dtuple->n_fields == index->n_fields);

-    if (ext)

-    {

-      /* Replace any references to ext, because ext will be allocated

-      from row_heap. */

-      for (ulint i= 1; i < dtuple->n_fields; i++)

-      {

-        dfield_t &dfield= dtuple->fields[i];

-        if (dfield.data >= ext->buf &&

-            dfield.data <= &ext->buf[ext->n_ext * ext->max_len])

-          dfield_dup(&dfield, heap);

-      }

-    }

+    /* Copy the fields after the MBR, because they may point to the

+    clustered index page (or to ext, which is allocated from row_heap).

+    insert() may commit the scan mini-transaction before all cached

+    tuples have been inserted, and the page may then be evicted or

+    modified. */

+    for (ulint i= 1; i < dtuple->n_fields; i++)

+      dfield_dup(&dtuple->fields[i], heap);

     m_dtuple_vec.push_back(dtuple);

   }

With this patch:

• the test above passes on debug builds of 10.11 and 11.4.12;
• the whole innodb_gis suite passes on both, including --big-test;
• a larger variant (300,000 random points, innodb_buffer_pool_size=6M, forced checkpoint) followed by an R-tree lookup of every row by its own point and PRIMARY KEY finds all rows.

Workaround

None that is reliable. A large buffer pool relative to the table makes the eviction unlikely. Affected SPATIAL INDEXes can be detected by comparing COUNT(DISTINCT pk) of an index scan (FORCE INDEX on the SPATIAL INDEX with an MBR covering all data) with COUNT(*), and repaired by dropping and re-adding the SPATIAL INDEX.