Thanks for the review. On Mon, Jan 16, 2017 at 4:13 PM, Jan Lindström <jan.lindstrom@mariadb.com> wrote:
Hi,
On Mon, Jan 16, 2017 at 2:10 PM, <marko.makela@mariadb.com> wrote:
diff --git a/storage/innobase/btr/btr0btr.cc b/storage/innobase/btr/btr0btr.cc index d1d9dfe64fe..1fb5cb06949 100644 --- a/storage/innobase/btr/btr0btr.cc +++ b/storage/innobase/btr/btr0btr.cc @@ -215,10 +215,6 @@ btr_root_get( buf_block_t* root = btr_root_block_get(index, RW_SX_LATCH, mtr);
- if (root && root->page.encrypted == true) { - root = NULL; - } - return(root ? buf_block_get_frame(root) : NULL);
This is unrelated to MySQL encryption, it is related to MariaDB encryption, not sure if btr_root_block_get would always return NULL when page is encrypted, upper code is peppered with assertions, thus this might not be safe.
Good catch. I was at one point assuming that the buf_page_t::encrypted had been added to MySQL 5.7. I will of course revert this and look for other references to the buf_page_t fields that I was going to remove. MySQL 5.7 is doing a little better when it comes to the encryption and compression fields: they are not put into buf_page_t (where they only matter for the short time when flushing is in progress), but they are also not put into the IORequest either. I think that we should at some point refactor the IORequest into something that contains all the necessary information.
- if (ptype == FIL_PAGE_PAGE_COMPRESSED_ENCRYPTED) { - header_len += FIL_PAGE_COMPRESSION_METHOD_SIZE; - } - - /* Do not try to uncompressed pages that are not compressed */ - if (ptype != FIL_PAGE_PAGE_COMPRESSED && - ptype != FIL_PAGE_PAGE_COMPRESSED_ENCRYPTED && - ptype != FIL_PAGE_COMPRESSED) { + switch (ptype) { + case FIL_PAGE_PAGE_COMPRESSED_ENCRYPTED: + header_len = FIL_PAGE_DATA + FIL_PAGE_COMPRESSED_SIZE + + FIL_PAGE_COMPRESSION_METHOD_SIZE; + break; + case FIL_PAGE_PAGE_COMPRESSED: + header_len = FIL_PAGE_DATA + FIL_PAGE_COMPRESSED_SIZE; + break; + default: + /* The page is not in our format. */ return;
This is also unrelated change but more like cosmetic, so ok.
Right, this is only minor refactoring. I prefer to use compile-time constants and to merge the conditions.
diff --git a/storage/innobase/include/buf0buf.ic b/storage/innobase/include/buf0buf.ic index f2b1b151598..5e75c446bbd 100644 --- a/storage/innobase/include/buf0buf.ic +++ b/storage/innobase/include/buf0buf.ic @@ -734,9 +734,6 @@ buf_block_get_frame( case BUF_BLOCK_ZIP_PAGE: case BUF_BLOCK_ZIP_DIRTY: case BUF_BLOCK_NOT_USED: - if (block->page.encrypted) { - goto ok; - }
Hmm, this is unrelated but could be actually correct.
Actually, I think it is correct (even though I said above that I may have accidentally removed some references to some buf_page_t:: fields that I intended to remove). ROW_FORMAT=COMPRESSED (which is what the BUF_BLOCK_ZIP_* refer to) should not allow any encryption. So, it would also an error if the page.encrypted flag set.
ut_error; break;
@@ -145,52 +137,22 @@ fil_page_type_validate( page_type == FIL_PAGE_TYPE_BLOB || page_type == FIL_PAGE_TYPE_ZBLOB || page_type == FIL_PAGE_TYPE_ZBLOB2 || - page_type == FIL_PAGE_COMPRESSED || - page_type == FIL_PAGE_TYPE_UNKNOWN || - page_type == FIL_PAGE_ENCRYPTED || - page_type == FIL_PAGE_COMPRESSED_AND_ENCRYPTED || - page_type == FIL_PAGE_ENCRYPTED_RTREE))) { - - uint key_version = mach_read_from_4(page + FIL_PAGE_FILE_FLUSH_LSN_OR_KEY_VERSION); - bool page_compressed = (page_type == FIL_PAGE_PAGE_COMPRESSED); - bool page_compressed_encrypted = (page_type == FIL_PAGE_PAGE_COMPRESSED_ENCRYPTED); + page_type == FIL_PAGE_TYPE_UNKNOWN))) {
Why you change this code, it is not MySQL compression code ? See below (after quoting all the changed code):
+ ulint space = mach_read_from_4(page + FIL_PAGE_ARCH_LOG_NO_OR_SPACE_ID); ulint offset = mach_read_from_4(page + FIL_PAGE_OFFSET); - ib_uint64_t lsn = mach_read_from_8(page + FIL_PAGE_LSN); - ulint compressed_len = mach_read_from_2(page + FIL_PAGE_DATA); fil_system_enter(); fil_space_t* rspace = fil_space_get_by_id(space); fil_system_exit();
/* Dump out the page info */ - fprintf(stderr, "InnoDB: Space %lu offset %lu name %s page_type %lu page_type_name %s\n" - "InnoDB: key_version %u page_compressed %d page_compressed_encrypted %d lsn %llu compressed_len %lu\n", - space, offset, rspace->name, page_type, fil_get_page_type_name(page_type), - key_version, page_compressed, page_compressed_encrypted, (ulonglong)lsn, compressed_len); - fflush(stderr); - - ut_ad(page_type == FIL_PAGE_PAGE_COMPRESSED || - page_type == FIL_PAGE_PAGE_COMPRESSED_ENCRYPTED || - page_type == FIL_PAGE_INDEX || - page_type == FIL_PAGE_RTREE || - page_type == FIL_PAGE_UNDO_LOG || - page_type == FIL_PAGE_INODE || - page_type == FIL_PAGE_IBUF_FREE_LIST || - page_type == FIL_PAGE_TYPE_ALLOCATED || - page_type == FIL_PAGE_IBUF_BITMAP || - page_type == FIL_PAGE_TYPE_SYS || - page_type == FIL_PAGE_TYPE_TRX_SYS || - page_type == FIL_PAGE_TYPE_FSP_HDR || - page_type == FIL_PAGE_TYPE_XDES || - page_type == FIL_PAGE_TYPE_BLOB || - page_type == FIL_PAGE_TYPE_ZBLOB || - page_type == FIL_PAGE_TYPE_ZBLOB2 || - page_type == FIL_PAGE_COMPRESSED || - page_type == FIL_PAGE_TYPE_UNKNOWN || - page_type == FIL_PAGE_ENCRYPTED || - page_type == FIL_PAGE_COMPRESSED_AND_ENCRYPTED || - page_type == FIL_PAGE_ENCRYPTED_RTREE); - + ib::fatal() << "Page " << space << ":" << offset + << " name " << (rspace ? rspace->name : "???") + << " page_type " << page_type + << " key_version " + << mach_read_from_4(page + FIL_PAGE_FILE_FLUSH_LSN_OR_KEY_VERSION) + << " lsn " << mach_read_from_8(page + FIL_PAGE_LSN) + << " compressed_len " << mach_read_from_2(page + FIL_PAGE_DATA); return false; }
The FIL_PAGE_TYPE_COMPRESSED (13) was never written anywhere in 10.1. In 10.2 it was somewhat dubiously-looking merged with the Oracle MySQL 5.7 FIL_PAGE_COMPRESSED (14). But as neither value is ever written by MariaDB to any page, it was dead code, and no harm done. I commented out all the #define for the Oracle MySQL 5.7 page types. Only the FIL_PAGE_PAGE_* are MariaDB types. The ut_ad() condition is always false. It is clearer to write ib::fatal().
ok to push, rest of the changes.
Is the last change OK with the above explanation? I will commit a revised patch tomorrow. Marko -- DON’T MISS M|17 April 11 - 12, 2017 The Conrad Hotel New York City https://m17.mariadb.com/ Marko Mäkelä, Lead Developer InnoDB MariaDB Corporation