Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: default sql.stats.statement_fingerprint.format_mask to use special flags #120507

Merged
merged 1 commit into from Mar 22, 2024

Conversation

xinhaoz
Copy link
Member

@xinhaoz xinhaoz commented Mar 14, 2024

Please note only the latest commit should be reviewed.


By default sql.stats.statement_fingerprint.format_mask is now
set to FmtCollapseLists|FmtConstantsAsUnderscores to reduce
statement fingerprint cardinality due to long constant lists and
variations in constant formatting. Note that the default fmt flag
for statement fingerprint generation is FmtHideConstants. Any
flags set with sql.stats.statement_fingerprint.format_mask will be
OR'd with FmtHideConstants.

Closes: #120409

Release note (sql change): Users will see the following changes
in their generated statement fingerprints from sql stats:

  • lists with only literals/placeholders and similar subexpressions are
    shortened to their first item followed by "more", e.g.
  • constants and placeholders are all replaced with the same character,
    an underscore _
SELECT * FROM foo WHERE f IN (1, $1, 1+2) ->
SELECT * FROM foo WHERE f IN (_, __more__)

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@xinhaoz xinhaoz force-pushed the enable-enhanced-fingerprinting branch from 7291464 to d938fe9 Compare March 15, 2024 21:30
@xinhaoz xinhaoz changed the title sql: default sql.stats.enhanced_statement_fingerprint.enabled to true sql: default sql.stats.statement_fingerprint.format_mask to use special flags Mar 15, 2024
@xinhaoz xinhaoz force-pushed the enable-enhanced-fingerprinting branch 3 times, most recently from ee1a8d2 to 581fa17 Compare March 20, 2024 14:38
@xinhaoz xinhaoz marked this pull request as ready for review March 20, 2024 14:38
@xinhaoz xinhaoz requested review from a team as code owners March 20, 2024 14:38
@xinhaoz xinhaoz requested review from abarganier and a team and removed request for a team March 20, 2024 14:38
@xinhaoz xinhaoz force-pushed the enable-enhanced-fingerprinting branch 2 times, most recently from a6654ed to adcfb6c Compare March 20, 2024 17:50
@xinhaoz xinhaoz requested a review from dhartunian March 21, 2024 13:18
@xinhaoz xinhaoz force-pushed the enable-enhanced-fingerprinting branch from adcfb6c to 5a6a978 Compare March 21, 2024 13:31
Copy link
Member

@abarganier abarganier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:, modulo a nit & question

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @dhartunian and @xinhaoz)


pkg/sql/conn_executor.go line 3533 at r1 (raw file):

	settings.ApplicationLevel,
	"sql.stats.statement_fingerprint.format_mask",
	"enables setting additional fmt flags  FmtHideConstants for statement fingerprint formatting. "+

nit: grammar and spacing

Code quote:

flags  FmtHideConstants f

pkg/sql/conn_executor.go line 3537 at r1 (raw file):

	int64(tree.FmtCollapseLists|tree.FmtConstantsAsUnderscores),
	settings.WithValidateInt(func(i int64) error {
		if i == 0 || int64(tree.FmtCollapseLists|tree.FmtConstantsAsUnderscores)&i == i {

So we only allow the clearing of the setting to 0, or masks that include tree.FmtCollapseLists|tree.FmtConstantsAsUnderscores in the bitmask? Just want to clarify.

Code quote:

int64(tree.FmtCollapseLists|tree.FmtConstantsAsUnderscores)&i == i

@xinhaoz
Copy link
Member Author

xinhaoz commented Mar 22, 2024

pkg/sql/conn_executor.go line 3537 at r1 (raw file):

Previously, abarganier (Alex Barganier) wrote…

So we only allow the clearing of the setting to 0, or masks that include tree.FmtCollapseLists|tree.FmtConstantsAsUnderscores in the bitmask? Just want to clarify.

Ideally yes but this bitmask is more permissible, allowing any setting that is a subset of bits in the union of bits of both flags. I think this is good enough for an undocumented setting. Otherwise we'll need to check every combination of all flags in the future (assuming we'll add more). Let me know if you think we shuold tighthen the check to be more strict though.

…cial flags

By default `sql.stats.statement_fingerprint.format_mask` is now
set to `FmtCollapseLists|FmtConstantsAsUnderscores` to reduce
statement fingerprint cardinality due to long constant lists and
variations in constant formatting. Note that the default fmt flag
for statement fingerprint generation is `FmtHideConstants`. Any
flags set with sql.stats.statement_fingerprint.format_mask will be
OR'd with `FmtHideConstants`.

Closes: cockroachdb#120409

Release note (sql change): Users will see the following changes
in their generated statement fingerprints from sql stats:
- lists with only literals/placeholders and similar subexpressions are
shortened to their first item followed by "__more__", e.g.
- constants and placeholders are all replaced with the same character,
an underscore `_`
```
SELECT * FROM foo WHERE f IN (1, $1, 1+2) ->
SELECT * FROM foo WHERE f IN (_, __more__)
```
@xinhaoz xinhaoz force-pushed the enable-enhanced-fingerprinting branch from 5a6a978 to bf24702 Compare March 22, 2024 17:58
Copy link
Member

@abarganier abarganier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @dhartunian and @xinhaoz)


pkg/sql/conn_executor.go line 3537 at r1 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Ideally yes but this bitmask is more permissible, allowing any setting that is a subset of bits in the union of bits of both flags. I think this is good enough for an undocumented setting. Otherwise we'll need to check every combination of all flags in the future (assuming we'll add more). Let me know if you think we shuold tighthen the check to be more strict though.

That sgtm, just wanted to make sure I understood! Thanks

@xinhaoz
Copy link
Member Author

xinhaoz commented Mar 22, 2024

TFTR!
bors r+

@craig
Copy link
Contributor

craig bot commented Mar 22, 2024

@craig craig bot merged commit a2e6ec5 into cockroachdb:master Mar 22, 2024
19 of 22 checks passed
@xinhaoz xinhaoz deleted the enable-enhanced-fingerprinting branch April 1, 2024 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: make statement fingerprint string more generalized
3 participants