server: support separate transactions on sql api #108447

THardy98 · 2023-08-09T15:02:50Z

This change introduces a separate_txns field to the sql api endpoint.
This field allows callers to run provided statements in separate
transactions. Failures in separate transactions are scoped to their
respective transaction and do not interfere with the execution of
subsequent transactions. The exception to this case is if the max
response size is exceeded. If the response size is exceeded in an
earlier transaction, it will still exceed the limit in subsequent
transactions. The response-level error using separate_txns is a
generic message: separate transaction payload encountered transaction error(s), indicating that an error has occurred in at least one of the
transactions.

We additionally add a stop_on_error field to allow callers to stop
execution of subsequent txns (i.e. in the case where separate_txns is
true).

Release note: None

cockroach-teamcity · 2023-08-09T15:03:05Z

This change is

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @THardy98)

pkg/server/api_v2_sql.go line 377 at r1 (raw file):

	err = timeutil.RunWithTimeout(ctx, "run-sql-via-api", timeout, func(ctx context.Context) error {
		if !requestPayload.SeparateTxns {
			execRes, _, err := a.runSingleTxn(ctx, 0, 0, username, requestPayload, requestPayload.Statements, options)

I feel there is a lot of code duplication here.

Can you perhaps simplify the control flow a bit? Something like this:
knz@62099b2

j82w

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @THardy98)

pkg/server/api_v2_sql.go line 209 at r1 (raw file):

	Execute         bool        `json:"execute"`
	SeparateTxns    bool        `json:"separate_txns"`
	Statements      []statement `json:"statements"`

Would it be better to make an array of transactions and each transaction has an array of stmts? That way it's more flexible and matches the response with multiple txns and each has multiple stmts.

THardy98

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @j82w and @knz)

pkg/server/api_v2_sql.go line 209 at r1 (raw file):

Previously, j82w (Jake) wrote…

Would it be better to make an array of transactions and each transaction has an array of stmts? That way it's more flexible and matches the response with multiple txns and each has multiple stmts.

I agree, but I chose this approach because its additive, non-breaking and this functionality is currently blocking some other UI work. Changing the request/response format means changing the sql api UI-side and its corresponding usages.

pkg/server/api_v2_sql.go line 377 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

I feel there is a lot of code duplication here.

Can you perhaps simplify the control flow a bit? Something like this:
knz@62099b2

Yes that's a much better idea. I've refactored to follow this instead, much smaller code diff.

knz

Reviewed 1 of 3 files at r1, 1 of 2 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @j82w and @THardy98)

pkg/server/api_v2_sql.go line 208 at r2 (raw file):

		ApplicationName string `json:"application_name"`
		Execute         bool   `json:"execute"`
		SeparateTxns    bool   `json:"separate_txns"`

May I recommend you add another bool field StopOnError which decides whether or not to stop subsequent txns/stmts from executing when an error is encountered.

From experience, there are valid use cases for both.

j82w · 2023-08-10T17:36:04Z

pkg/server/api_v2_sql.go line 209 at r1 (raw file):

Previously, THardy98 (Thomas Hardy) wrote…

I agree, but I chose this approach because its additive, non-breaking and this functionality is currently blocking some other UI work. Changing the request/response format means changing the sql api UI-side and its corresponding usages.

We could keep the current stmts and just add the txns property. Then in the db just treat the old stmts array as first txn.

THardy98

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @j82w and @knz)

pkg/server/api_v2_sql.go line 209 at r1 (raw file):

Previously, j82w (Jake) wrote…

We could keep the current stmts and just add the txns property. Then in the db just treat the old stmts array as first txn.

Yea that could work. I'm still a bit hesitant with this because we'd have to change the txnResult shape, or more safely, introduce a new field in execResult to hold txns that return multiple statement results, which weirdly would hold 2 fields for transaction results (one for single stmt, one for multi-stmt). WDYT? Is that fine?

j82w · 2023-08-10T18:08:11Z

pkg/server/api_v2_sql.go line 209 at r1 (raw file):

Previously, THardy98 (Thomas Hardy) wrote…

Yea that could work. I'm still a bit hesitant with this because we'd have to change the txnResult shape, or more safely, introduce a new field in execResult to hold txns that return multiple statement results, which weirdly would hold 2 fields for transaction results (one for single stmt, one for multi-stmt). WDYT? Is that fine?

