MDEV-16065 Assertion failed in btr_pcur_restore_position_func on UPDATE

btr_pcur_store_position(): Assert that the 'default row' record never
is the only record in a page. (If that would happen, an empty
root page would be re-created in the non-instant format, not containing
the special record.) When the cursor is positioned on the page infimum,
never use the 'default row' as the BTR_PCUR_BEFORE reference.
(This is additional cleanup, not fixing the bug.)

rec_copy_prefix_to_buf(): When converting a record prefix to
the non-instant-add format, copy the original number of null flags.
Rename the variable instant_len to instant_omit, and introduce a
few more variables to make the code easiser to read.

Note: In purge, rec_copy_prefix_to_buf() is also used for storing the
persistent cursor position on a 'default row' record. The stored record
reference will be garbage, but row_search_on_row_ref() will do special
handling to reposition the cursor on the 'default row', based on
ref->info_bits.

innodb.dml_purge: Also cover the 'default row'.

mysql-test/suite/innodb/r/dml_purge.result

@@ -11,6 +11,7 @@ connect  prevent_purge,localhost,root;
 START TRANSACTION WITH CONSISTENT SNAPSHOT;
 connection default;
 INSERT INTO t1 VALUES(1,2),(3,4);
+ALTER TABLE t1 ADD COLUMN c INT;
 UPDATE t1 SET b=-3 WHERE a=3;
 connect  con1,localhost,root;
 BEGIN;
@@ -21,8 +22,13 @@ InnoDB		0 transactions not purged
 disconnect con1;
 FLUSH TABLE t1 FOR EXPORT;
 Clustered index root page contents:
