Fix table.Session.Execute ignoring options.WithCommit()#2091
Merged
Conversation
The tableClientExecutor.execute unconditionally overwrote request.TxControl with the txControl parameter after options (including options.WithCommit) had already been applied to the request, silently dropping the CommitTx=true flag set by WithCommit. This caused transactions opened with BeginTx to stay uncommitted when WithCommit was used on the Execute call. Drop the redundant assignment so modifications made by ExecuteDataQueryOption options are preserved. Extend the table_tx_control integration test with a write-with-commit scenario that upserts a row via WithCommit and asserts it is visible in a subsequent read. Made-with: Cursor
summaryInferred base version: v3.134.1 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2091 +/- ##
==========================================
- Coverage 74.66% 74.43% -0.24%
==========================================
Files 427 427
Lines 44011 44009 -2
==========================================
- Hits 32863 32760 -103
- Misses 9978 10061 +83
- Partials 1170 1188 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kprokopenko
force-pushed
the
withCommit_fix
branch
from
e59f583 to
cd9f134
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a regression where table.Session.Execute could silently drop options.WithCommit() by overwriting request.TxControl after options had already mutated it.
Changes:
- Removed the redundant
request.TxControl = ...assignment intableClientExecutor.executeso option-mutatedTxControlis preserved. - Renamed the now-unused
txControlparameter to_to keep thedataQueryExecutorinterface unchanged. - Added an integration subtest intended to verify that
options.WithCommit()actually commits aBeginTxtransaction.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
internal/table/session.go |
Stops clobbering ExecuteDataQueryRequest.TxControl so WithCommit() is honored. |
tests/integration/table_tx_control_test.go |
Adds an integration repro/verification for committing via options.WithCommit() on tx.Execute. |
CHANGELOG.md |
Documents the bugfix for end users at the top of the changelog. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
🌋 SLO Test Results🟡 8 workload(s) tested — 1 workload(s) exceeded warning thresholds
Threshold violations: database-sql-query:
Generated by ydb-slo-action |
asmyasnikov
approved these changes
Apr 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull request type
What is the current behavior?
tableClientExecutor.executeunconditionally overwroterequest.TxControlwith thetxControlargument afterExecuteDataQueryOptions had already been applied to the request. This silently dropped theCommitTx=trueflag thatoptions.WithCommit()sets on the request, so transactions opened viatable.BeginTx(...)remained uncommitted whenWithCommitwas passed toExecute.Issue: repro added in #2089.
What is the new behavior?
request.TxControl = txControl.ToYdbTableTransactionControl()assignment intableClientExecutor.executeso theTxControlbuilt bySession.Execute(includingExecuteDataQueryOptionmutations such asoptions.WithCommit()) is sent to the server as-is.txControlparameter to_— it is still required by thedataQueryExecutorinterface used by the query-service executor.TestTableTxControlwith awrite-with-commitsubtest that opens a transaction withBeginTx, upserts a row usingoptions.WithCommit(), then re-reads the row in an independentDefaultTxControltransaction to verify it was actually committed.Other information
Git-bisect traced the regression to commit
afdce85("replaced table transaction control types to internal/tx control").Made with Cursor