nvm, I thought txnResults already had support for multiple statements, but it's just poorly named. It should be labeled stmtResult.

THardy98

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @j82w and @knz)

pkg/server/api_v2_sql.go line 208 at r2 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

May I recommend you add another bool field StopOnError which decides whether or not to stop subsequent txns/stmts from executing when an error is encountered.

From experience, there are valid use cases for both.

Added.

j82w · 2023-08-10T20:35:06Z

pkg/server/api_v2_sql.go line 400 at r3 (raw file):

				// If StopOnError is specified, return the outerErr and terminate
				if requestPayload.StopOnError {
					return outerErr, true

Does the stmtErr need to be combined with the outerErr?

THardy98

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @j82w and @knz)

pkg/server/api_v2_sql.go line 400 at r3 (raw file):

Previously, j82w (Jake) wrote…

Does the stmtErr need to be combined with the outerErr?

I decided for the outerErr to be a generic message when we're running separate txns. stmtErr in this case is captured only at the stmt level. My thinking is that separate txns = separate errors, no need to capture it again at the top-level response error. I initially thought about combining stmtErr into outerErr but thought with an unbounded number of stmts, it can become hard to read and a bit redundant.

j82w · 2023-08-10T20:52:10Z

pkg/server/api_v2_sql.go line 400 at r3 (raw file):

Previously, THardy98 (Thomas Hardy) wrote…

I decided for the outerErr to be a generic message when we're running separate txns. stmtErr in this case is captured only at the stmt level. My thinking is that separate txns = separate errors, no need to capture it again at the top-level response error. I initially thought about combining stmtErr into outerErr but thought with an unbounded number of stmts, it can become hard to read and a bit redundant.

Does the ui show and/or log the inner stmt error? My concern is it only check if the txn fails and then stops. It doesn't bother checking the inner errors.

THardy98

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @j82w and @knz)

pkg/server/api_v2_sql.go line 400 at r3 (raw file):

Previously, j82w (Jake) wrote…

Does the ui show and/or log the inner stmt error? My concern is it only check if the txn fails and then stops. It doesn't bother checking the inner errors.

That's correct. I think this is an easy UI side change though. We can add a small change when we format responses to also log stmt-level errors. I do this in a follow-up PR handling stmt-level errors on the db page.

THardy98 · 2023-08-11T15:43:08Z

RFAL / any remaining feedback/thoughts?

knz

LGTM but you also need a review approval from your team.

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @j82w and @THardy98)

maryliag

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @j82w and @THardy98)

THardy98 · 2023-08-14T14:05:09Z

TYFR

THardy98 · 2023-08-14T14:05:13Z

bors r+

craig · 2023-08-14T14:49:29Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-08-14T15:18:19Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-08-14T16:31:49Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

knz · 2023-08-14T17:05:42Z

bors r-

The CI failure indicates a real problem.

craig · 2023-08-14T17:05:44Z

Canceled.

knz · 2023-08-14T17:06:08Z

knz · 2023-08-14T17:06:51Z

You should filter out the source part of the JSON error output. The references to the source code change too often.

THardy98 · 2023-08-16T00:36:27Z

You should filter out the source part of the JSON error output. The references to the source code change too often.

Done.

Part of: cockroachdb#102386 This change introduces a `separate_txns` field to the sql api endpoint. This field allows callers to run provided statements in separate transactions. Failures in separate transactions are scoped to their respective transaction and do not interfere with the execution of subsequent transactions. The exception to this case is if the max response size is exceeded. If the response size is exceeded in an earlier transaction, it will still exceed the limit in subsequent transactions. The response-level error using `separate_txns` is a generic message: `separate transaction payload encountered transaction error(s)`, indicating that an error has occurred in at least one of the transactions. We additionally add a `stop_on_error` field to allow callers to stop execution of subsequent txns (i.e. in the case where `separate_txns` is true). Release note: None

THardy98 · 2023-08-16T14:56:49Z

bors r+

craig · 2023-08-16T15:58:17Z

Build succeeded:

Bazel Essential CI (Cockroach)

THardy98 requested review from knz and a team August 9, 2023 15:02

