Skip to content

server: return OK when there is no stmt#22738

Merged
ti-srebot merged 4 commits intopingcap:release-4.0from
morgo:fix-22646
Feb 5, 2021
Merged

server: return OK when there is no stmt#22738
ti-srebot merged 4 commits intopingcap:release-4.0from
morgo:fix-22646

Conversation

@morgo
Copy link
Contributor

@morgo morgo commented Feb 5, 2021

What problem does this PR solve?

Issue Number: Fixes #22646

Problem Summary:

There was a regression introduced in the 4.0 branch by a cherry-pick. The issue does not exist in the master branch, or any public released binaries yet. It was caught before 4.0.11 was released. It does not exist in 4.0.10.

I am unsure if connectors rely on this functionality. There is a COM_PING protocol feature, but it's possible that an incorrectly implemented driver uses an empty query instead. In which case it could be quite a serious issue.

What is changed and how it works?

Proposal: xxx

What's Changed:

How it Works:

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Manual test (add detailed scripts or steps below)

Side effects

  • Performance regression
    • Consumes more CPU
    • Consumes more MEM
  • Breaking backward compatibility

Release note

  • No release note

@morgo morgo requested a review from tiancaiamao February 5, 2021 03:30
@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label Feb 5, 2021
Copy link
Contributor

@jackysp jackysp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to add a unit test case?

@morgo
Copy link
Contributor Author

morgo commented Feb 5, 2021

Is it possible to add a unit test case?

Done

@coocood
Copy link
Member

coocood commented Feb 5, 2021

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 5, 2021
@morgo
Copy link
Contributor Author

morgo commented Feb 5, 2021

Note: I've changed the test to add a timeout component. There is no test for master for this case, so I will prepare another PR shortly.

Copy link
Member

@bb7133 bb7133 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Feb 5, 2021
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Feb 5, 2021
@jebter
Copy link

jebter commented Feb 5, 2021

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 5, 2021
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 631dbfd into pingcap:release-4.0 Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants