Re: bk commit into 5.0 tree (davi:1.2510) BUG#28870

[Konstantin Osipov]

* Davi Arnaut <davi@stripped> [07/08/31 06:29]:
> ChangeSet@stripped, 2007-08-30 23:00:33-03:00, davi@stripped +1 -0
>   Bug#28870 check that table locks are released/reset

The patch is OK to push. Please also see below.

>  /*
> +  Helper function for mysql_lock_tables error path cleanup
> +
> +  @param thd The current thread.
> +  @param sql_lock Lock structures to reset.
> +*/
> +static inline void mysql_lock_reset_and_free(THD *thd, MYSQL_LOCK *&sql_lock)

Doxygen comments start with /**
Please put an empty line after the comment as per the coding
style.
Please remove the 'inline' keyword. 'static' keyword gives a
modern compiler enough information to make a decision to inline or
not inline this function.

You're looking at thd->locked here,  this needs to be documented
clearly in the description.

> +{
> +  if (thd->locked && sql_lock->table_count)
> +    VOID(unlock_external(thd, sql_lock->table, sql_lock->table_count));
> +  /* Clear the lock type of all lock data to avoid reusage. */
> +  reset_lock_data(sql_lock);
> +  my_free((gptr) sql_lock,MYF(0));
> +  sql_lock=0;
> +}

We do not use references in the code, historically.
Please pass in MYSQL_LOCK **.

Coding style also demands a space after =.

Also I suggest a rename - mysql_ prefix is used for non-static
public functions. A better name perhaps, that is in line with
reset_lock_data, is reset_and_free_lock_data. 
BTW, do you still need reset_lock_data after your patch?
Please merge it with this function if not.

> +
> +/*
>    Lock tables.
>  
>    SYNOPSIS
> @@ -135,17 +151,12 @@ MYSQL_LOCK *mysql_lock_tables(THD *thd, 
>        */
>        if (wait_if_global_read_lock(thd, 1, 1))
>        {
> -        /* Clear the lock type of all lock data to avoid reusage. */
> -        reset_lock_data(sql_lock);
> -	my_free((gptr) sql_lock,MYF(0));
> -	sql_lock=0;
> +        mysql_lock_reset_and_free(thd, sql_lock);

ok.

>  	break;
>        }
>        if (thd->version != refresh_version)
>        {
> -        /* Clear the lock type of all lock data to avoid reusage. */
> -        reset_lock_data(sql_lock);
> -	my_free((gptr) sql_lock,MYF(0));
> +        mysql_lock_reset_and_free(thd, sql_lock);

ok.

>  	goto retry;
>        }
>      }
> @@ -154,10 +165,7 @@ MYSQL_LOCK *mysql_lock_tables(THD *thd, 
>      if (sql_lock->table_count && lock_external(thd, sql_lock->table,
>                                                 sql_lock->table_count))
>      {
> -      /* Clear the lock type of all lock data to avoid reusage. */
> -      reset_lock_data(sql_lock);
> -      my_free((gptr) sql_lock,MYF(0));
> -      sql_lock=0;
> +      mysql_lock_reset_and_free(thd, sql_lock);

ok.

>        break;
>      }
>      thd->proc_info="Table lock";
> @@ -172,22 +180,12 @@ MYSQL_LOCK *mysql_lock_tables(THD *thd, 
>                                                       thd->lock_id)];
>      if (rc > 1)                                 /* a timeout or a deadlock */
>      {
> -      if (sql_lock->table_count)
> -        VOID(unlock_external(thd, sql_lock->table, sql_lock->table_count));
> +      mysql_lock_reset_and_free(thd, sql_lock);

ok.

>        my_error(rc, MYF(0));
> -      my_free((gptr) sql_lock,MYF(0));
> -      sql_lock= 0;


>        break;
>      }
>      else if (rc == 1)                           /* aborted */
>      {
> -      /*
> -        reset_lock_data is required here. If thr_multi_lock fails it
> -        resets lock type for tables, which were locked before (and
> -        including) one that caused error. Lock type for other tables
> -        preserved.
> -      */
> -      reset_lock_data(sql_lock);
>        thd->some_tables_deleted=1;		// Try again
>        sql_lock->lock_count= 0;                  // Locks are already freed

Very tricky. At least add a comment what is going on (that the
current locks will be freed later).
>      }
> @@ -205,9 +203,17 @@ MYSQL_LOCK *mysql_lock_tables(THD *thd, 
>      }
>      thd->proc_info=0;
>  
> +    if (sql_lock->lock_count)
> +        thr_multi_unlock(sql_lock->locks,sql_lock->lock_count);
> +
> +    /*
> +      If thr_multi_lock fails it resets lock type for tables, which
> +      were locked before (and including) one that caused error. Lock
> +      type for other tables preserved.
> +    */
> +    mysql_lock_reset_and_free(thd, sql_lock);
> +

OK.

>      /* some table was altered or deleted. reopen tables marked deleted */
> -    mysql_unlock_tables(thd,sql_lock);
> -    thd->locked=0;
>  retry:
>      sql_lock=0;
>      if (flags & MYSQL_LOCK_NOTIFY_IF_NEED_REOPEN)
> 

-- 
-- Konstantin Osipov              Software Developer, Moscow, Russia
-- MySQL AB, www.mysql.com   The best DATABASE COMPANY in the GALAXY