backfill: override statement_timeout to 0 in VERIFY INDEX job

[annrpom]

Currently, for index validation jobs kicked off in schema changes like CREATE INDEX, we inherit the session variables used by the internal executor from the global default (i.e. whichever value is set by the cluster setting). This means that we can land in situations like the following:

root@localhost:29000/movr> SET CLUSTER SETTING sql.defaults.statement_timeout = '5ms';                                             
NOTICE: setting global default sql.defaults.statement_timeout is not recommended
HINT: use the `ALTER ROLE ... SET` syntax to control session variable defaults at a finer-grained level. See: https://www.cockroachlabs.com/docs/v23.2/alter-role.html#set-default-session-variable-values-for-a-role
SET CLUSTER SETTING

Time: 3ms total (execution 2ms / network 0ms)

root@localhost:29000/movr> SET statement_timeout = '0s';                                                                           
SET

Time: 0ms total (execution 0ms / network 0ms)

root@localhost:29000/movr> SHOW statement_timeout;                                                                                 
  statement_timeout
---------------------
  0
(1 row)

Time: 0ms total (execution 0ms / network 0ms)

root@localhost:29000/movr> CREATE INDEX test on public.users (credit_card, id);                                                    
ERROR: job 980684294138527745 could not be loaded: failed to get payload for job 980684294138527745: job-info-get: query execution canceled due to statement timeout
SQLSTATE: 57014
HINT: The job may not have succeeded.

Jobs kicked off by schema changes should not need to respect this timeout -- which was the behavior pre-23.2.

We can set a SessionDataOverride that overrides statement_timeout to '0s' for this job.

Jira issue: CRDB-39824

Epic CRDB-40419

[annrpom]

Seen https://github.com/cockroachlabs/support/issues/2998

[annrpom]

As a follow-up, we should ensure other jobs kicked off by schema changes also get this override.

To tag on, we might want to audit if other session variables would land us in a similar spot.

[annrpom]

Actually, we can just make all background jobs ignore the statement timeout in here

cockroach/pkg/sql/internal.go

Lines 1131 to 1150 in b9680b4

	var sd *sessiondata.SessionData
	if ie.sessionDataStack != nil {
	// TODO(andrei): Properly clone (deep copy) ie.sessionData.
	sd = ie.sessionDataStack.Top().Clone()
	} else {
	sd = NewInternalSessionData(context.Background(), ie.s.cfg.Settings, "" /* opName */)
	}
	if globalOverride := ieMultiOverride.Get(&ie.s.cfg.Settings.SV); globalOverride != "" {
	globalOverride = strings.TrimSpace(globalOverride)
	// Prepend the "global" setting overrides to ensure that caller's
	// overrides take precedence.
	if localOverride := sessionDataOverride.MultiOverride; localOverride != "" {
	sessionDataOverride.MultiOverride = globalOverride + "," + localOverride
	} else {
	sessionDataOverride.MultiOverride = globalOverride
	}
	}
	applyInternalExecutorSessionExceptions(sd)
	applyOverrides(sessionDataOverride, sd)

ref: https://cockroachlabs.slack.com/archives/C04N0AS14CT/p1719416225444619?thread_ts=1719410054.255929&cid=C04N0AS14CT

[rafiss]

I think this diff might be what we want. We could update the comment on NewInternalSessionData to indicate this.

diff --git a/pkg/sql/internal.go b/pkg/sql/internal.go
index 24685b16219..cfb161132b4 100644
--- a/pkg/sql/internal.go
+++ b/pkg/sql/internal.go
@@ -95,6 +95,7 @@ func NewInternalSessionData(
 	sd.SearchPath = sessiondata.DefaultSearchPathForUser(username.NodeUserName())
 	sd.SequenceState = sessiondata.NewSequenceState()
 	sd.Location = time.UTC
+	sd.StmtTimeout = 0
 	return sd
 }

[rafiss]

For people who are affected by this right now, we can recommend that they configure the default statement_timeout using ALTER ROLE ALL SET statement_timeout = ... rather than the sql.defaults.statement_timeout cluster setting.

[annrpom]

i'll leave this open until i take care of my backports

[annrpom]

Re-opening as per https://cockroachlabs.slack.com/archives/C0168LW5THS/p1723150249619579

[annrpom]

Tested the original idea proprosed #126261 (comment) with roachprod + 5 min of the tpcc workload running:

root@localhost:26257/defaultdb> SET CLUSTER SETTING sql.defaults.statement_timeout = '5ms';                          
NOTICE: setting global default sql.defaults.statement_timeout is not recommended
HINT: use the `ALTER ROLE ... SET` syntax to control session variable defaults at a finer-grained level. See: https://www.cockroachlabs.com/docs/dev/alter-role.html#set-default-session-variable-values-for-a-role
SET CLUSTER SETTING

Time: 3ms total (execution 3ms / network 0ms)

root@localhost:26257/defaultdb> SET statement_timeout = '0s';                                                        
SET

Time: 0ms total (execution 0ms / network 0ms)

root@localhost:26257/tpcc> ALTER TABLE order_line ADD COLUMN updated_at_test TIMESTAMP NOT NULL DEFAULT now();       
ALTER TABLE

Time: 46.847s total (execution 46.847s / network 0.000s)

-- even smaller timeout
root@localhost:26257/tpcc> SET CLUSTER SETTING sql.defaults.statement_timeout = '1ms';                               
NOTICE: setting global default sql.defaults.statement_timeout is not recommended
HINT: use the `ALTER ROLE ... SET` syntax to control session variable defaults at a finer-grained level. See: https://www.cockroachlabs.com/docs/dev/alter-role.html#set-default-session-variable-values-for-a-role
SET CLUSTER SETTING

Time: 3ms total (execution 3ms / network 0ms)

root@localhost:26257/tpcc> ALTER TABLE order_line ADD COLUMN test2 int NOT NULL DEFAULT 12;                          
ALTER TABLE

Time: 51.388s total (execution 51.388s / network 0.000s)

We will go with this approach. The main concern of doing this in the first place was that this would also affect non-"internal" queries (specifically: Yahor brought up the concern of how this would affect the cloud sql shell, as it uses the run-query-via-api). We think that this result is OK as the api itself has a specified request timeout already:

cockroach/pkg/server/api_v2_sql.go

Lines 303 to 310 in 281b6e7

	if requestPayload.Timeout == "" {
	requestPayload.Timeout = "5s"
	}
	timeout, err := time.ParseDuration(requestPayload.Timeout)
	if err != nil {
	topLevelError(err, http.StatusBadRequest)
	return
	}

[blathers-crl]

Based on the specified backports for linked PR #129489, I applied the following new label(s) to this issue: branch-release-24.2. Please adjust the labels as needed to match the branches actually affected by this issue, including adding any known older branches.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}