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

sql: execute batch statements in an implicit transaction #76834

Merged
merged 3 commits into from
Mar 15, 2022

Conversation

rafiss
Copy link
Collaborator

@rafiss rafiss commented Feb 20, 2022

fixes #44803
relates to #76490

Release justification: high value bug fix to existing functionality

*: prepare tests for batch stmt txn change

This commit will make the next one easier to review.

sql: execute batch statements in an implicit transaction

Release note (bug fix): Previously statements that arrived in a batch
during the simple query protocol would all execute in their own implicit
transactions. Now, we match the Postgres wire protocol, so all these
statements share the same implicit transaction. If a BEGIN is included
in a statement batch, then the existing implicit transaction is
upgraded to an explicit transaction.

sql: add session var for old implicit txn behavior

Release note (sql change): The enable_implicit_transaction_for_batch_statements
session variable was added. It defaults to true. When it is true,
multiple statements in a single query (a.k.a. a "batch statement") will
all be run in the same implicit transaction, which matches the Postgres
wire protocol. This setting is provided for users who want to preserve
the behavior of CockroachDB versions v21.2 and earlier.

@rafiss rafiss requested a review from andreimatei February 20, 2022 23:26
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rafiss rafiss force-pushed the batch-stmt-implicit-txn branch 2 times, most recently from c4f5971 to 7205a02 Compare February 21, 2022 22:58
@rafiss rafiss requested review from otan and a team February 21, 2022 23:18
@rafiss rafiss force-pushed the batch-stmt-implicit-txn branch from 7205a02 to 417fddf Compare February 22, 2022 17:56
@rafiss rafiss force-pushed the batch-stmt-implicit-txn branch 2 times, most recently from bd01597 to 1cde010 Compare February 22, 2022 20:57
Copy link
Contributor

@otan otan left a comment

Choose a reason for hiding this comment

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

change by itself lgtm, but i think this may cause some tests to fail as they use multiple schema changes in the same cmd. the test failures seem suspicious as a result.

@rafiss rafiss force-pushed the batch-stmt-implicit-txn branch from 1cde010 to 1073168 Compare February 25, 2022 18:26
@rafiss rafiss requested a review from a team as a code owner February 25, 2022 18:26
@rafiss rafiss requested a review from a team February 25, 2022 18:26
@rafiss rafiss requested a review from a team as a code owner February 25, 2022 18:26
@rafiss rafiss requested review from a team and stevendanna and removed request for a team February 25, 2022 18:26
@blathers-crl blathers-crl bot requested a review from otan February 25, 2022 18:26
@rafiss rafiss force-pushed the batch-stmt-implicit-txn branch from 1073168 to d463dea Compare February 25, 2022 22:50
@rafiss rafiss requested a review from a team as a code owner February 25, 2022 22:50
@rafiss rafiss requested review from a team and removed request for a team February 28, 2022 16:28
@rafiss rafiss force-pushed the batch-stmt-implicit-txn branch from d463dea to 38d4cf7 Compare February 28, 2022 20:29
@rafiss rafiss requested a review from a team as a code owner February 28, 2022 20:29
@rafiss rafiss force-pushed the batch-stmt-implicit-txn branch 2 times, most recently from 49d30bc to 8ea59b7 Compare March 1, 2022 07:12
@rafiss rafiss requested a review from a team as a code owner March 1, 2022 07:12
@rafiss rafiss force-pushed the batch-stmt-implicit-txn branch from 8ea59b7 to e138f8a Compare March 1, 2022 19:48
@craig
Copy link
Contributor

craig bot commented Mar 15, 2022

Canceled.

@rafiss
Copy link
Collaborator Author

rafiss commented Mar 15, 2022

bors r=otan

craig bot pushed a commit that referenced this pull request Mar 15, 2022
76834: sql: execute batch statements in an implicit transaction r=otan a=rafiss

fixes #44803
relates to #76490

Release justification: high value bug fix to existing functionality 

### *: prepare tests for batch stmt txn change

This commit will make the next one easier to review.

### sql: execute batch statements in an implicit transaction

Release note (bug fix): Previously statements that arrived in a batch
during the simple query protocol would all execute in their own implicit
transactions. Now, we match the Postgres wire protocol, so all these
statements share the same implicit transaction. If a BEGIN is included
in a statement batch, then the existing implicit transaction is
upgraded to an explicit transaction.

### sql: add session var for old implicit txn behavior

Release note (sql change): The enable_implicit_transaction_for_batch_statements
session variable was added. It defaults to true. When it is true,
multiple statements in a single query (a.k.a. a "batch statement") will
all be run in the same implicit transaction, which matches the Postgres
wire protocol. This setting is provided for users who want to preserve
the behavior of CockroachDB versions v21.2 and earlier.

77780: roachtest: update 22.1 version map to v21.2.7 r=celiala a=Azhng

Release note: None
Release justification: non-production code change.

77781: cloud: bump orchestrator to v21.2.7 r=celiala a=Azhng

Release note: None
Release justification: non-production code change.

77803: kvserverbase: move MaxCommandSizeDefault out of kvserver r=ajwerner a=ajwerner

```
$ bazel query "somepath(//pkg/sql, //pkg/kv/kvserver)"
INFO: Empty results
Loading: 0 packages loaded
```

Release justification: non-production code changes

Release note: None

Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Azhng <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
@rafiss rafiss force-pushed the batch-stmt-implicit-txn branch from 53ecf6c to 3f45266 Compare March 15, 2022 15:52
@craig
Copy link
Contributor

craig bot commented Mar 15, 2022

Canceled.

@rafiss
Copy link
Collaborator Author

rafiss commented Mar 15, 2022

bors r=otan

@rafiss
Copy link
Collaborator Author

rafiss commented Mar 15, 2022

bors r-

rafiss added 3 commits March 15, 2022 15:15
This commit will make the next one easier to review.

Release justification: test only change

Release note: None
Release justification: high value bug fix to existing functionality.

Release note (bug fix): Previously statements that arrived in a batch
during the simple query protocol would all execute in their own implicit
transactions. Now, we match the Postgres wire protocol, so all these
statements share the same implicit transaction. If a BEGIN is included
in a statement batch, then the existing implicit transaction is
upgraded to an explicit transaction.
Release justification: low risk new setting.

Release note (sql change): The enable_implicit_transaction_for_batch_statements
session variable was added. It defaults to true. When it is true,
multiple statements in a single query (a.k.a. a "batch statement") will
all be run in the same implicit transaction, which matches the Postgres
wire protocol. This setting is provided for users who want to preserve
the behavior of CockroachDB versions v21.2 and earlier.
@rafiss rafiss force-pushed the batch-stmt-implicit-txn branch from 3f45266 to 4fc43c8 Compare March 15, 2022 19:15
@rafiss
Copy link
Collaborator Author

rafiss commented Mar 15, 2022

bors r=otan

@craig
Copy link
Contributor

craig bot commented Mar 15, 2022

Build succeeded:

@craig craig bot merged commit cfe38f2 into cockroachdb:master Mar 15, 2022
@blathers-crl
Copy link

blathers-crl bot commented Mar 15, 2022

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating backport branch refs/heads/blathers/backport-release-21.2-76834: POST https://api.github.com/repos/cockroachlabs/cockroach/git/refs: 403 Resource not accessible by integration []

Backport to branch 21.2.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@rafiss rafiss deleted the batch-stmt-implicit-txn branch March 16, 2022 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: pg mandates multi-stmt batches to execute as single txn
5 participants