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: improve inner plan leaf transaction tracking #112498

Merged

Conversation

rharding6373
Copy link
Collaborator

This PR changes planner's innerPlansMustUseLeafTxn from being treated as a boolean to a counter instead. As a result, the contract with getFinishedSetupFn has changed so that the returned finishedSetupFn and cleanup functions should only be called once.

This change cleans up how we handle leaf transactions when running a plan inside a plan, e.g., executing UDFs.

Epic: none
Informs: #111097

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rharding6373 rharding6373 marked this pull request as ready for review October 17, 2023 15:43
@rharding6373 rharding6373 requested a review from a team as a code owner October 17, 2023 15:43
Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball and @rharding6373)


pkg/sql/apply_join.go line 329 at r1 (raw file):

			&subqueryResultMemAcc,
			false, /* skipDistSQLDiagramGeneration */
			atomic.LoadInt32(&params.p.atomic.innerPlansMustUseLeafTxn) == 1,

This now should be >= 1. Perhaps we should introduce a helper method for this?


pkg/sql/apply_join.go line 356 at r1 (raw file):

	planCtx := execCfg.DistSQLPlanner.NewPlanningCtx(ctx, evalCtx, &plannerCopy, plannerCopy.txn, distributeType)
	planCtx.stmtType = recv.stmtType
	planCtx.mustUseLeafTxn = atomic.LoadInt32(&params.p.atomic.innerPlansMustUseLeafTxn) == 1

ditto


pkg/sql/distsql_running.go line 2028 at r1 (raw file):

		// We already called finishedSetupFn in the previous call to Run, since we
		// only got here if we got a distributed error, not an error during setup.
		dsp.Run(ctx, localPlanCtx, txn, localPhysPlan, recv, evalCtx, nil)

nit: add /* finishedSetupFn */.


pkg/sql/planner.go line 184 at r1 (raw file):

	atomic struct {
		// innerPlansMustUseLeafTxn is set to 1 if the "outer" plan is using

nit: update the comment to indicate that it's a counter.


pkg/sql/logictest/testdata/logic_test/udf_fk line 490 at r1 (raw file):


statement ok
CREATE TABLE IF NOT EXISTS parent_cascade (p INT PRIMARY KEY);

nit: is this added so that apply_join subtest can be run?

@rharding6373 rharding6373 force-pushed the 20231011_udf_concurrent_txn_pt2_111097 branch from 15a91e9 to e08d767 Compare October 17, 2023 21:18
Copy link
Collaborator Author

@rharding6373 rharding6373 left a comment

Choose a reason for hiding this comment

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

TFTR!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball and @yuzefovich)


pkg/sql/apply_join.go line 329 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

This now should be >= 1. Perhaps we should introduce a helper method for this?

Thanks for catching this oversight. Done.


pkg/sql/apply_join.go line 356 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

ditto

Done.


pkg/sql/distsql_running.go line 2028 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: add /* finishedSetupFn */.

Done.


pkg/sql/planner.go line 184 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: update the comment to indicate that it's a counter.

Done


pkg/sql/logictest/testdata/logic_test/udf_fk line 490 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: is this added so that apply_join subtest can be run?

Yes. Before this change when run alone the subtest would fail.

Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball and @rharding6373)


pkg/sql/apply_join.go line 25 at r2 (raw file):

	"github.com/cockroachdb/errors"
	"github.com/cockroachdb/redact"
	"strconv"

nit: this seems out of order now.

@rharding6373 rharding6373 force-pushed the 20231011_udf_concurrent_txn_pt2_111097 branch 2 times, most recently from f13cdf4 to c38e258 Compare October 18, 2023 02:44
@rharding6373 rharding6373 added the backport-23.2.x Flags PRs that need to be backported to 23.2. label Oct 18, 2023
@rharding6373
Copy link
Collaborator Author

Fixing one more formatting issue. The TestStreamingRegionalConstraint failure is from a recently unskipped test (known issue: #112541).

@rharding6373 rharding6373 force-pushed the 20231011_udf_concurrent_txn_pt2_111097 branch from c38e258 to 0899896 Compare October 18, 2023 04:39
This PR changes planner's `innerPlansMustUseLeafTxn` from being treated
as a boolean to a counter instead. As a result, the contract with
`getFinishedSetupFn` has changed so that the returned `finishedSetupFn`
and `cleanup` functions should only be called once.

This change cleans up how we handle leaf transactions when running a
plan inside a plan, e.g., executing UDFs.

Epic: none
Informs: cockroachdb#111097

Release note: None
@rharding6373 rharding6373 force-pushed the 20231011_udf_concurrent_txn_pt2_111097 branch from 0899896 to 4d92d85 Compare October 18, 2023 17:27
@rharding6373
Copy link
Collaborator Author

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Oct 18, 2023

Build succeeded:

@craig craig bot merged commit a4581e2 into cockroachdb:master Oct 18, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.2.x Flags PRs that need to be backported to 23.2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants