MDEV-35394 Innochecksum misinterprets freed undo pages #3636
Conversation
|
|
Thirunarayanan
force-pushed
the
10.6-MDEV-35394
branch
2 times, most recently
from
29e04c6
to
d93ade0
Compare
Thirunarayanan
force-pushed
the
10.6-MDEV-35394
branch
3 times, most recently
from
532eed6
to
e95959e
Compare
- Innochecksum misinterprets the freed pages as active one. This leads the user to think there are too many valid pages exist. - To avoid this confusion, innochecksum introduced one more option --skip-freed-pages and -r to avoid the freed pages while dumping or printing the summary of the tablespace. - Innochecksum can safely assume the page is freed if the respective extent doesn't belong to a segment and marked as freed in XDES_BITMAP in extent descriptor page. - Innochecksum shouldn't assume that zero-filled page as extent descriptor page.
Thirunarayanan
force-pushed
the
10.6-MDEV-35394
branch
from
e95959e
to
e94fe20
Compare
| const char *argument MY_ATTRIBUTE((unused)), | ||
| const char *argument, |
There was a problem hiding this comment.
I suggested to drop also the argument if it is actually unused. That’s the C++98 way of indicating unused arguments. However, I see that the argument is used #ifndef DBUG_OFF. A better way would be to write the following to avoid a warning in non-debug builds:
const char *IF_DBUG(argument,),Curiously, when I was searching for warnings in non-debug builds, I found some for other InnoDB files, but not for this one.
| init_page_size(buf); | ||
|
|
||
| extent_size = FSP_EXTENT_SIZE; |
There was a problem hiding this comment.
This is the only caller of init_page_size(), but I think that it would be more future-proof to initialize the extent_size there.
| let $resultlog=$MYSQLTEST_VARDIR/tmp/result.log; | ||
| exec $INNOCHECKSUM -S -r $MYSQLD_DATADIR/ibdata1 > $resultlog; | ||
|
|
||
| let SEARCH_FILE = $MYSQLTEST_VARDIR/tmp/result.log; | ||
| let SEARCH_PATTERN= Undo page state: 0 active, 0 cached, 0 to_purge, 0 prepared, 0 other; |
There was a problem hiding this comment.
Can we add one more test that uses a nonzero -s option? I might expect that such a test would report some undo pages, because it would not have access to the allocation metadata in the first page.
| /* Enabled for log write option. */ | ||
| static bool is_log_enabled = false; | ||
|
|
||
| /* Skip freed pages */ | ||
| static bool skip_freed_pages; |
There was a problem hiding this comment.
The comment before the variable declaration is not adding any value, and it is not in Doxygen format either.
In my previous review, I quoted the redundant initialization of is_log_enabled with the thought that you’d remove all redundant zero-initialization of any global variables in this compilation unit.
I also quoted some code in innochecksum_get_one_option() that seemed to be redundant. You addressed that comment by not adding further redundant code there, but you did not remove other redundant code:
case 'n':
no_check = true;
break;
case 'a':
case 'S':
break;
case 'w':
do_write = true;
break;
case 'l':
is_log_enabled = true;
break;Note: the following is not redundant in the same way, because the parameter that is bound to the D option is &page_dump_filename. For now, I am not suggesting to remove the variable page_type_dump, although I would assume that it is redundant too: page_type_dump == !!page_dump_filename.
case 'D':
page_type_dump = true;
break;
Description
Innochecksum misinterprets the freed undo log pages as active one. This leads the user to think there are too many undo log pages exist.
Innochecksum can safely assume the undo log page is freed if the respective extent doesn't belong to a segment and marked as freed in XDES_BITMAP in extent descriptor page.
Innochecksum shouldn't assume that zero-filled page as extent descriptor page.
How can this PR be tested?
./mtr innodb.innochecksum_undo_page
Basing the PR against the correct MariaDB version
mainbranch.PR quality check