sql: use global defaults for internal session data rather than zero-values by rafiss · Pull Request #101486 · cockroachdb/cockroach

rafiss · 2023-04-13T19:14:52Z

Release note: None

cockroach-teamcity · 2023-04-13T19:15:05Z

This change is

ecwall

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @benbardin, @mgartner, @rafiss, and @shermanCRL)

pkg/sql/internal.go line 56 at r4 (raw file):

// steps of background jobs and schema changes. Each session variable is
// initialized using the correct default value.
func NewInternalSessionData(

What makes this "internal"? Why is it instantiated differently from the user SessionData?

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @benbardin, @ecwall, @mgartner, and @shermanCRL)

pkg/sql/internal.go line 56 at r4 (raw file):

Previously, ecwall (Evan Wall) wrote…

What makes this "internal"? Why is it instantiated differently from the user SessionData?

there are two things that make it internal:

the opName is passed in to indicate which internal operation is being executed
a real user session has SessionArgs which come from the connection string parameters, and also includes defaults that are configured by ALTER ROLE username SET varname = .... we don't have this here.

ecwall

Reviewed 2 of 10 files at r1, 2 of 53 files at r5, 25 of 60 files at r6, 18 of 22 files at r7, 4 of 6 files at r8, 1 of 1 files at r9, 2 of 3 files at r10, 1 of 52 files at r11, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @benbardin, @mgartner, @rafiss, and @shermanCRL)

pkg/sql/internal.go line 57 at r11 (raw file):

// steps of background jobs and schema changes. Each session variable is
// initialized using the correct default value.
func NewInternalSessionData(

It looks like a few other params might be useful here like the setting database and the code I mentioned here https://reviewable.io/reviews/cockroachdb/cockroach/101486#-NTEXaA_8kM71fqLJX-O (feel free to do this in a separate ticket(s) since this PR already cleans up a lot).

pkg/sql/planner.go line 374 at r11 (raw file):

	sd.SessionData.UserProto = user.EncodeProto()
	sd.SessionData.Internal = true
	sd.SearchPath = sessiondata.DefaultSearchPathForUser(user)

newInternalPlanner looks like a constructor function so I would not expect it to also modify the input sd.

Can you extract the code modifying sd and call it before calling newInternalPlanner (maybe in a separate ticket)?

benbardin · 2023-04-17T14:21:54Z

pkg/ccl/streamingccl/streamproducer/replication_manager_test.go

 	var sessionSerialized []byte
 	tDB.QueryRow(t, "SELECT crdb_internal.serialize_session()").Scan(&sessionSerialized)
-	require.NoError(t, protoutil.Unmarshal(sessionSerialized, &sessionData))
+	require.NoError(t, protoutil.Unmarshal(sessionSerialized, &m))


Will this work on a mixed-version cluster?

yes, this will be fine, since adding new fields to a proto is backwards compatibile

chengxiong-ruan

very nice!

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @benbardin, @ecwall, @mgartner, and @shermanCRL)

pkg/sql/planner.go line 374 at r11 (raw file):

Previously, ecwall (Evan Wall) wrote…

newInternalPlanner looks like a constructor function so I would not expect it to also modify the input sd.

Can you extract the code modifying sd and call it before calling newInternalPlanner (maybe in a separate ticket)?

i added a call to clone so the input isn't modified

Release note: None

After changing the sessiondata to stop using zero-value defaults, a test started failing. It works when using a zero sessiondata, which deserves more investigation. Release note: None

Prevent zero-values from being used for session vars when creating an InternalDB. Release note: None

Now that the internal executor is using a non-zero session data, that means it uses the declarative schema changer by default. The upgrade manager had a few assumptions in it that were specific to the old schema changer: - The PGAttributeNum field should only be set if it differs from the column descriptor ID. - Check the DeclarativeSchemaChangerState when waiting for migrations to finish. Release note: None

rafiss · 2023-04-29T13:02:08Z

tftr!

bors r=chengxiong-ruan

craig · 2023-04-29T13:31:52Z

Build succeeded:

Bazel Essential CI (Cockroach)

Since cockroachdb#101486, the internal executor has used global defaults for session settings. This effectively enabled `optimizer_use_histograms` for the internal executor, which was disabled before. This caused a huge performance regression. The root cause of the regression is not yet understood. This commit disables `optimizer_use_histograms` as a temporary solution for the performance regression. Informs cockroachdb#102954 Release note: None

104443: sql: disable histogram usage for internal executor r=mgartner a=mgartner Since #101486, the internal executor has used global defaults for session settings. This effectively enabled `optimizer_use_histograms` for the internal executor, which was disabled before. This caused a huge performance regression. The root cause of the regression is not yet understood. This commit disables `optimizer_use_histograms` as a temporary solution for the performance regression. Informs #102954 Epic: None Release note: None Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>

108947: sql: fix sql compaction job full scan r=j82w a=j82w The sql-stats-compaction is failing with TransactionRetryError. This is caused by the internal executor uses the zero-values for the settings, rather than the cluster defaults. This causes `SET reorder_joins_limit = 0;` which then causes the `sql-stats-compaction` delete statement to do a full scan. The full scan is causing the query to take a long time causing other queries to conflict with it. Error: `TransactionRetryWithProtoRefreshError: TransactionRetryError: retry txn (RETRY_SERIALIZABLE - failed preemptive refresh due to a conflict: committed value on key` The fix to use the correct default value instead of 0 is made in #101486. Solution is to change the query to avoid the join and thus the full scan. Fixes: #108936 Release note (sql change): Optimized the sql-stats-compaction job delete query to avoid full scan. This helps avoid the TransactionRetryError which can cause the job to fail. Co-authored-by: j82w <jwilley@cockroachlabs.com>