-N_RECS=2; LEVEL=0
-header=0x010000030087 (a=0x696e66696d756d00)
+N_RECS=3; LEVEL=0
+header=0x0100000300c6 (a=0x696e66696d756d00)
+header=0x1000200b0087 (a=0x80000000,
+ DB_TRX_ID=0x000000000000,
+ DB_ROLL_PTR=0x80000000000000,
+ b=0x80000000,
+ c=NULL(4 bytes))
 header=0x0000100900a6 (a=0x80000001,
  DB_TRX_ID=0x000000000000,
  DB_ROLL_PTR=0x80000000000000,
@@ -31,11 +37,11 @@ header=0x000018090074 (a=0x80000003,
  DB_TRX_ID=0x000000000000,
  DB_ROLL_PTR=0x80000000000000,
  b=0x7ffffffd)
-header=0x030008030000 (a=0x73757072656d756d00)
+header=0x040008030000 (a=0x73757072656d756d00)
 UNLOCK TABLES;
 SELECT * FROM t1;
-a	b
-1	2
-3	-3
+a	b	c
+1	2	NULL
+3	-3	NULL
 DROP TABLE t1;
 SET GLOBAL innodb_purge_rseg_truncate_frequency = @saved_frequency;

mysql-test/suite/innodb/r/instant_alter.result

@@ -440,6 +440,12 @@ SELECT * FROM t1;
 a	b
 a	1
 DROP TABLE t1;
+CREATE TABLE t1 (a INT, b VARCHAR(8), PRIMARY KEY(b,a)) ENGINE=InnoDB ROW_FORMAT=REDUNDANT;
+INSERT INTO t1 VALUES (1,'foo');
+ALTER TABLE t1 ADD COLUMN c INT;
+UPDATE t1 SET c = 1;
+UPDATE t1 SET c = 2;
+DROP TABLE t1;
 CREATE TABLE t1
 (id INT PRIMARY KEY, c2 INT UNIQUE,
 c3 POINT NOT NULL DEFAULT ST_GeomFromText('POINT(3 4)'),
@@ -826,6 +832,12 @@ SELECT * FROM t1;
 a	b
 a	1
 DROP TABLE t1;
+CREATE TABLE t1 (a INT, b VARCHAR(8), PRIMARY KEY(b,a)) ENGINE=InnoDB ROW_FORMAT=COMPACT;
+INSERT INTO t1 VALUES (1,'foo');
+ALTER TABLE t1 ADD COLUMN c INT;
+UPDATE t1 SET c = 1;
+UPDATE t1 SET c = 2;
+DROP TABLE t1;
 CREATE TABLE t1
 (id INT PRIMARY KEY, c2 INT UNIQUE,
 c3 POINT NOT NULL DEFAULT ST_GeomFromText('POINT(3 4)'),
@@ -1212,10 +1224,16 @@ SELECT * FROM t1;
 a	b
 a	1
 DROP TABLE t1;
+CREATE TABLE t1 (a INT, b VARCHAR(8), PRIMARY KEY(b,a)) ENGINE=InnoDB ROW_FORMAT=DYNAMIC;
+INSERT INTO t1 VALUES (1,'foo');
+ALTER TABLE t1 ADD COLUMN c INT;
+UPDATE t1 SET c = 1;
+UPDATE t1 SET c = 2;
+DROP TABLE t1;
 disconnect analyze;
 SELECT variable_value-@old_instant instants
 FROM information_schema.global_status
 WHERE variable_name = 'innodb_instant_alter_column';
 instants
-36
+39
 SET GLOBAL innodb_purge_rseg_truncate_frequency= @saved_frequency;

mysql-test/suite/innodb/r/instant_alter_debug.result

@@ -35,7 +35,7 @@ ALTER TABLE t4 ADD COLUMN b INT;
 SELECT COUNT(*) FROM INFORMATION_SCHEMA.COLUMNS
 LEFT JOIN t4 ON (NUMERIC_SCALE = pk);
 COUNT(*)
-1748
+953
 SET DEBUG_SYNC='innodb_inplace_alter_table_enter SIGNAL enter WAIT_FOR delete';
 ALTER TABLE t4 ADD COLUMN c INT;
 connect  dml,localhost,root,,;

mysql-test/suite/innodb/t/dml_purge.test

@@ -20,6 +20,7 @@ START TRANSACTION WITH CONSISTENT SNAPSHOT;
 
 --connection default
 INSERT INTO t1 VALUES(1,2),(3,4);
+ALTER TABLE t1 ADD COLUMN c INT;
 UPDATE t1 SET b=-3 WHERE a=3;
 
 --connect (con1,localhost,root)
@@ -47,7 +48,7 @@ sysseek(FILE, 3*$ps, 0) || die "Unable to seek $file";
 die "Unable to read $file" unless sysread(FILE, $page, $ps) == $ps;
 print "N_RECS=", unpack("n", substr($page,38+16,2));
 print "; LEVEL=", unpack("n", substr($page,38+26,2)), "\n";
-my @fields=("a","DB_TRX_ID","DB_ROLL_PTR", "b");
+my @fields=qw(a DB_TRX_ID DB_ROLL_PTR b c);
 for (my $offset= 0x65; $offset;
      $offset= unpack("n", substr($page,$offset-2,2)))
 {

mysql-test/suite/innodb/t/instant_alter.test

@@ -311,6 +311,14 @@ INSERT INTO t1 SET a='a';
 SELECT * FROM t1;
 DROP TABLE t1;
 
+# MDEV-16065 Assertion failed in btr_pcur_restore_position_func on UPDATE
+eval CREATE TABLE t1 (a INT, b VARCHAR(8), PRIMARY KEY(b,a)) $engine;
+INSERT INTO t1 VALUES (1,'foo');
+ALTER TABLE t1 ADD COLUMN c INT;
+UPDATE t1 SET c = 1;
+UPDATE t1 SET c = 2;
+DROP TABLE t1;
+
 dec $format;
 }
 disconnect analyze;

storage/innobase/btr/btr0pcur.cc

@@ -127,6 +127,8 @@ btr_pcur_store_position(
 			  mtr, dict_index_get_lock(index),
 			  MTR_MEMO_X_LOCK | MTR_MEMO_SX_LOCK)));
 
+	cursor->old_stored = true;
+
 	if (page_is_empty(page)) {
 		/* It must be an empty index tree; NOTE that in this case
 		we do not store the modify_clock, but always do a search
@@ -136,10 +138,7 @@ btr_pcur_store_position(
 		ut_ad(page_is_leaf(page));
 		ut_ad(page_get_page_no(page) == index->page);
 
-		cursor->old_stored = true;
-
 		if (page_rec_is_supremum_low(offs)) {
-
 			cursor->rel_pos = BTR_PCUR_AFTER_LAST_IN_TREE;
 		} else {
 			cursor->rel_pos = BTR_PCUR_BEFORE_FIRST_IN_TREE;
@@ -149,21 +148,25 @@ btr_pcur_store_position(
 	}
 
 	if (page_rec_is_supremum_low(offs)) {
-
 		rec = page_rec_get_prev(rec);
 
-		cursor->rel_pos = BTR_PCUR_AFTER;
+		ut_ad(!page_rec_is_infimum(rec));
+		ut_ad(!rec_is_default_row(rec, index));
 
+		cursor->rel_pos = BTR_PCUR_AFTER;
 	} else if (page_rec_is_infimum_low(offs)) {
+		rec = page_rec_get_next(rec);
 
+		if (rec_is_default_row(rec, index)) {
 			rec = page_rec_get_next(rec);
+			ut_ad(!page_rec_is_supremum(rec));
+		}
 
 		cursor->rel_pos = BTR_PCUR_BEFORE;
 	} else {
 		cursor->rel_pos = BTR_PCUR_ON;
 	}
 
-	cursor->old_stored = true;
 	cursor->old_rec = dict_index_copy_rec_order_prefix(
 		index, rec, &cursor->old_n_fields,
 		&cursor->old_rec_buf, &cursor->buf_size);

storage/innobase/rem/rem0rec.cc

@@ -1835,11 +1835,6 @@ rec_copy_prefix_to_buf(
 						or NULL */
 	ulint*			buf_size)	/*!< in/out: buffer size */
 {
-	const byte*	nulls;
-	const byte*	lens;
-	ulint		prefix_len	= 0;
-	ulint		instant_len	= 0;
-
 	ut_ad(n_fields <= index->n_fields || dict_index_is_ibuf(index));
 	ut_ad(index->n_core_null_bytes <= UT_BITS_IN_BYTES(index->n_nullable));
 	UNIV_PREFETCH_RW(*buf);
@@ -1852,6 +1847,12 @@ rec_copy_prefix_to_buf(
 			       buf, buf_size));
 	}
 