THardy98 requested a review from a team as a code owner August 9, 2023 15:02

THardy98 force-pushed the sql_api_separate_txns branch 2 times, most recently from b1ce810 to 6a6eb1a Compare August 9, 2023 18:50

knz reviewed Aug 9, 2023

View reviewed changes

j82w reviewed Aug 10, 2023

View reviewed changes

THardy98 force-pushed the sql_api_separate_txns branch from 6a6eb1a to d116a38 Compare August 10, 2023 14:42

THardy98 commented Aug 10, 2023

View reviewed changes

THardy98 requested review from knz and j82w August 10, 2023 15:08

knz reviewed Aug 10, 2023

View reviewed changes

THardy98 commented Aug 10, 2023

View reviewed changes

THardy98 force-pushed the sql_api_separate_txns branch from d116a38 to e2e4448 Compare August 10, 2023 20:22

THardy98 commented Aug 10, 2023

View reviewed changes

THardy98 requested a review from knz August 10, 2023 20:22

THardy98 commented Aug 10, 2023

View reviewed changes

THardy98 added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Aug 11, 2023

knz approved these changes Aug 11, 2023

View reviewed changes

maryliag approved these changes Aug 14, 2023

View reviewed changes

THardy98 force-pushed the sql_api_separate_txns branch from e2e4448 to 629ecd6 Compare August 16, 2023 00:35

THardy98 force-pushed the sql_api_separate_txns branch from 629ecd6 to b1a4dce Compare August 16, 2023 12:51

craig bot merged commit 02cdbba into cockroachdb:master Aug 16, 2023

blathers-crl bot mentioned this pull request Aug 16, 2023

release-23.1: server: support separate transactions on sql api #108849

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: support separate transactions on sql api #108447

server: support separate transactions on sql api #108447

THardy98 commented Aug 9, 2023 •

edited

Loading

cockroach-teamcity commented Aug 9, 2023

knz left a comment

j82w left a comment

THardy98 left a comment

knz left a comment

j82w commented Aug 10, 2023

THardy98 left a comment

j82w commented Aug 10, 2023

THardy98 left a comment

j82w commented Aug 10, 2023

THardy98 left a comment

j82w commented Aug 10, 2023

THardy98 left a comment •

edited

Loading

THardy98 commented Aug 11, 2023

knz left a comment

maryliag left a comment

THardy98 commented Aug 14, 2023

THardy98 commented Aug 14, 2023

craig bot commented Aug 14, 2023

craig bot commented Aug 14, 2023

craig bot commented Aug 14, 2023

knz commented Aug 14, 2023

craig bot commented Aug 14, 2023

knz commented Aug 14, 2023

knz commented Aug 14, 2023

THardy98 commented Aug 16, 2023

THardy98 commented Aug 16, 2023

craig bot commented Aug 16, 2023

server: support separate transactions on sql api #108447

server: support separate transactions on sql api #108447

Conversation

THardy98 commented Aug 9, 2023 • edited Loading

cockroach-teamcity commented Aug 9, 2023

knz left a comment

Choose a reason for hiding this comment

j82w left a comment

Choose a reason for hiding this comment

THardy98 left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

j82w commented Aug 10, 2023

THardy98 left a comment

Choose a reason for hiding this comment

j82w commented Aug 10, 2023

THardy98 left a comment

Choose a reason for hiding this comment

j82w commented Aug 10, 2023

THardy98 left a comment

Choose a reason for hiding this comment

j82w commented Aug 10, 2023

THardy98 left a comment • edited Loading

Choose a reason for hiding this comment

THardy98 commented Aug 11, 2023

knz left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

THardy98 commented Aug 14, 2023

THardy98 commented Aug 14, 2023

craig bot commented Aug 14, 2023

craig bot commented Aug 14, 2023

craig bot commented Aug 14, 2023

knz commented Aug 14, 2023

craig bot commented Aug 14, 2023

knz commented Aug 14, 2023

knz commented Aug 14, 2023

THardy98 commented Aug 16, 2023

THardy98 commented Aug 16, 2023

craig bot commented Aug 16, 2023

THardy98 commented Aug 9, 2023 •

edited

Loading

THardy98 left a comment •

edited

Loading