sql: internal executor doesn't have session init'd properly and is just using go defaults #70888

cucaroach · 2021-09-29T21:34:48Z

The session data used by the internal executor looks like all the session data defaults are just the go value defaults:

SessionData = {github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb.SessionData} 
 Database = {string} ""
 ApplicationName = {string} "$ internal-claim-jobs"
 UserProto = {github.com/cockroachdb/cockroach/pkg/security.SQLUsernameProto} "root"
 DataConversionConfig = {github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb.DataConversionConfig} 
 VectorizeMode = {github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb.VectorizeExecMode} VectorizeUnset (0)
 TestingVectorizeInjectPanics = {bool} false
 DefaultIntSize = {int32} 0
 Location = {string} ""
 SearchPath = {[]string} nil
 TemporarySchemaName = {string} ""
 SeqState = {github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb.SequenceState} 
 WorkMemLimit = {int64} 0
 IntervalStyleEnabled = {bool} false
 DateStyleEnabled = {bool} false
 LockTimeout = {time.Duration} 0 0s
 Internal = {bool} true
 OnUpdateRehomeRowEnabled = {bool} false
LocalOnlySessionData = {github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb.LocalOnlySessionData} 
 SaveTablesPrefix = {string} ""
 OptimizerFKCascadesLimit = {int64} 0
 StmtTimeout = {time.Duration} 0 0s
 IdleInSessionTimeout = {time.Duration} 0 0s
 IdleInTransactionSessionTimeout = {time.Duration} 0 0s
 NoticeDisplaySeverity = {uint32} 0
 ReorderJoinsLimit = {int64} 0
 DefaultTxnPriority = {int64} 0
 DefaultTxnReadOnly = {bool} false
 DefaultTxnUseFollowerReads = {bool} false
 PartiallyDistributedPlansDisabled = {bool} false
 OptimizerUseHistograms = {bool} false
 OptimizerUseMultiColStats = {bool} false
 LocalityOptimizedSearch = {bool} false
 SafeUpdates = {bool} false
 PreferLookupJoinsForFKs = {bool} false
 ZigzagJoinEnabled = {bool} false
 RequireExplicitPrimaryKeys = {bool} false
 ForceSavepointRestart = {bool} false
 AllowPrepareAsOptPlan = {bool} false
 TempTablesEnabled = {bool} false
 ImplicitColumnPartitioningEnabled = {bool} false
 OverrideMultiRegionZoneConfigEnabled = {bool} false
 HashShardedIndexesEnabled = {bool} false
 DisallowFullTableScans = {bool} false
 ImplicitSelectForUpdate = {bool} false
 InsertFastPath = {bool} false
 AlterColumnTypeGeneralEnabled = {bool} false
 SynchronousCommit = {bool} false
 EnableSeqScan = {bool} false
 EnableUniqueWithoutIndexConstraints = {bool} false
 StubCatalogTablesEnabled = {bool} false
 ExperimentalComputedColumnRewrites = {string} ""
 CopyPartitioningWhenDeinterleavingTable = {bool} false
 EnableStreamReplication = {bool} false
 ResultsBufferSize = {int64} 0
 PropagateInputOrdering = {bool} false
 ExperimentalDistSQLPlanningMode = {github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb.ExperimentalDistSQLPlanningMode} ExperimentalDistSQLPlanningOff (0)
 DistSQLMode = {github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb.DistSQLExecMode} DistSQLOff (0)
 SerialNormalizationMode = {github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb.SerialNormalizationMode} SerialUsesRowID (0)
 NewSchemaChangerMode = {github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb.NewSchemaChangerMode} UseNewSchemaChangerOff (0)
 SequenceCache = {github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb.SequenceCache} nil
 PlacementEnabled = {bool} false
 SessionUserProto = {github.com/cockroachdb/cockroach/pkg/security.SQLUsernameProto} ""
 TxnRowsWrittenLog = {int64} 0
 TxnRowsWrittenErr = {int64} 0
 TxnRowsReadLog = {int64} 0
 TxnRowsReadErr = {int64} 0
 AutoRehomingEnabled = {bool} false
 IsSuperuser = {bool} false
 LargeFullScanRows = {float64} 0

Is this correct? Shouldn't we init some values to their cluster setting defaults? Thinking about EnableZigzagJoin, InsertFastPath, DistSQLMode, VectorizeMode, WorkMemLimit (looks like we interpret 0 as Default, ie 64MB here). Worried about internal queries running slowly because they aren't using all our chops.

Jira issue: CRDB-10271

The text was updated successfully, but these errors were encountered:

ajwerner · 2021-09-30T02:28:31Z

What's the context in which this internal executor is being executed? You're correct that the global one hanging off the execCfg doesn't have any session data set. This came up recently when I was debugging something.

The values from

cockroach/pkg/sql/internal.go

Lines 178 to 179 in d88ece5

    
           sds := sessiondata.NewStack(sd) 
        
           sdMutIterator := ie.s.makeSessionDataMutatorIterator(sds, nil /* sessionDefaults */)

