Ingo, Thanks for patiently explaining these things to me. I learned a
lot. Some comments inline. Based on your reply I approve this patch,
pending the fix of warnings. I also ask you to consider the following
suggestions:
1. As discussed, change (or if you prefer, create a bug report for
changing) the naming of backup_myisam tests and explain the
distinctions in header comments of each test. Move the test case
that does not need debug_sync to your new "normal" test. (See
comments below)
2. Consider whether one might as well flush all restored tables, not
just myisam tables, since that would simplify the code added to
kernel.cc. (See discussion below)
--
Øystein
Ingo Strüwing wrote:
> Hi Øystein,
>
> thanks for the review. Please see below.
>
> Øystein Grøvlen, 18.12.2008 12:20:
>> Thanks, for the patch, Ingo. I have a few comments and questions that
>> I want your response to before I finalize the review:
>>
>> 1. The new tests fail on Solaris x86 because a lot of warnings like
>> this:
>>
>> +Warning 1292 Truncated incorrect DOUBLE value: 'A'
>>
>> I think this is because your select query compares a varchar column
>> to an integer (see [1], [3] inline.)
>
> After applying the patch to a current tree, I saw these warnings on
> Linux too. Fixed.
Great.
>
>> 2. If a understand the patch correctly, it is the change to kernel.cc
>> that fixes the problem. The changes to mf_keycache.c is to prevent
>> future crashes with similar bugs?
>
> Yes, but the order of fixes were reverse. The initial behavior of the
> bug was a crash in keycache code. I fixed this first. Only then I found
> the kernel.cc problem.
>
>> 3. Are there any recommended policy for splitting test cases between
>> different test files? Why should not your test case go into
>> existing an existing backup test that tests myisam related stuff?
>> I am also unsure about the naming, and think I would like something
>> more informative that just a number to distinguish between
>> different backup/myisam tests.
>
> I don't know of a policy for this. In contrast, I have seen different
> opinions about test case splitting. One voice did even suggest to have
> one test case per bug fix. ;-)
>
> My first attempt was to add the new tests to one of the other
> backup_myisam tests. But then I found that they run with different
setups.
>
> backup_myisam1 has an option file to enable --external-locking. This has
> a high potential to fail on Windows. So we better keep this test case as
> short as possible.
>
> backup_myisam2 requires have_debug_sync. This test case will not
> normally run on a production server. So it should get only those tests
> that require synchronization.
>
> So I made a new test case with no special setup, which can be used for
> all MyISAM specific tests that don't require any.
>
> How would you rename that test case?
> backup.backup_myisam_with_external_locking.test,
> backup.backup_myisam_with_debug_sync.test,
> backup.backup_myisam_normal.test, and
> backup.backup_myisam_normal_coverage.test?
Very good reasons indeed. I think your proposed names would be good,
and some comments at the start of the test describing what
distinguishes this test from the other MyISAM tests would also be
good. I also note that the test case for Bug#38045 that has been
added at the end of the backup_myisam2 test, does not require
debug_sync. If you think this is too much changes that are irrelevant
for this bug, feel free to open a new bug instead.
>
>> 4. The backup_myisam3_coverage test is just the same as the
>> backup_myisam3 test, but with error injection. It does not seem to
>> me that it attempts to test the actions performed upon error.
>
> What error do you mean here?
I meant the injected error.
>
>> If I
>> understand correctly, in case of the injected error, the server
>> will flush all tables. However, there are only one table around,
>> so the test performs the same actions as without error injection.
>
> Coverage testing has a different goal. Its intention is to have every
> line of new or changed code executed. Without error injection, I was not
> able to let the execution run through the inner close_cached_tables().
> Indeed, in this test case it performs exactly the same operation as the
> outer close_cached_tables() does without error injection.
>
> But this is an excellent example to show, what coverage testing can do
> and cannot do.
>
> Though I run through each line of code now, I still don't test more
> cases than without coverage testing. The effort seems wasted.
>
> However, before I added the coverage test, the error handling within the
> loop looked different. Basically I copied the loop from
> Backup_restore_ctx::lock_tables_for_restore() and modified it slightly.
> The error handling was "return
> fatal_error(log_error(ER_OUT_OF_RESOURCES));". I prepended
> close_cached_tables() and all looked fine. Since the inner code was
> never executed, all looked great, all tests passed.
>
> Now, when I added the coverage test, I got problems because of the error
> reporting and later due to the missing call to close_thread_tables().
>
> Now the code became more simple and more correct. I do not report an
> error, but it is not necessary because calling close_cached_tables()
> with a list of tables is just an optimization. With a NULL list, all
> tables are closed, which is inefficient, but still correct. If later
> parts of code execution run into insufficient memory, they will
> (hopefully) report an error, if they don't have a way around it.
>
> In summary, though coverage testing catches a small class of problems
> only, it is still very useful to discover problems that otherwise might
> go undetected for a long time. These are mostly cases that will happen
> very seldom, but anyway it's better to fix them.
Good points about the purpose and limitations of coverage testing. I
guess we just have to keep in mind that 100% code coverage does not
imply 100% test coverage. My only concern is that adding such
"light-weight" code coverage tests, means that code coverage tools
will be of less value when working to improve test coverage. It is
often easier to detect that something is not handled at all, than to
find areas that need improvements.
>
>> 5. Which leads me to my next question: What would the disadvantages be
>> of simplifying this fix to always flush all tables after restore?
>
> The disadvantage would be that all tables that are open in the whole
> server would be closed, and have to be re-opened if they are used next
> time. On a big, busy server, this can make a remarkable burst of system
> activity and degrade performance until the table cache is filled with a
> sufficient number of open TABLE objects for all frequently used tables
> again.
>
Good point.
> Flushing just the tables, that we did freshly create during restore, is
> much less of a threat. We should be the only one to have them open
> anyway.
Some maybe the code could be simplified to flushing all tables, not
just myisam tables?
>
>> 6. If I understand correctly, it is not possible to add to do the
>> close_cached_tables call from the driver code. Is this because it
>> should only be called within the server? It seems to me that it
>> would be "more right" to handle such driver specific issues on the
>> driver level. Also, the driver already has a list of the tables it
>> is restoring. It seems a bit of a waste to use the catalog to
>> build another list when one such already exists. Could an
>> alternative be to iterate over drivers, instead of snapshots here,
>> and do this on a per driver basis?
>
> I made some attempts to request table refresh from the driver. This gave
> me nothing but crashes. It may be possible, but I don't know, how.
>
> It is an interesting idea, to use the driver's table list. But it will
> probably take me some time to locate it.
>
> OTOH it doesn't look like the most optimal modularization to me, if the
> backup driver, which belongs to the storage engine, manipulates the
> server's table cache.
>
> What we typically do in such cases is to report from the engine, if some
> special treatment is required. So it would be great, if during driver
> open, it could tell the backup kernel, that it needs a flush after
restore.
>
> I have read a comment that the kernel takes a copy of the table list for
> later reference, but I didn't find it. And I didn't find an appropriate
> place for it, if it is not there, in spite of the comment.
>
> And last not least, I expect that other engines might have similar
> requirements, once they have native drivers. Some common code doesn't
> look that bad then.
I see your points. I am not sure what is the best solution. I am not
quite comfortable with driver specific handling in the kernel, but as
you explain there are issues with other alternatives, too. I have
Cc-ed Rafal. He probably has some opinions on this.
>
> ...
>>> +--echo #
>>> +--echo # Inject error key_cache_read_block_error
>>> +--echo #
>>> +SET debug='d,key_cache_read_block_error';
>>> +--replace_regex /'.*\//'/
>> [4] Is this replace really necessary? Would not the path be the same
>> here for all users and platforms?
>
> Probably. I deleted it.
>
> ...
>>> +--echo #
>>> +--echo # Inject error key_cache_insert_block_error
>>> +--echo #
>>> +SET debug='d,key_cache_insert_block_error';
>> [5] Will this replace the previous debug flag, or is both set after
>> this?
>
> It will replace the list of keywords. To add one, one would write
> SET debug='+d,key_cache_insert_block_error';
>
>> ...
>>
>>> === modified file 'mysys/mf_keycache.c'
>>> --- a/mysys/mf_keycache.c 2008-10-20 09:16:47 +0000
>>> +++ b/mysys/mf_keycache.c 2008-12-03 16:02:14 +0000
>>> @@ -1373,7 +1373,11 @@ static void unreg_request(KEY_CACHE *key
>>> DBUG_ASSERT(block->prev_changed && *block->prev_changed ==
block);
>>> DBUG_ASSERT(!block->next_used);
>>> DBUG_ASSERT(!block->prev_used);
>>> - if (! --block->requests)
>>> + /*
>>> + Unregister the request, but do not link erroneous blocks into the
>>> + LRU ring.
>>> + */
>>> + if (!--block->requests && !(block->status &
BLOCK_ERROR))
>> [6] Did you consider making sure that unreg_request is not called for
>> error blocks instead of handling it here?
>
> It is called from free_block(), where it can be an error block. And it
> does --block->requests unconditionally. It would be ugly to count block
> requests in free-block() directly, just to avoid a call to
> unreg_request(). OTOH, it might be slightly more performant. If you
> insist...
Good points. No, I do not insist.
>
>>
>>> {
>>> my_bool hot;
>>> if (block->hits_left)
>>> @@ -1455,8 +1459,7 @@ static void wait_for_readers(KEY_CACHE *
>>> #ifdef THREAD
>>> struct st_my_thread_var *thread= my_thread_var;
>>> DBUG_ASSERT(block->status & (BLOCK_READ | BLOCK_IN_USE));
>>> - DBUG_ASSERT(!(block->status & (BLOCK_ERROR | BLOCK_IN_FLUSH |
>>> - BLOCK_CHANGED)));
>>> + DBUG_ASSERT(!(block->status & (BLOCK_IN_FLUSH | BLOCK_CHANGED)));
>> [7] I do not understand this change. The intention of this patch
>> seems to be to put restriction on error blocks, but here it seems you
>> remove the restriction that wait_for_readers should not be called for
>> error blocks. Please, explain.
>
> These asserts stem from my last big keycache patch. I added them, to
> understand, what kind of blocks can go through which functions. This was
> a several months task. I adjusted the asserts back and forth while my
> understanding of the code grew. Finally I ended up with a set which is
> pretty restrictive and does still survive all tests we've done since.
>
> However, during my work on the keycache I noticed that the handling of
> block errors was virtually not present, but at least not correct. I made
> some written notices and proposals to Monty, who reviewed my work at
> that time. But these problems are very rare, and so nothing has been
> done about it. Bug#40944 was the first time that a BLOCK_ERROR showed
> up, as far as I know.
>
> With this patch I propose a fix for the incorrect handling in
> key_cache_read(), which made the reported crash, and the very similar
> parts of key_cache_insert() and key_cache_write().
>
> The crash happened on an assert, which disallowed to put erroneous
> blocks in the LRU.
>
> My main proposal is to change the named functions so that they do not
> try to put erroneous blocks in the LRU any more, but to free them.
>
> In this course, it became necessary to let erroneous blocks through some
> functions, which are called from free_block().
>
> An alternative could be to clear the error flag before freeing the
> block, like I do with BLOCK_CHANGED in key_cache_write(). But I don't
> feel this to be a cleaner approach. If yo prefer, we can do a poll with
> Sergey.
This is not a big issue for me. I was just wondering.
>
> ...
>>> + This used to work until a brave soul tried to backup and restore
>>> + compressed tables. Now we know, that replacing the state info is
>>> + insufficient. The table is always re-created as a non-compressed
>>> + table.
>> Does this imply that if I backup a compressed table, it will not be
>> compressed after a restore? Please, clarify.
>
> Restore drops and re-creates the tables it restores. While the
> compression state might be visible through some SHOW functions, there is
> no way to create a compressed table by SQL. A CREATE TABLE statement as
> retrieved from SHOW CREATE TABLE does not show the compression state.
> Hence, the freshly re-created table is always a non-compressed one.
>
> Since the compression is handled completely inside MyISAM, and all
> required information is in the MyISAM table files .MYD and .MYI, the
> native driver does restore a compressed table. The next open of the
> files will load the internal MyISAM structures, so that it can handle
> the table correctly.
>
> On the server level this does not make a difference. But as long as it
> keeps the freshly created table open, it does also keep the MyISAM
> structures open, which were loaded from the freshly created,
> uncompressed table files. An attempt to use these structures to access
> the now compressed files fails.
>
> If we would use the built-in driver with MyISAM, each row would be
> uncompressed on read and put into the backup stream uncompressed. On
> restore, an uncompressed table would be created as usual. The built-in
> driver would write the uncompressed rows into it. The result would be an
> uncompressed table after restore.
This behavior is a bit strange, and could in theory create problems
for users with limited disk space since the restored database will
require more space than before. However, it seems that there is
nothing we can do about it.
--
Øystein
