sql: improve env collection in stmt bundles #123794
Conversation
|
This change is |
yuzefovich
force-pushed
the
bundle-env
branch
3 times, most recently
from
accfdad to
d1f5cef
Compare
yuzefovich
changed the title
yuzefovich
force-pushed
the
bundle-env
branch
2 times, most recently
from
39d91fc to
bc5fdf5
Compare
yuzefovich
added
backport-23.2.x
There was a problem hiding this comment.
Thank you for doing this! And nice fixes! I have some suggestions below but you can take or leave them.
Reviewed 2 of 2 files at r1, 1 of 1 files at r2, 2 of 2 files at r3, 5 of 5 files at r4, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)
-- commits line 62 at r4:
Would it be simpler to print SET statements for all writable session variables, regardless of whether they had a non-default value? Admittedly, these SET statements would usually be unnecessary for most session variables, but I don't think it would hurt and would get us closer to being able to run cockroach debug statement-bundle recreate without any manual edits.
pkg/sql/explain_bundle.go line 847 at r4 (raw file):
if gen.Set == nil && gen.RuntimeSet == nil && gen.SetWithPlanner == nil { // Skip all read-only variables. continue
Some of the read-only variables could be interesting for analysis even though they are not writable. For example, locality, results_buffer_size, node_id, session_id, session_user, transaction_priority, transaction_status might come in handy if we're trying to understand why the query executed in a certain way, even though they don't have Set (or GlobalDefault).
pkg/sql/explain_bundle.go line 954 at r4 (raw file):
} fmt.Fprintf( w, "-- %s = %s (default value: %s)\n",
Would it make sense to directly print SET CLUSTER SETTING %s = %s;\n here? (Seems like that would help if our goal is to run cockroach debug statement-bundle recreate without needing edits.)
pkg/sql/explain_bundle_test.go line 764 at r4 (raw file):
vars := strings.Split(sb.String(), "\n") for _, line := range vars { _, err := sqlDB.ExecContext(ctx, line)
Is this how you caught the bugs fixed in the previous commits? If so, nice job!
There was a problem hiding this comment.
Nice!
Reviewed 2 of 2 files at r1, 1 of 1 files at r2, 2 of 2 files at r3, 5 of 5 files at r4, all commit messages.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
Previously, michae2 (Michael Erickson) wrote…
Would it be simpler to print
SETstatements for all writable session variables, regardless of whether they had a non-default value? Admittedly, theseSETstatements would usually be unnecessary for most session variables, but I don't think it would hurt and would get us closer to being able to runcockroach debug statement-bundle recreatewithout any manual edits.
I also mention this in another comment (which I typed before this one), but one of my goals with this PR is reducing the amount of stuff we include into env.sql to only have useful information. This is because, at least personally, apart from using debug sb recreate I also look at env.sql directly, so having extra lines, even commented out, makes spotting important non-defaults harder. Can you expand on how having all writable variables would avoid / reduce manual editing?
Relatedly, I updated the PR to include necessary cluster setting statements, but there will still be some extra work needed to reconcile session variables and cluster settings in sb recreate command (with current approach, the go-to option will be connecting to the demo cluster from another shell and then copy-paste SET statements for session vars). My hope is that it'll be rare when we have many non-default values for both session vars and cluster settings, so in vast majority of bundles we'd either have no or little extra manual work.
Another idea I had for reconciling session vars and cluster settings would be to do a two-step modification of session vars. Namely:
- run
SETsession variable statements for non-default session vars. Memorize all of these statements - run
SET CLUSTER SETTINGstatements for non-default cluster settings. - collect all non-default session variable statements again. These should include those already executed in step 1. but should also include a new stmt for each one executed in step 2. Execute only new ones via
RESET session_var;.
This should work, right? Thoughts on whether it'd be worth it?
pkg/sql/explain_bundle.go line 847 at r4 (raw file):
Previously, michae2 (Michael Erickson) wrote…
Some of the read-only variables could be interesting for analysis even though they are not writable. For example,
locality,results_buffer_size,node_id,session_id,session_user,transaction_priority,transaction_statusmight come in handy if we're trying to understand why the query executed in a certain way, even though they don't haveSet(orGlobalDefault).
Hm, it's an interesting question I haven't really considered before.
I'd like for us to be conscious about what we include vs exclude. Having everything in env.sql, even commented out if matches the default or is read-only, at least for me creates more mental overhead when examining the bundle, so I think it's worth it to exclude things that we think are unlikely to be useful. At the moment there are 23 read-only session variables (and those that don't have GlobalDefault set are a subset of them):
crdb_version
integer_datetimes
is_superuser
lc_collate
lc_ctype
locality
max_connections
max_identifier_length
max_index_keys
node_id
results_buffer_size
server_encoding
server_version
server_version_num
session_authorization
session_id
session_user
ssl
system_identity
tracing
transaction_priority
transaction_status
virtual_cluster_name
Thoughts on which ones might be useful?
I looked at all of them and right now included is_superuser, locality, node_id, results_buffer_size, ssl, transaction_priority, transaction_status. crdb_version also is useful but it's already included separately. I didn't include session_id and session_user since I don't see how that can be useful - what scenario are you thinking about? Thoughts on what we should include vs exclude?
pkg/sql/explain_bundle.go line 954 at r4 (raw file):
Previously, michae2 (Michael Erickson) wrote…
Would it make sense to directly print
SET CLUSTER SETTING %s = %s;\nhere? (Seems like that would help if our goal is to runcockroach debug statement-bundle recreatewithout needing edits.)
The difficulty with that is it wouldn't apply to the current session that was already created on debug sb recreate command, so we'd need to connect to the demo from another shell for these to take effect. Then, if we connect from another shell we'd lose all the session variables that were SET during recreate.
Still, I do see the value of having this in copy-pastable SQL format, so I made the corresponding change.
pkg/sql/explain_bundle_test.go line 764 at r4 (raw file):
Previously, michae2 (Michael Erickson) wrote…
Is this how you caught the bugs fixed in the previous commits? If so, nice job!
Some of them - copy_num_retries_per_batch bug was caught via this test, but "cost scans" one was caught by the linter (it wasn't used anywhere except in the old env bundle implementation). This test is also how I generated the maps for which ones need quotes.
There was a problem hiding this comment.
Reviewable status:
Previously, yuzefovich (Yahor Yuzefovich) wrote…
I also mention this in another comment (which I typed before this one), but one of my goals with this PR is reducing the amount of stuff we include into
env.sqlto only have useful information. This is because, at least personally, apart from usingdebug sb recreateI also look atenv.sqldirectly, so having extra lines, even commented out, makes spotting important non-defaults harder. Can you expand on how having all writable variables would avoid / reduce manual editing?
Relatedly, I updated the PR to include necessary cluster setting statements, but there will still be some extra work needed to reconcile session variables and cluster settings in
sb recreatecommand (with current approach, the go-to option will be connecting to thedemocluster from another shell and then copy-pasteSETstatements for session vars). My hope is that it'll be rare when we have many non-default values for both session vars and cluster settings, so in vast majority of bundles we'd either have no or little extra manual work.Another idea I had for reconciling session vars and cluster settings would be to do a two-step modification of session vars. Namely:
- run
SETsession variable statements for non-default session vars. Memorize all of these statements- run
SET CLUSTER SETTINGstatements for non-default cluster settings.- collect all non-default session variable statements again. These should include those already executed in step 1. but should also include a new stmt for each one executed in step 2. Execute only new ones via
RESET session_var;.This should work, right? Thoughts on whether it'd be worth it?
Thinking more about this - I don't think this idea is really feasible. In order to be able to identify session variables that have non-default values because of the corresponding cluster settings we would need to spin up a fresh demo shell in step 2, during bundle collection. In other words, during bundle collection we would include SET CLUSTER SETTING statements into env.sql and then we'd also need to execute each statement against fresh demo to see which session variable, if any, changed; if there is a change, then we'd include RESET session_var statement right after the corresponding SET CLUSTER SETTING. We cannot use the internal executor against the cluster on which we're collecting the bundle because this cluster already has the settings set. Using non-session-bound internal executor (i.e. jobs-like) also wouldn't work. So I think we would have to spin up a fresh demo shell for each.
The only approach to address this I see is extending each session var so that in addition to (or instead of) GlobalDefault getter method we would have to explicitly specify the cluster setting object that provides the default for a particular session variable. Effectively, this would move "session var -> cluster setting" mapping (that was incomplete) from the bundle collection to session variable declaration. I'll take a look at this - perhaps it's not that much work.
There was a problem hiding this comment.
Reviewable status:
Can you expand on how having all writable variables would avoid / reduce manual editing?
I was thinking that if we have SET statements for all writable session vars, then executing all of them would be a lazy way to handle both (a) the case where the session var was set, and (b) the case where the session var came from a cluster setting that was set, without having to distinguish between the two cases.
one of my goals with this PR is reducing the amount of stuff we include into
env.sqlto only have useful information.
That is a fair point.
The only approach to address this I see is extending each session var so that in addition to (or instead of)
GlobalDefaultgetter method we would have to explicitly specify the cluster setting object that provides the default for a particular session variable. Effectively, this would move "session var -> cluster setting" mapping (that was incomplete) from the bundle collection to session variable declaration. I'll take a look at this - perhaps it's not that much work.
Is there some way to call GlobalDefault with the default settings.Values instead of the current settings.Values?
pkg/sql/explain_bundle.go line 847 at r4 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Hm, it's an interesting question I haven't really considered before.
I'd like for us to be conscious about what we include vs exclude. Having everything in
env.sql, even commented out if matches the default or is read-only, at least for me creates more mental overhead when examining the bundle, so I think it's worth it to exclude things that we think are unlikely to be useful. At the moment there are 23 read-only session variables (and those that don't haveGlobalDefaultset are a subset of them):crdb_version integer_datetimes is_superuser lc_collate lc_ctype locality max_connections max_identifier_length max_index_keys node_id results_buffer_size server_encoding server_version server_version_num session_authorization session_id session_user ssl system_identity tracing transaction_priority transaction_status virtual_cluster_nameThoughts on which ones might be useful?
I looked at all of them and right now included
is_superuser,locality,node_id,results_buffer_size,ssl,transaction_priority,transaction_status.crdb_versionalso is useful but it's already included separately. I didn't includesession_idandsession_usersince I don't see how that can be useful - what scenario are you thinking about? Thoughts on what we should include vs exclude?
Your list SGTM.
There was a problem hiding this comment.
Reviewable status:
I was thinking that if we have SET statements for all writable session vars, then executing all of them would be a lazy way to handle both (a) the case where the session var was set, and (b) the case where the session var came from a cluster setting that was set, without having to distinguish between the two cases.
Right, good point. For some reason, I missed this point since I was focused on reducing env.sql too much 😃
Is there some way to call
GlobalDefaultwith the defaultsettings.Valuesinstead of the currentsettings.Values?
That's brilliant! Indeed, I think we can just use a "fresh" values container to get "binary defaults". I updated the commit to include a session variable whenever its value differs either from the "binary default" or the "cluster default" (the binary default is used in the commented out part of the SET statement as before this patch). I used cluster.MakeTestingClusterSettings() for that "fresh" container which should work (although this is a non-test code, should I refactor this?).
yuzefovich
force-pushed
the
bundle-env
branch
2 times, most recently
from
841352b to
80a5892
Compare
There was a problem hiding this comment.
Reviewed 7 of 7 files at r9, 1 of 1 files at r10, 2 of 2 files at r11, 5 of 5 files at r12, all commit messages.
Reviewable status:
-- commits line 62 at r4:
Yay!
I used
cluster.MakeTestingClusterSettings()for that "fresh" container which should work (although this is a non-test code, should I refactor this?).
🤷♂️ seems ok to me. It's also used by genSettingsListCmd for example.
pkg/sql/explain_bundle.go line 958 at r12 (raw file):
// but aren't useful in stmt bundles. switch *setting { case "cluster.secret", "diagnostics.reporting.enabled", "version":
Maybe also skip enterprise.license?
pkg/sql/explain_bundle.go line 1175 at r12 (raw file):
"server_version_num": {}, "session_authorization": {}, "session_id": {},
session_id could help us match this bundle to a debug.zip taken at the same time. It might be worth keeping.
pkg/sql/explain_bundle.go line 1203 at r12 (raw file):
// clusterSettingNeedsQuotes contains all cluster settings that have values that // need single quotes around them in SET CLUSTER SETTING statements. var clusterSettingNeedsQuotes = map[string]struct{}{
It might be hard to maintain this list (and the one above). Would it work to put quotes around the values for all SET and SET CLUSTER SETTING statements?
We were incorrectly using the boolean getter for this integer variable. The bug was introduced when this variable was added in 1c1d313. Release note: None
This commit fixes an omission where `cost_scans_with_default_col_size` session variable didn't actually use the corresponding cluster setting for its default value. In other words, the cluster setting actually had no effect. The bug has been present since this variable was introduced in af32d9d. Release note: None
This commit adjusts global defaults for a few session variables to use the postgres style (so that the global default value and session variable value would be stored alike). Release note: None
This commit refactors how we collect session variables and cluster settings into `env.sql` file in the stmt bundle. Previously, we used a hard-coded list of session variables that were always included while ignoring all variables not in the list. If a variable had a value different from the "binary default" (meaning the value that you got right after the cluster startup, before any cluster setting modifications are applied), then it would be in a SET statement that would run on the `bundle recreate`; if the value was the same as "binary default", then the SET statement was commented out. Also, all cluster setting values were included into the file as a comment. This commit makes it so that we include all session variables and cluster settings that differ from their defaults. This allows us to recreate the environment while also reducing the overhead of including everything into `env.sql`. For session variables further clarification is warranted: - all read-only variables have been audited, and most are explicitly excluded. Out of 23 currently present session variables, only 9 were deemed to be possibly useful during investigations, so they are not excluded (one of 9 is `crdb_version` which we print out separately, so it's actually excluded too). - for writable variables, we include it if its value differs from the "binary" default (the one that you get on a fresh cluster) or from the "cluster" default (the one that you get on a fresh session on the current cluster). The latter kind is obtained via the provided `settings.Values` container whereas the former is obtained via a global singleton container. Additionally, this commit adjusts the SET and SET CLUSTER SETTING statements to have single quotes around the setting values that need them. For cluster settings all types work with having single quotes, but for session variables some (like integers) don't work with quotes while others (like strings) need them. This commit adds a map for those that need them as well as a simple test to catch some of the missing ones (the list might be incomplete given that the test exercises the default config). All values are adjusted to fit on a single line (we have some cluster settings that might span multiple lines). It also adjusts `EXPLAIN (OPT, ENV)` to include the list of cluster settings with non-default values. Release note: None
There was a problem hiding this comment.
Thanks for the reviews!
I think this change is a low risk and high enough reward to be backported, so I added the corresponding labels, but we can discuss merits of the backport on the backport PRs.
bors r+
Reviewable status:
pkg/sql/explain_bundle.go line 1175 at r12 (raw file):
Previously, michae2 (Michael Erickson) wrote…
session_idcould help us match this bundle to a debug.zip taken at the same time. It might be worth keeping.
Ack, let's keep it.
pkg/sql/explain_bundle.go line 1203 at r12 (raw file):
Previously, michae2 (Michael Erickson) wrote…
It might be hard to maintain this list (and the one above). Would it work to put quotes around the values for all
SETandSET CLUSTER SETTINGstatements?
Thanks for asking! I also thought of this when working on session variables, and for them it doesn't work (e.g. SET reorder_joins_limit = '1'; fails), but it seems to work for all cluster settings (which I assumed wouldn't work either so didn't try before), so I removed one of the maps 😄
In terms of maintaining this list, I hope that the newly added test should help with that, but it is indeed incomplete - some session variables have "" as their default which effectively by-passes the test. We'll need to adjust the list manually over time when we find variables that need quotes (I think it should be pretty rare - this would mean that we got a bundle with non-default config for a variable that needs quotes).
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 8ed9c31 to blathers/backport-release-23.2-123794: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.2.x failed. See errors above. error creating merge commit from 4e5c031 to blathers/backport-release-24.1-123794: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 24.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
This PR improves stmt bundle collection to include all session variables and cluster settings that differ from their defaults.
Fixes: #119786
Fixes: #123830.
Release note: None