maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Nirbhay!

On Jun 08, Nirbhay Choubey wrote:
> revision-id: 9a03e780f179b575dc0d7be8de0c1731adf8726e (mariadb-10.2.0-82-g9a03e78)
> parent(s): 3826f59972f19791754c7ef542e9e4a9beb67004
> author: Nirbhay Choubey
> committer: Nirbhay Choubey
> timestamp: 2016-06-08 10:09:42 -0400
> message:
> 
> MDEV-5535: Cannot reopen temporary table
> 
> Fix for some test failures and clean up.
> 
> diff --git a/sql/temporary_tables.cc b/sql/temporary_tables.cc
> index acb9507..e62cc13 100644
> --- a/sql/temporary_tables.cc
> +++ b/sql/temporary_tables.cc
> @@ -30,6 +30,9 @@
>  #define IS_USER_TABLE(A) ((A->tmp_table == TRANSACTIONAL_TMP_TABLE) || \
>                            (A->tmp_table == NON_TRANSACTIONAL_TMP_TABLE))
>  
> +#define HAS_TEMPORARY_TABLES ((!rgi_slave && has_temporary_tables()) || \
> +                              (rgi_slave &&                             \
> +                               unlikely(has_slave_temporary_tables())))

Ok, now I'm confused.
This begs for a function, like

  if (rgi_slave)
    return has_temporary_tables();
  else
    return has_temporary_tables();

but we had it and have just split it because you always called it with
a constant true or false, never with rgi_slave.

So, a function is still needed? Or was it not needed before but is
needed now?

Oh, I see... It was

 bool Temporary_tables::is_empty(bool check_slave)
 {
   DBUG_ENTER("Temporary_tables::is_empty");
   bool result;
   if (!m_thd)
     DBUG_RETURN(true);
   rpl_group_info *rgi_slave= m_thd->rgi_slave;
   if (check_slave && rgi_slave)
     result= (rgi_slave->rli->save_temp_table_shares == NULL) ? true : false;
   else
     result= (m_table_shares == NULL) ? true : false;
   DBUG_RETURN(result);
 }

so you should've split it into:

  bool has_temporary_tables()
  { return m_table_shares; }

  bool has_slave_temporary_tables()
  { return rgi_slave ? rgi_slave->rli->save_temp_table_shares : has_temporary_tables(); }

because your 'check_slave' was always true or always false, so you
should've split this function in two. one for check_slave=true, and one
for check_slave=false;

>  /**
>    Check whether temporary tables exist. The decision is made based on the
> @@ -117,7 +117,7 @@ TABLE *THD::find_temporary_table(const char *db,
>    uint key_length;
>    bool locked;
>  
> -  if (!(has_temporary_tables() || (rgi_slave && has_slave_temporary_tables())))
> +  if (!HAS_TEMPORARY_TABLES)
>    {
>      DBUG_RETURN(NULL);
>    }
Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx