-
Notifications
You must be signed in to change notification settings - Fork 85
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
Prepare Statement Improvement #3140
Conversation
575e5f7
to
c08c334
Compare
b3407f2
to
9e4b419
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me. You can merge if the CI passes.
@@ -323,6 +345,9 @@ std::unique_ptr<PreparedStatement> ClientContext::prepareNoLock( | |||
} else { | |||
preparedStatement->logicalPlans = std::move(plans); | |||
} | |||
if (transactionContext->isAutoTransaction() && requireNewTx) { | |||
this->transactionContext->commit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we have a similar check when rollback?
if (!preparedStatement->isSuccess()) { | ||
return queryResultWithError(preparedStatement->errMsg); | ||
} | ||
if (preparedStatement->preparedSummary.statementType != common::StatementType::TRANSACTION && | ||
this->getTx() == nullptr) { | ||
if (preparedStatement->parsedStatement->requireTx() && requiredNexTx && getTx() == nullptr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can u branch out from this branch and try commit after prepare and we open a new transaction during execute. In this design, we should no longer need to pass requiredNexTx
e006d4c
to
21cd43c
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3140 +/- ##
==========================================
- Coverage 92.08% 92.07% -0.01%
==========================================
Files 1168 1168
Lines 44065 44091 +26
==========================================
+ Hits 40577 40597 +20
- Misses 3488 3494 +6 ☔ View full report in Codecov by Sentry. |
see #3065
A little different implementation: for a query statement, we always need to prepare, and we will start a transaction.
prepare()
is a standalone call. (requiredNewTx=true)prepare()
called inquery()
. Inquery()
,execute()
is always followed withprepare()
, we will close the transaction inexecute()
. (requiredNewTx=false)Similar logic is used in
execute()
.