+	ulint		prefix_len	= 0;
+	ulint		instant_omit	= 0;
+	const byte*	nulls		= rec - (REC_N_NEW_EXTRA_BYTES + 1);
+	const byte*	nullf		= nulls;
+	const byte*	lens		= nulls - index->n_core_null_bytes;
+
 	switch (rec_get_status(rec)) {
 	default:
 		/* infimum or supremum record: no sense to copy anything */
@@ -1859,12 +1860,8 @@ rec_copy_prefix_to_buf(
 		return(NULL);
 	case REC_STATUS_ORDINARY:
 		ut_ad(n_fields <= index->n_core_fields);
-		nulls = rec - (REC_N_NEW_EXTRA_BYTES + 1);
-		lens = nulls - index->n_core_null_bytes;
 		break;
 	case REC_STATUS_NODE_PTR:
-		nulls = rec - (REC_N_NEW_EXTRA_BYTES + 1);
-		lens = nulls - index->n_core_null_bytes;
 		/* For R-tree, we need to copy the child page number field. */
 		compile_time_assert(DICT_INDEX_SPATIAL_NODEPTR_SIZE == 1);
 		if (dict_index_is_spatial(index)) {
@@ -1890,15 +1887,18 @@ rec_copy_prefix_to_buf(
 		/* We would have !index->is_instant() when rolling back
 		an instant ADD COLUMN operation. */
 		ut_ad(index->is_instant() || page_rec_is_default_row(rec));
-		nulls = &rec[-REC_N_NEW_EXTRA_BYTES];
+		nulls++;
 		const ulint n_rec = ulint(index->n_core_fields) + 1
 			+ rec_get_n_add_field(nulls);
-		instant_len = ulint(&rec[-REC_N_NEW_EXTRA_BYTES] - nulls);
-		ut_ad(instant_len == 1 || instant_len == 2);
-		const uint n_nullable = index->get_n_nullable(n_rec);
-		lens = --nulls - UT_BITS_IN_BYTES(n_nullable);
+		instant_omit = ulint(&rec[-REC_N_NEW_EXTRA_BYTES] - nulls);
+		ut_ad(instant_omit == 1 || instant_omit == 2);
+		nullf = nulls;
+		const uint nb = UT_BITS_IN_BYTES(index->get_n_nullable(n_rec));
+		instant_omit += nb - index->n_core_null_bytes;
+		lens = --nulls - nb;
 	}
 
+	const byte* const lenf = lens;
 	UNIV_PREFETCH_R(lens);
 
 	/* read the lengths of fields 0..n */
@@ -1950,27 +1950,39 @@ rec_copy_prefix_to_buf(
 
 	UNIV_PREFETCH_R(rec + prefix_len);
 
-	prefix_len += ulint(rec - (lens + 1)) - instant_len;
+	ulint size = prefix_len + ulint(rec - (lens + 1)) - instant_omit;
 
-	if ((*buf == NULL) || (*buf_size < prefix_len)) {
+	if (*buf == NULL || *buf_size < size) {
 		ut_free(*buf);
-		*buf_size = prefix_len;
-		*buf = static_cast<byte*>(ut_malloc_nokey(prefix_len));
-	}
-
-	if (instant_len) {
-		ulint hdr = ulint(&rec[-REC_N_NEW_EXTRA_BYTES] - (lens + 1))
-			- instant_len;
-		memcpy(*buf, lens + 1, hdr);
-		memcpy(*buf + hdr, &rec[-REC_N_NEW_EXTRA_BYTES],
-		       prefix_len - hdr);
-		ut_ad(rec_get_status(*buf + hdr + REC_N_NEW_EXTRA_BYTES)
-		      == REC_STATUS_COLUMNS_ADDED);
-		rec_set_status(*buf + hdr + REC_N_NEW_EXTRA_BYTES,
-			       REC_STATUS_ORDINARY);
-		return *buf + hdr + REC_N_NEW_EXTRA_BYTES;
+		*buf_size = size;
+		*buf = static_cast<byte*>(ut_malloc_nokey(size));
+	}
+
+	if (instant_omit) {
+		/* Copy and convert the record header to a format where
+		instant ADD COLUMN has not been used:
+		+ lengths of variable-length fields in the prefix
+		- omit any null flag bytes for any instantly added columns
+		+ index->n_core_null_bytes of null flags
+		- omit the n_add_fields header (1 or 2 bytes)
+		+ REC_N_NEW_EXTRA_BYTES of fixed header */
+		byte* b = *buf;
+		/* copy the lengths of the variable-length fields */
+		memcpy(b, lens + 1, ulint(lenf - lens));
+		b += ulint(lenf - lens);
+		/* copy the null flags */
+		memcpy(b, nullf - index->n_core_null_bytes,
+		       index->n_core_null_bytes);
+		b += index->n_core_null_bytes + REC_N_NEW_EXTRA_BYTES;
+		ut_ad(ulint(b - *buf) + prefix_len == size);
+		/* copy the fixed-size header and the record prefix */
+		memcpy(b - REC_N_NEW_EXTRA_BYTES, rec - REC_N_NEW_EXTRA_BYTES,
+		       prefix_len + REC_N_NEW_EXTRA_BYTES);
+		ut_ad(rec_get_status(b) == REC_STATUS_COLUMNS_ADDED);
+		rec_set_status(b, REC_STATUS_ORDINARY);
+		return b;
 	} else {
-		memcpy(*buf, lens + 1, prefix_len);
+		memcpy(*buf, lens + 1, size);
 		return *buf + (rec - (lens + 1));
 	}
 }