Chuck Bell wrote:
> Hi Oystein,
>
> My replies to your questions...
>
>> Maybe not the right time to raise this question, but why do
>> we need a separate timeout for the ddl blocker? Why not use
>> the existing table_lock_wait_timeout for this? Seems a bit
>> cumbersome to have to set several different timeout variables
>> in order to control when a query times out.
>
> This timeout -- as Peter has so eloquently stated -- affects only DDL
> operations that occur during backup. Hence the name change from
> ddl_wait_timeout. These DDL operations are blocked by the DDL blocker so any
> other timeout does not apply -- the DDL block occurs before tables are
> locked so not even the table lock timeout will work. Since the DDL blocker
> (currently) has no timeout feature, a DDL operation blocked by backup is
> blocked for as long as backup runs. Thus, if a backup runs for a very long
> time, the DDL operations are stuck. We needed a way to allow the DDL
> operations to fail if backup runs for a long time (or is hung).
I understand the motivation for a timeout. I am not sure that using a
separate variable for this timeout is the solution. Why not have a
single variable that specifies timeouts for all types of queries.
Otherwise, you may have to set several variables in order to make sure
your query (in this case a DDL op) times out when you want it to time
out.
>
>> I have not been able to understand the mechanism used for
>> variables, but since you are following the same pattern as
>> for existing variables, I guess it is OK. However, it
>> puzzles me a bit that I am not able to find any code that
>> instantiate the new sys_var_backup_wait_timeout class nor
>> implements the declared sys_backup_wait_timeout function.
>
> Welcome to one of the most treacherous sections of the MySQL code! The
> variable code is second to the optimizer for developer fatalities.
>
> Ok, all jocularity aside...
>
> Instantiation is in set_var.cc @ 230.
Ah, please excuse me, my C++-knowledge is a bit rusty. I thought that
was just a declaration. I think static confused me a bit.
>
> The function sys_backup_wait_timeout is in the same place. It is of type
> sys_var_backup_wait_timeout class.
>
>> In addition, the following note in set_var.cc concerns me:
>> @note
>> Be careful with var->save_result: sys_var::check() only updates
>> ulonglong_value; so other members of the union are
>> garbage then; to use
>> them you must first assign a value to them (in specific
>> ::check() for
>> example).
>>
>> I see that sys_var_backup_wait_timeout::update accesses
>> save_result.ulong_value. Is the above note relevant in that context?
>
> I don't think so since we do not use it for anything else. Some variables
> could be used for more than one type. We only use backup time out as ulong.
OK. I trust you on this.
>
>> Another concern is that the ddl_blocker seems very brittle
>> with respect to errors. It seems to me that in case of
>> errors in DDL operations, it may happen that only one of the
>> start_DDL()/end_DDL() functions are called. This is a more
>> general problem than related to this patch, but I think this
>> patch adds new risks in this area (see comments below).
>
> Care was taken when the DDL blocker was written to ensure the pair of
> start/end methods were called. Take a look in sql_parse where most of these
> occur and you will see the end method is called in the event of an error.
I am not convinced that this is true. There seems to be several 'goto
err' that skips the end method.
> It is also true that the ddl blocker is not intended to be long lived. Once
> the runtime team gets their longer term plans in motion, the ddl blocker
> will be made obsolete and removed. So we need to strive for a compromise of
> what is good enough for backup and perfection for the server general
> purpose.
I am not sure it is a good idea to compromise on with respect to
consistency of multi-threaded access. That often leads to errors that
are difficult to understand and reproduce. For counters like in this
case, I think it is best that each thread records whether they have
incremented the counter, so that they know whether to decrement it at
the end.
...
>>> +# Create data for this test and tailor it to the test.
>>> +--source include/backup_ddl_create_data.inc
>>> +
>>> +--disable_warnings
>>> +DROP TABLE IF EXISTS bup_ddl_blocker.t1, bup_ddl_blocker.t2,
>>> + bup_ddl_blocker.t3, bup_ddl_blocker.t4;
>>> +--enable_warnings
>> What is the point in first running a script to create some
>> tables and then drop them right afterwards?
>
> The script is the same for all of the test cases. It saves time and much
> duplication. Some tests require all tables but this one doesn't need them at
> all (a special case). If you are completely turned off by this I can copy
> and paste but that will only add to confusion if someone needs to modify the
> script.
I do not see how this leads to confusion in this case since there are
many tests that do not use this script, and all that are needed in
this case is to create the database. I think it is more confusing to
try to figure out which part of the script is actually needed in this
case.
>
>>> +--echo Set ddl timeout to 1 second
>>> +SET backup_wait_timeout = 1;
>>> +SHOW VARIABLES LIKE 'backup_wait%';
>>> +
>>> +--echo con2: Try a ddl operation and watch it expire --error
>>> +ER_DDL_TIMEOUT CREATE TABLE bup_ddl_blocker.t2 (col_a CHAR(40))
>>> +ENGINE=MEMORY;
>> Would it be possible to check that the timing is right? That
>> is, that it times out after approx. 1 sec. and not 10?
>
> I really don't think so. The timing aspect in mysql testing is really
> difficult to predict. On some machines, it works correctly but on others the
> timing can be so bad several minutes could go by before the processor
> (simulated in VM) gets time from the clock.
>
I agree.
...
>>> === modified file 'sql/sql_parse.cc'
>>> --- a/sql/sql_parse.cc 2008-07-14 14:56:49 +0000
>>> +++ b/sql/sql_parse.cc 2008-08-08 14:07:00 +0000
>>> @@ -2326,7 +2326,11 @@ mysql_execute_command(THD *thd)
>>> TABLE in the same way. That way we avoid that a new table is
>>> created during a gobal read lock.
>>> */
>>> - DDL_blocker->check_DDL_blocker(thd);
>>> + if (!DDL_blocker->check_DDL_blocker(thd))
>>> + {
>>> + res= 1;
>>> + goto end_with_restore_list;
>>> + }
>> The got leads to code where end_DDL is called. However, if
>> DDL op timed out, start_DDL has not been called. This can
>> lead to accounting problems wrt DDL_blocks counter.
>
> No, in check_ddl_blocker(), it either returns success or it fails with a
> timeout error. There are no other error conditions that could result in a
> failed return.
>
> See:
> if (ret -- 0)
> ...
> else
> my_error()
>
I think you misunderstood my comment here. Take a scenario where the
following sequence of command arrives on different connections:
1. backup
2. create table (timeout = 100)
3. create table (timeout = 0)
4. backup
In think the following might happen in this scenario:
When 2. is executed it is blocked by the ddl_blocker and DDL_blocks is
incremented to 1. When 3. is executed check_ddl_blocker() will return
without incrementing DDL_blocks. However, 'goto
end_with_restore_list' will cause end_DDL to be called causing
DDL_blocks to be decremented to 0. The blocked create table will
continue when the first backup completes. However, since DDL_blocks
is 0 at that point, another backup may start before the create table
operation is completed.
>>> if (!thd->locked_tables_mode &&
>>> !(need_start_waiting=
>> !wait_if_global_read_lock(thd, 0, 1)))
>>> {
>>> @@ -2467,7 +2471,8 @@ end_with_restore_list:
>>> table without having to do a full rebuild.
>>> */
>>> {
>>> - DDL_blocker->check_DDL_blocker(thd);
>>> + if (!DDL_blocker->check_DDL_blocker(thd))
>>> + goto error;
>> This error handling is OK for the call to check_DDL_blocker,
>> but if later errors causes goto error, end_DDL will not be
>> called even if check_DDL_blocker called start_DDL. (This is
>> not introduced by this patch. Maybe another bug report
>> should be created for this problem.)
>
> I see what you are talking about, but in this case there is an error and the
> query is going to fail.
The problem is not related to the failing query. The problem is that
failing to decrement DDL_blocks will, as far as I understand, block
future backup operations.
> I agree this is outside the scope of this bug (BUG#33414). However,
> if feel strongly this is wrong then please open a new bug on the
> problem with a test that shows how to produce the error conditions.
I can open a new bug, but I cannot guarantee that I will able to come
up with a test that shows how to produce the error condition. Neither
do I think that should be required in order to file issues detected by
code reading. If we only fix multithreading issues that we are able to
reproduce, I think we will have a very unstable product with lots of
intermittent failures and new issues constantly popping up.
--
Øystein