If you use the InternalExecutor hanging of off the evalCtx it should share session settings with the associated connection. Honestly this behavior is also sort of surprising as that's the only state that's really shared between the internal executor and the session. I hate that. See #69495 for a different internal executor problem about things not being associated tightly enough with the session.

cucaroach · 2021-09-30T11:49:03Z

What's the context in which this internal executor is being executed?

I didn't catalog them all but I think it was mostly jobs related?

ajwerner · 2021-09-30T12:05:16Z

What's the context in which this internal executor is being executed?

I didn't catalog them all but I think it was mostly jobs related?

Yeah, I jobs were not doing anything with session state. My guess is that we should rework the "default" session state for the global internal executor. I think it's a pretty small change but I suspect there'll be some surprising fallout.

cucaroach · 2021-09-30T12:45:59Z

To be exact I was going around in circles trying to figure out why EnableZigzagJoin was false. This is purely about initializing sessions with the correct default state which isn't necessarily the same as Go default state which seems like an accidental oversight.

ajwerner · 2021-09-30T12:57:15Z

To be exact I was going around in circles trying to figure out why EnableZigzagJoin was false. This is purely about initializing sessions with the correct default state which isn't necessarily the same as Go default state which seems like an accidental oversight.

Right, agreed. I ran into this the other day too doing something similar (slack thread). Very painful.

I'm just saying the fix is to change the logic here. where we construct the session state if it's not provided (linked above and again below). I speculate that it'll break something subtle that's relying on these zero values.

cockroach/pkg/sql/internal.go

Lines 178 to 179 in d88ece5

    
           sds := sessiondata.NewStack(sd) 
        
           sdMutIterator := ie.s.makeSessionDataMutatorIterator(sds, nil /* sessionDefaults */)

rafiss · 2021-11-24T03:35:13Z

I think #71246 might fix this -- is that right @RichardJCai ?

RichardJCai · 2021-11-24T05:34:25Z

I think #71246 might fix this -- is that right @RichardJCai ?

Yeah I believe it should since we now have to pass session data to create the InternalExecutor.

cucaroach · 2021-11-24T11:20:40Z

Woot, thanks, I'll verify and close this when that PR lands.

rafiss · 2021-11-26T17:34:12Z

Hm nevermind, I might have been mistaken. There is now a InternalExecutorFactory as a field of ExecCfg, but there still is also the ExecCfg.InternalExecutor, which has ~158 usages and still has this issue. So I believe the work for this issue would be to move all those usages to use the InternalExecutorFactory instead.

vy-ton · 2022-05-23T20:39:45Z

@rafiss This came up in Queries triage/discussion. I wasn't sure if there were plans to tackle it with https://cockroachlabs.atlassian.net/browse/CRDB-14492?

ajwerner · 2022-05-23T20:44:46Z

cc @ZhouXing19 the (perhaps reluctantly) up and coming internal executor expert.

cucaroach · 2022-05-23T20:58:34Z

I gave this another go on Friday and didn't get super far but regardless happy to collab or handoff. Probably one of those things better to get into 22.2 early to weed out all the gremlins.

ZhouXing19 · 2022-05-23T21:04:23Z

I'll pick this up, maybe along with #80262, which is refactoring the internal executor initialization.

vy-ton · 2022-05-24T17:04:49Z

@ZhouXing19 When you take a look here, could you compile a list of settings where the go default differs from the CockroachDB default?

RichardJCai · 2022-05-24T17:08:49Z

@ZhouXing19 When you take a look here, could you compile a list of settings where the go default differs from the cluster default?

What's the reason for needing this list? Also I'm pretty sure the list just what Tommy listed in the body of the issue.

cucaroach · 2022-05-24T17:14:00Z

It would be nice to see where the go defaults and the cluster defaults diverge, it would be a much smaller list. The problem is not all internal executors are the same and there's specific overrides at play (like migration jobs opt out of distsql). The crime is things like InsertFastPath are disabled.

RichardJCai · 2022-05-24T17:20:08Z

It would be nice to see where the go defaults and the cluster defaults diverge, it would be a much smaller list. The problem is not all internal executors are the same and there's specific overrides at play (like migration jobs opt out of distsql). The crime is things like InsertFastPath are disabled.

I think I was more concerned here since cluster defaults can be updated so all these values can diverge but I think in this thread we're actually talking about the CRDB "default" assuming it's unchanged by users. I see what you mean now.

cucaroach · 2022-05-24T17:54:32Z

Although you do bring up a good point about cluster settings that have side affects, like if sql.trace.txn.threshold is on should that apply to internal executors?

ZhouXing19 · 2022-05-24T20:27:55Z

@ZhouXing19 When you take a look here, could you compile a list of settings where the go default differs from the CockroachDB default?

Made a list for entries in SessionData and LocalOnlySessionData: https://docs.google.com/spreadsheets/d/1QgwM8qHJtRsPApy0LCHw_nPvg_xZi4VM1EwAiHCQ0V0/edit?usp=sharing
cc @vy-ton

