sql: use global defaults for internal session data rather than zero-values #101486
Conversation
|
This change is |
rafiss
requested review from
a team,
benbardin and
shermanCRL
and removed request for
a team
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Reviewable status:
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
opNameis passed in to indicate which internal operation is being executed - a real user session has
SessionArgswhich come from the connection string parameters, and also includes defaults that are configured byALTER ROLE username SET varname = .... we don't have this here.
rafiss
force-pushed
the
fix-zero-values
branch
7 times, most recently
from
b03249d to
236414b
Compare
There was a problem hiding this comment.
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:
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)?
| 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)) |
There was a problem hiding this comment.
Will this work on a mixed-version cluster?
There was a problem hiding this comment.
yes, this will be fine, since adding new fields to a proto is backwards compatibile
There was a problem hiding this comment.
Reviewable status:
pkg/sql/planner.go line 374 at r11 (raw file):
Previously, ecwall (Evan Wall) wrote…
newInternalPlannerlooks like a constructor function so I would not expect it to also modify the inputsd.Can you extract the code modifying
sdand call it before callingnewInternalPlanner(maybe in a separate ticket)?
i added a call to clone so the input isn't modified
Release note: None
Release note: None
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
|
tftr! bors r=chengxiong-ruan |
|
Build succeeded: |
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 <[email protected]>
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 <[email protected]>
fixes #70888
Release note: None