MySQL Bugs: #77689: mysql_execute_command SQLCOM_UNLOCK_TABLES redundant trans_check

Bug #77689	mysql_execute_command SQLCOM_UNLOCK_TABLES redundant trans_check_state check?
Submitted:	11 Jul 2015 14:27	Modified:	2 Sep 2019 12:04
Reporter:	Laurynas Biveinis (OCA)	Email Updates:
Status:	Verified	Impact on me:	None
Category:	MySQL Server: Locking	Severity:	S5 (Performance)
Version:	5.6,5.7	OS:	Any
Assigned to:		CPU Architecture:	Any
Tags:	cleanup

Description:
mysql_execute_command(THD *thd)
...
  case SQLCOM_UNLOCK_TABLES:
    if (thd->variables.option_bits & OPTION_TABLE_LOCK)
    {
      /*
        Can we commit safely? If not, return to avoid releasing
        transactional metadata locks.
      */
      if (trans_check_state(thd))
        DBUG_RETURN(-1);
...

trans_check_state returns TRUE iff we are either in a SP, either in an XA transaction. But SP cannot contain UNLOCK TABLES command, and XA cannot have OPTION_TABLE_LOCK at this point (it cannot be active at XA START nor you can issue a command that would set it inside XA).

So, is trans_check_state here dead code? It was added by b6c3c7978e89dc097889bd38d04fed58b991a828, and it is not clear why.

Obviously this is a very minor bug if confirmed, this is more of me asking if my code understanding here is correct.

How to repeat:
Code analysis

Suggested fix:
Remove trans_check_state call

commit b6c3c7978e89dc097889bd38d04fed58b991a828
Author: Jon Olav Hauglid <jon.hauglid@oracle.com>
Date:   Wed Apr 10 13:44:46 2013 +0200

    Bug#16362832: DDL IN XA TRANSACTION RELEASES TRANSACTION LOCKS
                  WITHOUT ENDING TRANSACTION
    
    If statements causing implicit commit are executed with an XA
    transaction in ACTIVE state, "The command cannot be executed
    when global transaction is in the ACTIVE state" error is
    reported. This error causes the implicit commit to fail (as it should).
    
    The problem was that if the implicit commit at the start of a
    statement execution failed, we also tried to commit at the end
    of the statement. This commit would fail for the same reason.
    However for this commit we assumed that the transaction
    was either successfully committed or rolled back and therefore
    released transactional metadata locks.
    
    This meant that the objects accessed earlier in the XA transaction
    no longer were protected and could be e.g. dropped by other
    connections. This again could lead to assertions, valgrind warnings
    and various other problems when the XA transaction later tried
    to commit.
    
    This patch fixes the problem by aborting the statement if the
    implicit commit at the beginning of the statement execution fails.
    This means that the release of transactional locks at the end
    of execution of statements causing implicit commit, will be skipped.

See also remotely related bug 80174

Hi Laurynas,

Thank you for your bug report.

First of all, your user tag is wrong, since recently, but not when you reported the bug.

Second, the code has changed a lot in recent 5.7 and in 8.0. If you study the entire code in latest 5.7, you will see that the option that is used in the condition is cleared in all cases that you mentioned. 8.0 goes even further and changes the code completely.

Hence, do you agree that we close this bug ????

Sinisa - 

I'll leave it to the organisation OCA admins to clear the tag :)

As for the bug, you say "the option that is used in the condition is cleared in all cases that you mentioned" - which is my bug report exactly, the option cannot be set here, hence the if statement can be replaced with DBUG_ASSERT with negated if condition.

Hi Laurynas,

This makes it a small performance improvement, so I verify it as such.

Verified as a performance improvement ......