Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update the handling of COM_SLEEP #38612

Closed
igxlin opened this issue Oct 23, 2022 · 4 comments · Fixed by #38645
Closed

Update the handling of COM_SLEEP #38612

igxlin opened this issue Oct 23, 2022 · 4 comments · Fixed by #38645
Assignees
Labels
type/enhancement The issue or PR belongs to an enhancement.

Comments

@igxlin
Copy link
Contributor

igxlin commented Oct 23, 2022

Enhancement

The current tidb (commit 41518c9e02) handles COM_SLEEP specially and doesn't take it as an unknown command.

// server/conn.go:(*clientConn).dispatch()
	case mysql.ComSleep:
		// TODO: According to mysql document, this command is supposed to be used only internally.
		// So it's just a temp fix, not sure if it's done right.
		// Investigate this command and write test case later.
		return nil

However, the current mysql (commit a246bad7) takes it as an unknown command and raise an error.

// sql/sql_parse.cc:dispatch_command()
    case COM_SLEEP:
    case COM_CONNECT:         // Impossible here
    case COM_TIME:            // Impossible from client
    case COM_DELAYED_INSERT:  // INSERT DELAYED has been removed.
    case COM_END:
    default:
      my_error(ER_UNKNOWN_COM_ERROR, MYF(0));
      break;

Should we follow the mysql implementation to return unknown error when handling this command?

@igxlin igxlin added the type/enhancement The issue or PR belongs to an enhancement. label Oct 23, 2022
@xhebox
Copy link
Contributor

xhebox commented Oct 26, 2022

Which version is the mysql? If it is mysql5, we should follow.

If it is a newer version, from the point of compatibility, it is best to be silent.

@igxlin
Copy link
Contributor Author

igxlin commented Oct 26, 2022

Which version is the mysql? If it is mysql5, we should follow.

If it is a newer version, from the point of compatibility, it is best to be silent.

@xhebox , in mysql 5.7, it is also taken as an unknown command and error is raised.

Source code: https://github.com/mysql/mysql-server/blob/c4f63caa8d9f30b2850672291e0ad0928dd89d0e/sql/sql_parse.cc#L1890

  case COM_SLEEP:
  case COM_CONNECT:				// Impossible here
  case COM_TIME:				// Impossible from client
  case COM_DELAYED_INSERT: // INSERT DELAYED has been removed.
  case COM_END:
  default:
    my_message(ER_UNKNOWN_COM_ERROR, ER(ER_UNKNOWN_COM_ERROR), MYF(0));
    break;
  }

I will create a PR on it if it is OK to you.

@xhebox
Copy link
Contributor

xhebox commented Oct 26, 2022

I will create a PR on it if it is OK to you.

Sure, thanks for your contribution.

@igxlin
Copy link
Contributor Author

igxlin commented Oct 26, 2022

/assign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants