Skip to content

Commit

Permalink
Correctly handle multi-queries passed into single query API
Browse files Browse the repository at this point in the history
Summary:
Prior the refactoring work on Squangle calling the single query API with a multi-query generated some sort of exception, but during the refactor this instead hit a failed CHECK() and crashed.  We had no test in our own test suite for this but a customer ran into it (https://fb.workplace.com/groups/mysql.users/posts/26881187268169867/).

Here I've modified the code to detect the situation and return an appropriate exception for the issue.  In addition I've added a test to check for this in the MySQL C++ Client to prevent regressions in the future.

Reviewed By: fadimounir

Differential Revision: D62663718

fbshipit-source-id: 72aa3c4381c7f927b5a931a01685954ec421e3df
  • Loading branch information
Jay Edgar authored and facebook-github-bot committed Sep 16, 2024
1 parent 811133e commit bada5d6
Show file tree
Hide file tree
Showing 8 changed files with 23 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,11 @@ void FetchOperationImpl::actionable() {
++num_queries_executed_;
no_index_used_ |= conn().getNoIndexUsed();
was_slow_ |= conn().wasSlow();
op.notifyQuerySuccess(more_results);
if (!op.notifyQuerySuccess(more_results)) {
// This usually means a multi-query was passed to the single query API
active_fetch_action_ = FetchAction::CompleteOperation;
return;
}
}
current_row_stream_.reset();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ class FetchOperation : public Operation {
// store fetched data, initialize data).
virtual void notifyInitQuery() = 0;
virtual void notifyRowsReady() = 0;
virtual void notifyQuerySuccess(bool more_results) = 0;
virtual bool notifyQuerySuccess(bool more_results) = 0;
virtual void notifyFailure(OperationResult result) = 0;
virtual void notifyOperationCompleted(OperationResult result) = 0;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ void MultiQueryOperation::notifyFailure(OperationResult result) {
current_query_result_->setOperationResult(result);
}

void MultiQueryOperation::notifyQuerySuccess(bool) {
bool MultiQueryOperation::notifyQuerySuccess(bool) {
current_query_result_->setPartial(false);

current_query_result_->setOperationResult(OperationResult::Succeeded);
Expand All @@ -70,6 +70,7 @@ void MultiQueryOperation::notifyQuerySuccess(bool) {
query_results_.emplace_back(std::move(*current_query_result_.get()));
current_query_result_ =
std::make_unique<QueryResult>(current_query_result_->queryNum() + 1);
return true;
}

void MultiQueryOperation::notifyOperationCompleted(OperationResult result) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ class MultiQueryOperation : public FetchOperation {
protected:
void notifyInitQuery() override;
void notifyRowsReady() override;
void notifyQuerySuccess(bool more_results) override;
bool notifyQuerySuccess(bool more_results) override;
void notifyFailure(OperationResult result) override;
void notifyOperationCompleted(OperationResult result) override;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,13 @@ void MultiQueryStreamOperation::notifyRowsReady() {
invokeCallback(StreamState::RowsReady);
}

void MultiQueryStreamOperation::notifyQuerySuccess(bool) {
bool MultiQueryStreamOperation::notifyQuerySuccess(bool) {
// Query Boundary, only for streaming to allow the user to read from the
// connection.
// This will allow pause in the end of the query. End of operations don't
// allow.
invokeCallback(StreamState::QueryEnded);
return true;
}

void MultiQueryStreamOperation::notifyFailure(OperationResult) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class MultiQueryStreamOperation : public FetchOperation {

void notifyInitQuery() override;
void notifyRowsReady() override;
void notifyQuerySuccess(bool more_results) override;
bool notifyQuerySuccess(bool more_results) override;
void notifyFailure(OperationResult result) override;
void notifyOperationCompleted(OperationResult result) override;

Expand Down
12 changes: 10 additions & 2 deletions third-party/squangle/src/squangle/mysql_client/QueryOperation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,16 @@ void QueryOperation::notifyRowsReady() {
}
}

void QueryOperation::notifyQuerySuccess(bool more_results) {
bool QueryOperation::notifyQuerySuccess(bool more_results) {
if (more_results) {
// Bad usage of QueryOperation, we are going to cancel the query
// This is the single-query API; we can't support multi-queries here, so if
// we have more results we need to generate an error and cancel.
setAsyncClientError(
(unsigned int)SquangleErrno::SQ_INVALID_API_USAGE,
"Multi-queries are not supported in this API - "
"use the multi-query API instead");
cancel();
return false;
}

query_result_->setOperationResult(OperationResult::Succeeded);
Expand All @@ -62,6 +68,8 @@ void QueryOperation::notifyQuerySuccess(bool more_results) {

// We are not going to make callback to user now since this only one query,
// we make when we finish the operation

return true;
}

void QueryOperation::notifyFailure(OperationResult result) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ class QueryOperation : public FetchOperation {
protected:
void notifyInitQuery() override;
void notifyRowsReady() override;
void notifyQuerySuccess(bool more_results) override;
bool notifyQuerySuccess(bool more_results) override;
void notifyFailure(OperationResult result) override;
void notifyOperationCompleted(OperationResult result) override;

Expand Down

0 comments on commit bada5d6

Please sign in to comment.