Re: bk commit into 5.0 tree (svoj:1.2526) BUG#29734

[Andrei Elkin]

Sergey, salut!

The patch is okay, though I recommend to extend the comments and to
add in @todo or deploy an assert inlined further.

>
> ChangeSet@stripped, 2007-07-13 22:09:50+05:00, svoj@stripped +1 -0
>   BUG#29734 - thread_id=0 in binary log which leads to temporary table conflicts
>   
>   pseudo_thread_id was not reset properly after mysql_change_user() command,
>   which may cause replication failure when replicating temporary
>   tables.

Please state it more bold. My version:

pseudo_thread_id was reset to zero via mysql_change_user() handling
whereas there is no reason to do that.  Moreover, having two
concurrent threads that change user and create a namesake temp tables
leads to recording the dup pair of queries:

   set @@session.pseudo_thread_id = 0;
   CREATE temporary table `the namesake`;

which will stall the slave as the second instance can not be created.
And that is the bug case.

>   
>   Fixed by correcting pseudo_thread_id value after mysql_change_user().
>
>   sql/sql_class.cc@stripped, 2007-07-13 22:09:48+05:00, svoj@stripped +8 -1
>     Fixed that pseudo_thread_id was set to 0 after mysql_change_user().
>
> diff -Nrup a/sql/sql_class.cc b/sql/sql_class.cc
> --- a/sql/sql_class.cc	2007-07-06 03:43:02 +05:00
> +++ b/sql/sql_class.cc	2007-07-13 22:09:48 +05:00
> @@ -211,7 +211,7 @@ THD::THD()
>    time_after_lock=(time_t) 0;
>    current_linfo =  0;
>    slave_thread = 0;
> -  variables.pseudo_thread_id= 0;
> +  thread_id= 0;
>    one_shot_set= 0;
>    file_id = 0;
>    query_id= 0;
> @@ -328,6 +328,12 @@ void THD::init(void)
>  					       variables.date_format);
>    variables.datetime_format= date_time_format_copy((THD*) 0,
>  						   variables.datetime_format);
> +  /*
> +    variables= global_system_variables above has reset
> +    variables.pseudo_thread_id to 0. We need to correct it here to
> +    avoid temporary tables replication failure.
> +  */
> +  variables.pseudo_thread_id= thread_id;
>    pthread_mutex_unlock(&LOCK_global_system_variables);
>    server_status= SERVER_STATUS_AUTOCOMMIT;
>    if (variables.sql_mode & MODE_NO_BACKSLASH_ESCAPES)
> @@ -579,6 +585,7 @@ bool THD::store_globals()
>      By default 'slave_proxy_id' is 'thread_id'. They may later become different
>      if this is the slave SQL thread.
>    */
> +  /** @todo we already do it in init(), see if we still need to do it here */

There are at least four invocation places of THD::store_globals. 
You could try

   DBUG_ASSERT(variables.pseudo_thread_id == thread_id);

instead of the line
>    variables.pseudo_thread_id= thread_id;

Please either challange that or add the assert in @todo.

>    /*
>      We have to call thr_lock_info_init() again here as THD may have been
>

thanks for your good analysis and fruiful discussion!

Andrei