maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #10291
Re: [Commits] 43e2a78f2a0: Remove MYSQL_COMPRESSION.
Thanks for the review.
On Mon, Jan 16, 2017 at 4:13 PM, Jan Lindström
<jan.lindstrom@xxxxxxxxxxx> wrote:
> Hi,
>
> On Mon, Jan 16, 2017 at 2:10 PM, <marko.makela@xxxxxxxxxxx> 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
References
