Jan Lindström (Inactive) added a comment - 2017-08-14 04:31 Is that ENCRYPTED=YES necessary for crash ?

Elena Stepanova added a comment - 2017-08-14 09:39 It appears that the table must be encrypted in order to hit the failure, at least I never got the failure otherwise. Whether it's encrypted because it has ENCRYPTED=YES or because the tables are set to be encrypted by default, wasn't important in the test initially, but on some reason the latest version of the test failed for me with the explicit clause and didn't fail otherwise, so I kept the clause.

Jan Lindström (Inactive) added a comment - 2017-09-19 10:23 https://github.com/MariaDB/server/commit/26301165ffa30cee5d237560d961834736d9b9ed

Marko Mäkelä added a comment - 2017-09-19 11:49 I think that this needs another round of review. There are not many encryption tests for row_format=compressed. Perhaps there should be a test combination for encryption that would be used in some innodb_zip tests.

Jan Lindström (Inactive) added a comment - 2017-09-20 08:23 Following existing test also test row_format=compressed: innodb-compressed-blob innodb-first-page-read innodb-force-corrupt innodb-key-rotation-disable innodb_onlinealter_encryption innodb_page_encryption_key_change innodb-page_encryption_log_encryption innodb-page_encryption innodb-redo-badkey innodb-redo-nokeys Using some of innodb_zip tests is possible but they seem to create quite a few pages i.e. would need to wait until key rotation really has time to encrypt something before table is gone.

Marko Mäkelä added a comment - 2017-09-20 09:26 At the very least, I would like to see a test that reproduces the problem when the fix is not present.

Jan Lindström (Inactive) added a comment - 2017-09-20 10:57 https://github.com/MariaDB/server/commit/9b998c18283c8acd4ed3d8e6df503fe861af04f0

Marko Mäkelä added a comment - 2017-10-06 08:50 I do not understand what the fix is supposed to achieve, apart from skipping the comparison that is failing. A design constraint of ROW_FORMAT=COMPRESSED which I designed and implemented in the InnoDB Plugin for MySQL 5.1 is that the compressed and uncompressed index page headers must match at all times. That is, if one of the headers is updated, then the other one must be update as well, while holding X-latch or SX-latch on block->lock. Once the latch is released, the two copies must be equivalent. Something is apparently breaking this constraint, and we must find and fix the cause of this. I do not think that the check should be relaxed without fully understanding the problem. I cannot repeat any problem with the included test case (the commit features an slightly extended version of t3.test ), either in 10.1 or 10.2. I did verify that page_zip_decompress() is being successfully called after restart and that the data file is encrypted. If I could repeat the problem, I could better suggest how to fix this.

Marko Mäkelä added a comment - 2017-10-06 09:41 If the mismatch is caused by encryption updating only one of block->frame or block->page.zip.data for the buf_block_t* block of a ROW_FORMAT=COMPRESSED table, then the root cause should be fixed by updating both copies at the same time. Note that during buffer pool flushing, both the uncompressed and compressed page must exist in the buffer pool. This is because flushing is protected by buf_block_t::lock. At other times, the page might exist in compressed format only. If it contains changes that must be written to the *.ibd file, then the buf_page_t::state will be BUF_BLOCK_ZIP_DIRTY.

