Bug #60049 Clarify the comments in row_ins_must_modify

Description:
This is forked from Bug #59622.

The function row_rename_table_for_mysql() leaves un-purgeable garbage records in DICT_TABLE_IDS_ID, the unique index on SYS_TABLES.ID. This could affect all updates of unique secondary indexes.

How to repeat:
Create a database from the scratch.

CREATE TABLE t(a INT)ENGINE=InnoDB;
RENAME TABLE t TO u;
DROP TABLE u;
SET GLOBAL innodb_fast_shutdown=0;
-- shut down (ctrl-\ or SIGQUIT)

dd ibs=16384 skip=9 count=1 if=ibdata1 of=page.bin

See that the record chain on the page is as follows:

infimum, SYS_FOREIGN, SYS_FOREIGN_COLS, test/t, test/u, supremum

The delete-marked test/t and test/u should not be linked there. The next-record pointer of SYS_FOREIGN_COLS (at byte offset 158) should point to the supremum (0x0074) instead of the delete-marked test/t (0x00c0).

Suggested fix:
=== modified file 'storage/innobase/row/row0ins.c'
--- storage/innobase/row/row0ins.c	revid:marko.makela@oracle.com-20110208121614-jk53j7052eb7t9rf
+++ storage/innobase/row/row0ins.c	2011-02-09 12:26:05 +0000
@@ -1935,14 +1935,13 @@ record so that the intended insert of th
 the existing record. In the case of a clustered index, the prefix must be
 n_unique fields long, and in the case of a secondary index, all fields must be
 equal.
-@return 0 if no update, ROW_INS_PREV if previous should be updated;
-currently we do the search so that only the low_match record can match
-enough to the search tuple, not the next record */
+@return 0 if no update; ROW_INS_PREV if previous should be updated,
+ROW_INS_NEXT if next should be updated */
 UNIV_INLINE
 ulint
 row_ins_must_modify(
 /*================*/
-	btr_cur_t*	cursor)	/*!< in: B-tree cursor */
+	btr_cur_t*	cursor)	/*!< in/out: B-tree cursor */
 {
 	ulint	enough_match;
 	rec_t*	rec;
@@ -1966,6 +1965,16 @@ row_ins_must_modify(
 		}
 	}
 
+	if (cursor->up_match >= enough_match) {
+
+		rec = page_rec_get_next(btr_cur_get_rec(cursor));
+
+		if (!page_rec_is_supremum(rec)) {
+
+			return(ROW_INS_NEXT);
+		}
+	}
+
 	return(0);
 }

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/130887

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/130888

I was uncertain if the shutdown_server 10 in my test case works as I intended. Andrei Elkin enlightened me that the directive is implemented in client/mysqltest.cc, do_shutdown_server(). It does ensure that the server is killed, but it can fail (kill -KILL the server) if the specified timeout is exceeded. Because other tests in mysql-test seem to be using 10 as the timeout value, I guess that it is safe enough. If it starts failing nondeterministically on a busy server, the timeout can be increased.

Heikki says that there is a general rule not to update secondary index records in place, other than clearing or setting the delete-mark flag.

This is a special case where we would be updating a non-indexed column in a unique secondary index. There could be some record locking considerations.

There is a small possibility that there is a bug in the purge that only affects system tables. I will check if this bug is repeatable for user tables.

The issue does not seem to affect user tables. I just tested with this in the built-in InnoDB of MySQL 5.1:

CREATE TABLE t(a INT PRIMARY KEY,b INT NOT NULL UNIQUE)ENGINE=InnoDB;
INSERT INTO t VALUES(1,2);
UPDATE t SET a=2;
COMMIT;
SET GLOBAL innodb_fast_shutdown=0;

-- shutdown, and inspect the file t.ibd

The unique secondary index page contained infimum, supremum, (b=2,a=1) [deleted], (b=2,a=2). They were linked like this: infimum, (b=2,a=2), supremum. The record (b=2,a=1) was in the free list. That is, the record (1,2) that was delete-marked by the UPDATE was purged.

To be on the safe side, we should change the updates of non-key columns of unique secondary index records to be in place, but instead find out why the purge is not processing the records in DICT_TABLE_IDS_ID a.k.a. ID_IND on SYS_TABLES(ID). This is the only unique index in the system tables.

Sorry, there is no bug after all. It looks like I did not wait long enough for the purge to run.

I set a breakpoint on row_purge_remove_sec_if_poss(), and it succeeded for both test/t and test/u. The test case in my commit does pass even without the patch. If I change the parameter innodb_fast_shutdown=0 to 1 or 2, the test will fail.

Anyway, I would like to fix the comments in row_ins_must_modify() and remove the unused constant ROW_INS_NEXT and the dead code that checks for it. That would be a non-functional change. The test case could be used for regression testing (in case bugs appear in the purge system).

Fixed in 5.6.2