…ecutor() This commit introduces two functions that allow users to run sql statements with an internal executor. We intend to limit the usage of a real internal executor only inside these functions, instead of free-floating or hanging off certain structs. In other words, we restrict the init of an internal executor. The motivation is that if an internal executor is used to run multiple sql statements in a txn manner, these executions are expected to use the same set of info (such as descriptor collections) among their conn executor. While this rule can be easily forgot by the user of internal executors. Hence we provide an interface that wraps the initialization of internal executors with the query executions, so that the users won't need to be worried. Informs: once all existing usages of the internal executors are replaced with the new interfaces proposed here, cockroachdb#70888 should be solved. Release note: None

…Executor() This commit introduces two functions that allow users to run sql statements with an internal executor. We intend to limit the usage of a real internal executor only inside these functions, instead of free-floating or hanging off certain structs. In other words, we restrict the init of an internal executor. The motivation is that if an internal executor is used to run multiple sql statements in a txn manner, these executions are expected to use the same set of info (such as descriptor collections) among their conn executor. While this rule can be easily forgot by the user of internal executors. Hence we provide an interface that wraps the initialization of internal executors with the query executions, so that the users won't need to be worried. Informs: once all existing usages of the internal executors are replaced with the new interfaces proposed here, cockroachdb#70888 should be solved. Release note: None

ajwerner · 2022-10-24T15:03:03Z

I keep stumbling over this. I added a new feature and I started with a cluster settings. Then I switched it to use a session variable and I almost thought I had done it right, but noticed that the CPU usage of background internal queries was still high. It's awkward because I don't know that I feel comfortable setting my session variable up as part of the "minimal session data".

cucaroach added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-queries SQL Queries Team labels Sep 29, 2021

RaduBerinde assigned cucaroach Oct 5, 2021

cucaroach mentioned this issue Dec 6, 2021

sql: rename setting disable_plan_gists to plan_gists_enabled #73348

Closed

blathers-crl bot added the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label May 23, 2022

exalate-issue-sync bot removed the T-sql-queries SQL Queries Team label May 23, 2022

ZhouXing19 assigned ZhouXing19 and unassigned cucaroach May 23, 2022

cucaroach mentioned this issue Aug 8, 2022

sql: introduce new internal executor interfaces #82477

Merged

cucaroach mentioned this issue Apr 6, 2023

sql: add unbounded_parallel_scans session variable #100773

Merged

ecwall assigned ecwall and unassigned ZhouXing19 Apr 13, 2023

rafiss mentioned this issue Apr 13, 2023

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

Merged

rafiss assigned rafiss and unassigned ecwall Apr 14, 2023

craig bot closed this as completed in 1ee30a1 Apr 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: internal executor doesn't have session init'd properly and is just using go defaults #70888

sql: internal executor doesn't have session init'd properly and is just using go defaults #70888

cucaroach commented Sep 29, 2021 •

edited by exalate-issue-sync bot

Loading

ajwerner commented Sep 30, 2021

cucaroach commented Sep 30, 2021

ajwerner commented Sep 30, 2021

cucaroach commented Sep 30, 2021

ajwerner commented Sep 30, 2021

rafiss commented Nov 24, 2021

RichardJCai commented Nov 24, 2021

cucaroach commented Nov 24, 2021

rafiss commented Nov 26, 2021

vy-ton commented May 23, 2022

ajwerner commented May 23, 2022

cucaroach commented May 23, 2022

ZhouXing19 commented May 23, 2022

vy-ton commented May 24, 2022 •

edited

Loading

RichardJCai commented May 24, 2022

cucaroach commented May 24, 2022

RichardJCai commented May 24, 2022

cucaroach commented May 24, 2022

ZhouXing19 commented May 24, 2022

ajwerner commented Oct 24, 2022

sql: internal executor doesn't have session init'd properly and is just using go defaults #70888

sql: internal executor doesn't have session init'd properly and is just using go defaults #70888

Comments

cucaroach commented Sep 29, 2021 • edited by exalate-issue-sync bot Loading

ajwerner commented Sep 30, 2021

cucaroach commented Sep 30, 2021

ajwerner commented Sep 30, 2021

cucaroach commented Sep 30, 2021

ajwerner commented Sep 30, 2021

rafiss commented Nov 24, 2021

RichardJCai commented Nov 24, 2021

cucaroach commented Nov 24, 2021

rafiss commented Nov 26, 2021

vy-ton commented May 23, 2022

ajwerner commented May 23, 2022

cucaroach commented May 23, 2022

ZhouXing19 commented May 23, 2022

vy-ton commented May 24, 2022 • edited Loading

RichardJCai commented May 24, 2022

cucaroach commented May 24, 2022

RichardJCai commented May 24, 2022

cucaroach commented May 24, 2022

ZhouXing19 commented May 24, 2022

ajwerner commented Oct 24, 2022

cucaroach commented Sep 29, 2021 •

edited by exalate-issue-sync bot

Loading

vy-ton commented May 24, 2022 •

edited

Loading