Marko Mäkelä added a comment - 2017-10-06 14:48 I found the problem by running the test on perro. On two subsequent runs, the problem occurred on page 4:10, giving some hope of determinism. So, I decided to run it inside --manual-gdb. Until the shutdown, there was no problem. Before the restart, I set a breakpoint: break buf_page_get_gen cond 1 space==4&&offset==10 run finish disable display/x $1->page[24]@14 display/x $1->page.zip.data[24]@14 At this point, I noticed that both the uncompressed frame and the compressed page contain non-zero bytes. The mismatch that I had observed in the core dumps was that the block->page.zip.data contained zeroes while block->frame contained nonzero bytes. So, I started to suspect that the problem is that block->page.zip.data is zeroed out, and not that block->frame is being written with nonzero bytes without updating block->page.zip.data accordingly. I set a watchpoint: p (unsigned*)&$1->page.zip.data[28] watch *$2 continue This watchpoint was triggered in a page flush: #1 0x000055555616dda7 in buf_flush_update_zip_checksum ( page=0x7fffed4f8000 "\f%", zip_size=8192, lsn=2195866) at /home/mariadb/git/10.1-mdev13512/storage/xtradb/buf/buf0flu.cc:760 #2 0x000055555616df0c in buf_flush_init_for_writing ( page=0x7fffed4f4000 "\f%", page_zip_=0x7fffecfbfaf8, newest_lsn=2195866) at /home/mariadb/git/10.1-mdev13512/storage/xtradb/buf/buf0flu.cc:804 #3 0x000055555616e3e3 in buf_flush_write_block_low (bpage=0x7fffecfbfae8, flush_type=BUF_FLUSH_LIST, sync=false) at /home/mariadb/git/10.1-mdev13512/storage/xtradb/buf/buf0flu.cc:950 #4 0x000055555616ea0e in buf_flush_page (buf_pool=0x7ffff0fb0478, bpage=0x7fffecfbfae8, flush_type=BUF_FLUSH_LIST, sync=false) at /home/mariadb/git/10.1-mdev13512/storage/xtradb/buf/buf0flu.cc:1130 #5 0x000055555616efe8 in buf_flush_try_neighbors (space=4, offset=10, flush_type=BUF_FLUSH_LIST, n_flushed=1, n_to_flush=100) at /home/mariadb/git/10.1-mdev13512/storage/xtradb/buf/buf0flu.cc:1344 #6 0x000055555616f308 in buf_flush_page_and_try_neighbors ( bpage=0x7fffecfbfae8, flush_type=BUF_FLUSH_LIST, n_to_flush=100, count=0x7fffe33feae8) at /home/mariadb/git/10.1-mdev13512/storage/xtradb/buf/buf0flu.cc:1433 #7 0x000055555616fe48 in buf_do_flush_list_batch (buf_pool=0x7ffff0fb0478, min_n=100, lsn_limit=18446744073709551615) buf_flush_update_zip_checksum() zeroes out the FIL_PAGE_FILE_FLUSH_LSN_OR_KEY_VERSION field, which it perhaps should not do? ib_uint32_t checksum = static_cast <ib_uint32_t>( page_zip_calc_checksum( page, zip_size, static_cast <srv_checksum_algorithm_t>( srv_checksum_algorithm)));   mach_write_to_8(page + FIL_PAGE_LSN, lsn); memset (page + FIL_PAGE_FILE_FLUSH_LSN_OR_KEY_VERSION, 0, 8); mach_write_to_4(page + FIL_PAGE_SPACE_OR_CHKSUM, checksum); Note that the page here is the compressed page frame (in this case, zip_size is half the srv_page_size aka innodb_page_size). Without encryption, this field would always be zero in both the compressed and uncompressed page header. Perhaps we should remove this memset() altogether. The field is excluded from the checksum calculation. This memset() became redundant with my MySQL Bug #53306 fix which added memset() to fsp_init_file_page_low() . So, it is safe to remove the memset(). It looks like the memset() must be breaking SPATIAL INDEX in 10.2 for ROW_FORMAT=COMPRESSED, by zeroing out the SSN (split sequence number) field. In MySQL 5.7.5, two memset() were removed from buf0flu.cc in WL#6968 InnoDB GIS: R-tree index support . The proper fix is to remove the 2 offending memset() from both InnoDB and XtraDB, rather than to shoot or cripple the messenger (the consistency check). diff --git a/storage/innobase/buf/buf0flu.cc b/storage/innobase/buf/buf0flu.cc index 31feb322826..dacddfca385 100644 --- a/storage/innobase/buf/buf0flu.cc +++ b/storage/innobase/buf/buf0flu.cc @@ -715,7 +715,6 @@ buf_flush_update_zip_checksum( srv_checksum_algorithm))); mach_write_to_8(page + FIL_PAGE_LSN, lsn); - memset(page + FIL_PAGE_FILE_FLUSH_LSN_OR_KEY_VERSION, 0, 8); mach_write_to_4(page + FIL_PAGE_SPACE_OR_CHKSUM, checksum); } @@ -894,8 +893,6 @@ buf_flush_write_block_low( bpage->newest_modification); ut_a(page_zip_verify_checksum(frame, zip_size)); - - memset(frame + FIL_PAGE_FILE_FLUSH_LSN_OR_KEY_VERSION, 0, 8); break; case BUF_BLOCK_FILE_PAGE: frame = bpage->zip.data; I believe that the reason why I was unable to repeat the bug on my laptop is that there was not enough concurrency. On perro, there are more processor cores and more true concurrency between the page flush and the SQL execution thread. I tried to prove my claim that SPATIAL INDEX is broken on ROW_FORMAT=COMPRESSED tables, but we do not have the larger --suite=innodb_gis tests from 5.7 (until MDEV-13626 addresses that), and a modification of innodb.instant_alter in 10.3 to use ROW_FORMAT=COMPRESSED did not trigger any assertion failure. I suppose that the SSN only matters in certain SPATIAL INDEX operations.

Marko Mäkelä added a comment - 2017-10-06 15:01 I removed the offending memset() .