Hi Chuck,
Chuck Bell wrote:
(cut)
>> I see no reason for introducing the new ddl_timeout member to
>> THD structure. It is only used internally inside
>> check_DDL_blocker() method and can be declared as a local
>> variable there.
>
> There is a good reason -- how will each thread (connection) be able to
> accurately count it's timer for the timeout? For example, suppose con1 is
> blocked at time_0 with a timeout of 30 seconds and con2 is blocked at time_0
> + 5 seconds with a timeout of 10 seconds. When should each timeout? If the
> value is local, you are forcing all threads to use the same timeout which in
> this case occurs at time_0 + 15 seconds (because con2 reset the timeout
> variable) which is 15 seconds too early for con1. No, it needs to be
Why local value forces all threads to use the same timeout? Local means that
each thread (each connection) has a separate copy of the timespec structure and
con2 can not reset timeout of con1. Your argument looks like if you are talking
about a global timeout value...
> per-thread so that the timeouts can occur at the correct moments for each
> thread. This also holds true for when the global timeout is set.
>
The change which I have in mind is as follows:
> my_bool DDL_blocker_class::check_DDL_blocker(THD *thd)
> {
> int ret = 0;
> bool timeout= FALSE;
+ struct timespec ddl_timeout;
<...>
> if (thd->variables.ddl_wait_timeout > 0)
> {
> + set_timespec(ddl_timeout, thd->variables.ddl_wait_timeout);
> timeout= TRUE;
> }
> else if (global_system_variables.ddl_wait_timeout > 0)
> {
> + set_timespec(ddl_timeout, global_system_variables.ddl_wait_timeout);
> timeout= TRUE;
> }
<...>
> while (DDL_blocked && !thd->DDL_exception && (ret == 0))
> {
> if (!timeout)
> pthread_cond_wait(&COND_DDL_blocker, &THR_LOCK_DDL_is_blocked);
> else
> ret= pthread_cond_timedwait(&COND_DDL_blocker,
&THR_LOCK_DDL_is_blocked,
> + &ddl_timeout);
> }
<...>
> DBUG_RETURN(FALSE);
> }
Thus the ddl_timeout structure is local to the check_DDL_blocker() method. It is
filled there and used to time the pthread_cond_wait() call. Obviously, it is
completely independent from the same structures used in any other invocations of
check_DDL_blocker() which might run concurrently in other threads. Therefore the
timeouts in different threads will be independent from each other without
polluting the THD structure with new members.
(cut)
>>> (uchar**) &opt_date_time_formats[MYSQL_TIMESTAMP_DATETIME],
>>> (uchar**) &opt_date_time_formats[MYSQL_TIMESTAMP_DATETIME],
>>> 0, GET_STR, REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
>>> - { "default_week_format", OPT_DEFAULT_WEEK_FORMAT,
>>> + {"ddl_wait_timeout", OPT_DDL_WAIT_TIMEOUT,
>>> + "The number of seconds a DDL can be blocked before cancelled.",
>>> + (uchar**) &global_system_variables.ddl_wait_timeout,
>>> + (uchar**) &max_system_variables.ddl_wait_timeout, 0, GET_ULONG,
>>> + REQUIRED_ARG, DDL_WAIT_TIMEOUT, 0, 1024 * 1024 * 1024, 0, 1, 0},
>> Could you shortly explain what the entries mean? This is to
>> help me learn
>> something here and to check that you know what you are doing.
>
> See my_getopt.h @line#43:
>
> struct my_option
> {
> const char *name; /* Name of the option */
> int id; /* unique id or short option */
> const char *comment; /* option comment, for autom. --help
> */
> uchar **value; /* The variable value */
> uchar **u_max_value; /* The user def. max variable value
> */
> struct st_typelib *typelib; /* Pointer to possible values */
> ulong var_type;
> enum get_opt_arg_type arg_type;
> longlong def_value; /* Default value */
> longlong min_value; /* Min allowed value */
> longlong max_value; /* Max allowed value */
> longlong sub_size; /* Subtract this from given value */
> long block_size; /* Value should be a mult. of this
> */
> void *app_type; /* To be used by an application */
> };
>
Thanks for reference. After seeing this I have two more questions.
1. Why do you use REQUIRED_ARG as the value of arg_type member? Shouldn't it be
NO_ARG instead?
2. Where max_system_variables.ddl_wait_timeout is initialized?
Rafal
