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

• Davi Arnaut <davi@mysql.com> [07/08/31 06:29]:
  > ChangeSet@1.2510, 2007-08-30 23:00:33-03:00, davi@moksha.local +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 =.

Do you need help?

In addition to our free technical library, Pantek provides professional IT services to companies who utilize Open Source software: Learn more about Pantek's Technical Support offerings Place an order for 24/7/365 Technical Support here Learn more about Pantek, who we are, and what we do Chat with a Live Operator Related lists.mysql.com Links announce benchmarks bugs certification cluster commits dotnet gui-tools internals java maxdb myodbc mysql mysqldoc packagers perl plusplus replication win32 Request more information about Pantek

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);

Do you need more help?

In addition to our free technical library, Pantek provides professional IT services to companies who utilize Open Source software: Learn more about Pantek's Technical Support offerings Place an order for 24/7/365 Technical Support here Learn more about Pantek, who we are, and what we do Chat with a Live Operator Related lists.mysql.com Links announce benchmarks bugs certification cluster commits dotnet gui-tools internals java maxdb myodbc mysql mysqldoc packagers perl plusplus replication win32 Request more information about Pantek

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

-- 
MySQL Code Commits Mailing List
For list archives: 
http://lists.mysql.com/commits
To unsubscribe:    
http://lists.mysql.com/commits?unsub=lists@pantek.com