rafiss requested a review from ecwall April 13, 2023 19:14

rafiss requested a review from a team as a code owner April 13, 2023 19:14

rafiss requested a review from a team April 13, 2023 19:14

rafiss requested a review from a team as a code owner April 13, 2023 19:14

rafiss requested a review from mgartner April 13, 2023 19:14

rafiss force-pushed the fix-zero-values branch from 1162c2e to bed4c1a Compare April 13, 2023 19:20

rafiss requested review from a team as code owners April 13, 2023 20:31

rafiss requested a review from a team April 13, 2023 20:31

rafiss requested review from a team as code owners April 13, 2023 20:31

rafiss requested review from a team, benbardin and shermanCRL and removed request for a team April 13, 2023 20:31

rafiss force-pushed the fix-zero-values branch from 5a964ae to 03b12de Compare April 13, 2023 20:55

ecwall reviewed Apr 13, 2023

View reviewed changes

rafiss commented Apr 14, 2023

View reviewed changes

rafiss force-pushed the fix-zero-values branch 7 times, most recently from b03249d to 236414b Compare April 15, 2023 06:06

ecwall reviewed Apr 17, 2023

View reviewed changes

benbardin reviewed Apr 17, 2023

View reviewed changes

rafiss force-pushed the fix-zero-values branch from f17ddb5 to b8a7495 Compare April 28, 2023 21:17

chengxiong-ruan approved these changes Apr 28, 2023

View reviewed changes

rafiss commented Apr 29, 2023

View reviewed changes

rafiss added 6 commits April 29, 2023 09:01

*: change newInternalPlanner so it can use non-zero session data

608f03a

Release note: None

sql: rename NewFakeSessionData to NewInternalSessionData

2add0f8

Release note: None

changefeedccl: use default sessiondata instead zero sessiondata

51ff3a7

Release note: None

sql: add a TODO for sql-queries to investigate a test

2f03212

After changing the sessiondata to stop using zero-value defaults, a test started failing. It works when using a zero sessiondata, which deserves more investigation. Release note: None

sql: use NewInternalSessionData for InternalDB

fe816b6

Prevent zero-values from being used for session vars when creating an InternalDB. Release note: None

rafiss force-pushed the fix-zero-values branch from b8a7495 to 7409dab Compare April 29, 2023 13:01

craig bot merged commit 1ee30a1 into cockroachdb:master Apr 29, 2023

smg260 mentioned this pull request May 9, 2023

roachtest: 2023/04/30 kv0/nodes=3/cpu=96 regression [optimizer_use_histograms in internal executor] #102954

Open

This was referenced May 12, 2023

sql/importer: TestImportWorkerFailure failed #102839

Closed

logictest: race when updating DistSQL span resolver #99107

Closed

rafiss deleted the fix-zero-values branch May 16, 2023 16:40

mgartner mentioned this pull request Jun 6, 2023

sql: disable histogram usage for internal executor #104443

Merged

ecwall mentioned this pull request Aug 4, 2023

release-23.1: sql: fix CREATE AS [SHOW CREATE FUNCTION] job failure #108161

Closed

This was referenced Aug 17, 2023

sql-stats-compaction: failing with TransactionRetryError #108936

Closed

sql: fix sql compaction job full scan #108947

Merged

This was referenced Aug 18, 2023

release-23.1: sql: fix sql compaction job full scan #108987

Merged

release-23.1.9-rc: sql: fix sql compaction job full scan #108990

Merged

yuzefovich mentioned this pull request Apr 17, 2024

sql: add a way of changing session variables for internal executor #122542

Closed

annrpom mentioned this pull request Jul 8, 2024

server: background job inheriting from cluster setting #126806

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: use global defaults for internal session data rather than zero-values#101486

sql: use global defaults for internal session data rather than zero-values#101486
craig[bot] merged 7 commits intocockroachdb:masterfrom
rafiss:fix-zero-values

rafiss commented Apr 13, 2023

Uh oh!

cockroach-teamcity commented Apr 13, 2023

Uh oh!

ecwall left a comment

Uh oh!

rafiss left a comment

Uh oh!

ecwall left a comment

Uh oh!

benbardin Apr 17, 2023

Uh oh!

rafiss Apr 29, 2023

Uh oh!

chengxiong-ruan left a comment

Uh oh!

rafiss left a comment

Uh oh!

rafiss commented Apr 29, 2023

Uh oh!

craig bot commented Apr 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

rafiss commented Apr 13, 2023

Uh oh!

cockroach-teamcity commented Apr 13, 2023

Uh oh!

ecwall left a comment

Choose a reason for hiding this comment

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

ecwall left a comment

Choose a reason for hiding this comment

Uh oh!

benbardin Apr 17, 2023

Choose a reason for hiding this comment

Uh oh!

rafiss Apr 29, 2023

Choose a reason for hiding this comment

Uh oh!

chengxiong-ruan left a comment

Choose a reason for hiding this comment

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

rafiss commented Apr 29, 2023

Uh oh!

craig bot commented Apr 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants