sql: add default_value and origin to show cluster settings #104449

maryliag · 2023-06-06T21:33:52Z

Fixes https://cockroachlabs.atlassian.net/browse/CRDB-28519

Commit 1
This adds an API to the settings package to track the origin of the current value of each setting, such as whether it is set by the the default value, an explicit override or an external override.

Commit 2
Previously, there was no easy way to see default values for
cluster settings. This commit add the column for default_value
and origin to crdb_internal.cluster_settings and
the show cluster settings command.

Release note (sql change): Add columns default_value and
origin (default, override, external-override) to the
show cluster settings command.

cockroach-teamcity · 2023-06-06T21:34:01Z

This change is

rafiss

nice, this is a great improvement! just small nits

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)

-- commits line 12 at r1:
nit: let's avoid mentioning the crdb_internal table in the release note. it's good for the commit message though.

pkg/sql/crdb_internal.go line 2117 at r1 (raw file):

			defaultVal, err := setting.DecodeToString(setting.EncodedDefault())
			if err != nil {
				defaultVal = setting.EncodedDefault()

why should we ignore the error here? i don't think this is ever expected to happen. if it is expected for some reason, that deserves a comment in the code

maryliag

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

pkg/sql/crdb_internal.go line 2117 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

why should we ignore the error here? i don't think this is ever expected to happen. if it is expected for some reason, that deserves a comment in the code

do you think this is a "panic" level or just "log this error" type of thing here?

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)

pkg/sql/crdb_internal.go line 2117 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

do you think this is a "panic" level or just "log this error" type of thing here?

i'd say neither, it should just be a:

if err != nil {
  return err
}

knz · 2023-06-07T13:18:16Z

Maybe this would benefit from #104487

maryliag · 2023-06-07T14:16:41Z

Made changes to the column be origin instead of is_overridden. Right now is just comparing with the default, but once #104487 gets merged, this PR can be updated to use the value from there

maryliag

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

-- commits line 12 at r1:

Previously, rafiss (Rafi Shamim) wrote…

nit: let's avoid mentioning the crdb_internal table in the release note. it's good for the commit message though.

Done

pkg/sql/crdb_internal.go line 2117 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i'd say neither, it should just be a:
if err != nil {
  return err
}

Done

rafiss

lgtm, but had a question

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)

pkg/sql/crdb_internal.go line 2123 at r4 (raw file):

			// TODO(dt): make this reflect overrides for tenants
			if strVal != defaultVal {
				origin = "override"

what would "external-override" mean? i saw it in the comment, but doesn't look like we ever use it.

maryliag

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

pkg/sql/crdb_internal.go line 2123 at r4 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

what would "external-override" mean? i saw it in the comment, but doesn't look like we ever use it.

Currently not using because I'm waiting for #104487 to get merged, so I can use the flag from there on this PR as I mentioned on this comment.
The difference:
default: the default
ExplicitlySet: the user was the one who made the change
ExternallySet: someone set up for the entire cluster, so if a tenant is looking it will see the value has changed, but they were not the ones who did it

This adds an API to the settings package to track the origin of the current value of each setting, such as whether it is set by the the default value, an explicit override or an external override. Release note: none. Epic: none.

maryliag

Commit from #104487 was added to this PR. PTAL
thank you @dt for the changes!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

pkg/sql/logictest/testdata/logic_test/cluster_settings line 95 at r6 (raw file):

----
sql.index_recommendation.drop_unused_duration  10s 168h0m0s override

@dt do you have any suggestions on how to test the external-override ?

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @maryliag)

pkg/settings/setting.go line 164 at r7 (raw file):

func (v ValueOrigin) String() string {
	return [...]string{"default", "override", "external-override"}[v]

nit: this has a risk of an index out of bounds. can we add:

if v > OriginExternallySet {
  return fmt.Sprintf("invalid (%d)", m)
}

pkg/sql/logictest/testdata/logic_test/cluster_settings line 95 at r6 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

@dt do you have any suggestions on how to test the external-override ?

i think it would be something like this:

user host-cluster-root

statement ok
ALTER TENANT ALL SET CLUSTER SETTING some.setting = 'value'

user root

query TTTT
SELECT variable, value, default_value, origin FROM [SHOW ALL CLUSTER SETTINGS]
WHERE variable IN ('some.setting')

maryliag

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @rafiss)

pkg/settings/setting.go line 164 at r7 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: this has a risk of an index out of bounds. can we add:
if v > OriginExternallySet {
  return fmt.Sprintf("invalid (%d)", m)
}

Done

pkg/sql/logictest/testdata/logic_test/cluster_settings line 95 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i think it would be something like this:

user host-cluster-root

statement ok
ALTER TENANT ALL SET CLUSTER SETTING some.setting = 'value'

user root

query TTTT
SELECT variable, value, default_value, origin FROM [SHOW ALL CLUSTER SETTINGS]
WHERE variable IN ('some.setting')

Nice! Thanks! Added

Fixes https://cockroachlabs.atlassian.net/browse/CRDB-28519 Previously, there was no easy way to see default values for cluster settings. This commit add the column for `default_value` and `origin` to `crdb_internal.cluster_settings` and the `show cluster settings` command. Release note (sql change): Add columns `default_value` and `origin` (default, override, external-override) to the `show cluster settings` command.

maryliag · 2023-06-23T17:36:57Z

TFTRs!
bors r+

craig · 2023-06-23T18:27:58Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl · 2023-06-23T18:28:12Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error setting reviewers, but backport branch blathers/backport-release-23.1-104449 is ready: POST https://api.github.com/repos/cockroachdb/cockroach/pulls/105452/requested_reviewers: 422 Reviews may only be requested from collaborators. One or more of the teams you specified is not a collaborator of the cockroachdb/cockroach repository. []

Backport to branch 23.1.x failed. See errors above.

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

The addition of new columns on cluster_settings view done on cockroachdb#104449 were causing debug zip fails to add the information from that table, since the query used to gather the information was doing a join and not considering the new columns. This commit updates the query to use the explicit columns, so even if new columns are added it won't be a problem in the future. It also adds tests for all custom querys used to generate the debug zip, so this type of issue would have been caught. Fixes cockroachdb#107103 Release note (bug fix): Debug zip now are properly showing the information from cluster_settings.

The addition of new columns on cluster_settings view done on cockroachdb#104449 were causing debug zip fails to add the information from that table, since the query used to gather the information was doing a join and not considering the new columns. This commit updates the query to use the explicit columns, so even if new columns are added it won't be a problem in the future. It also adds tests for all custom querys used to generate the debug zip, so this type of issue would have been caught. The file `crdb_internal.cluster_settings.txt` in debug zips was empty due to this bug on v23.1.5 (only version affected by this bug). Fixes cockroachdb#107103 Release note (bug fix): Debug zip now are properly showing the information from cluster_settings. The file `crdb_internal.cluster_settings.txt` in debug zips was empty due to this bug on v23.1.5 (only version affected by this bug).

106485: opt: only infer self-join equality with a key over the base table r=DrewKimball a=DrewKimball #### opt: only infer self-join equality with a key over the base table Self-join equality inference was added by #105214, so that the `FuncDeps` for a self-join would include equalities between *every* pair of columns at the same ordinal position in the base table if there was an equality between key columns (also at the same ordinal position). However, the key column check was performed using the FDs of the join inputs rather than the base table's FDs. This could lead to incorrectly adding self-join equality filters. For example, consider the following: ``` CREATE TABLE t106371 (x INT NOT NULL, y INT NOT NULL); INSERT INTO t106371 VALUES (1, 1), (1, 2); SELECT * FROM t106371 a JOIN t106371 b ON a.x = b.x; SELECT * FROM (SELECT * FROM t106371 ORDER BY y DESC LIMIT 1) a JOIN (SELECT DISTINCT ON (x) * FROM t106371) b ON a.x = b.x; ``` In the first query above, `a.x = b.x` does not consitute joining on key columns. But in the second query, one input has one row and the other de-duplicated by the `x` column and so `x` is a key over both inputs. However, the query as written will select different rows for each input - `a` will return the `(1, 2)` row, while `b` will return the `(1, 1)` row. Inferring a `a.y = b.y` filter will incorrectly cause the join to return no rows. This patch fixes the problem by requiring the initial self-join equalities to form a key over the *base* table, not just the inputs of the join. Fixes #106371 Release note: None 106779: acceptance: Fix DockerCSharp Test r=rimadeodhar a=rimadeodhar This PR updates the .net framework within the csproj setup to .net 6. Additionally, it also fixes the CS program that we run as a part of this test with the new npgsql DateTime changes made as a part of npgsql 6 version update. The old datetime types have been deprecated and needed to be removed. With these fixes, the test is running successfully and can be unskipped. Epic: none Fixes: #86852 Release note: none 107104: cli: fix debug zip with new columns for cluster settings r=maryliag a=maryliag The addition of new columns on cluster_settings view done on #104449 were causing debug zip fails to add the information from that table, since the query used to gather the information was doing a join and not considering the new columns. This commit updates the query to use the explicit columns, so even if new columns are added it won't be a problem in the future. It also adds tests for all custom querys used to generate the debug zip, so this type of issue would have been caught. The file `crdb_internal.cluster_settings.txt` in debug zips was empty due to this bug on v23.1.5 (only version affected by this bug). Fixes #107103 Release note (bug fix): Debug zip now are properly showing the information from cluster_settings. The file `crdb_internal.cluster_settings.txt` in debug zips was empty due to this bug on v23.1.5 (only version affected by this bug). Co-authored-by: Drew Kimball <[email protected]> Co-authored-by: rimadeodhar <[email protected]> Co-authored-by: maryliag <[email protected]>

The addition of new columns on cluster_settings view done on #104449 were causing debug zip fails to add the information from that table, since the query used to gather the information was doing a join and not considering the new columns. This commit updates the query to use the explicit columns, so even if new columns are added it won't be a problem in the future. It also adds tests for all custom querys used to generate the debug zip, so this type of issue would have been caught. The file `crdb_internal.cluster_settings.txt` in debug zips was empty due to this bug on v23.1.5 (only version affected by this bug). Fixes #107103 Release note (bug fix): Debug zip now are properly showing the information from cluster_settings. The file `crdb_internal.cluster_settings.txt` in debug zips was empty due to this bug on v23.1.5 (only version affected by this bug).

maryliag requested a review from a team June 6, 2023 21:33

maryliag requested a review from a team as a code owner June 6, 2023 21:33

maryliag added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Jun 6, 2023

rafiss requested changes Jun 6, 2023

View reviewed changes

maryliag commented Jun 6, 2023

View reviewed changes

rafiss requested changes Jun 6, 2023

View reviewed changes

maryliag force-pushed the cluster-set branch from 706e0a9 to 45d41ab Compare June 7, 2023 14:12

maryliag changed the title ~~sql: add default_value and is_overridden to cluster settings~~ sql: add default_value and origin to show cluster settings Jun 7, 2023

maryliag force-pushed the cluster-set branch from 45d41ab to a04e1f5 Compare June 7, 2023 14:13

maryliag commented Jun 7, 2023

View reviewed changes

maryliag force-pushed the cluster-set branch 2 times, most recently from 0c55b5b to 73a3bed Compare June 7, 2023 16:54

maryliag removed the backport-23.1.x Flags PRs that need to be backported to 23.1 label Jun 8, 2023

rafiss approved these changes Jun 9, 2023

View reviewed changes

maryliag commented Jun 9, 2023

View reviewed changes

settings: track the origin of the current values

f13f008

This adds an API to the settings package to track the origin of the current value of each setting, such as whether it is set by the the default value, an explicit override or an external override. Release note: none. Epic: none.

maryliag force-pushed the cluster-set branch from 73a3bed to d2a807f Compare June 22, 2023 16:43

maryliag requested review from a team as code owners June 22, 2023 16:43

maryliag commented Jun 22, 2023

View reviewed changes

maryliag mentioned this pull request Jun 22, 2023

settings: track the origin of the current values #104487

Closed

maryliag force-pushed the cluster-set branch from d2a807f to 2060b92 Compare June 22, 2023 17:27

maryliag requested a review from rafiss June 22, 2023 17:29

rafiss approved these changes Jun 23, 2023

View reviewed changes

maryliag force-pushed the cluster-set branch from 2060b92 to 5aaa500 Compare June 23, 2023 14:38

maryliag commented Jun 23, 2023

View reviewed changes

maryliag force-pushed the cluster-set branch from 5aaa500 to 819a929 Compare June 23, 2023 15:05

maryliag force-pushed the cluster-set branch 2 times, most recently from 4e44d1b to c33aa4d Compare June 23, 2023 15:56

maryliag force-pushed the cluster-set branch from c33aa4d to e64b6a4 Compare June 23, 2023 16:04

dt approved these changes Jun 23, 2023

View reviewed changes

maryliag added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Jun 23, 2023

craig bot merged commit a7c1494 into cockroachdb:master Jun 23, 2023

blathers-crl bot mentioned this pull request Jun 23, 2023

release-23.1: sql: add default_value and origin to show cluster settings #105452

Merged

maryliag deleted the cluster-set branch June 23, 2023 18:28

cockroach-teamcity mentioned this pull request Jun 24, 2023

PR #104449 - sql: add default_value and is_overridden to cluster settings cockroachdb/docs#17337

Closed

mgartner mentioned this pull request Jul 18, 2023

cli: cluster settings omitted in debug zip #107103

Closed

This was referenced Jul 18, 2023

cli: fix debug zip with new columns for cluster settings #107104

Merged

release-23.1: cli: fix debug zip with new columns for cluster settings #107105

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add default_value and origin to show cluster settings #104449

sql: add default_value and origin to show cluster settings #104449

maryliag commented Jun 6, 2023 •

edited

Loading

cockroach-teamcity commented Jun 6, 2023

rafiss left a comment

maryliag left a comment

rafiss left a comment

knz commented Jun 7, 2023

maryliag commented Jun 7, 2023

maryliag left a comment

rafiss left a comment

maryliag left a comment

maryliag left a comment

rafiss left a comment

maryliag left a comment

maryliag commented Jun 23, 2023

craig bot commented Jun 23, 2023

blathers-crl bot commented Jun 23, 2023

sql: add default_value and origin to show cluster settings #104449

sql: add default_value and origin to show cluster settings #104449

Conversation

maryliag commented Jun 6, 2023 • edited Loading

cockroach-teamcity commented Jun 6, 2023

rafiss left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

knz commented Jun 7, 2023

maryliag commented Jun 7, 2023

maryliag left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

maryliag commented Jun 23, 2023

craig bot commented Jun 23, 2023

blathers-crl bot commented Jun 23, 2023

maryliag commented Jun 6, 2023 •

edited

Loading