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/opt: fix negative annotation caching for RegionConfig #88515

Merged

Conversation

ajwerner
Copy link
Contributor

Fixes #88511

Release note (performance improvement): Fixed minor bug where negative caching of region configuration information could result in extra work when use a database that is not configured for multi-region.

@ajwerner ajwerner requested a review from a team as a code owner September 22, 2022 21:37
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yuzefovich yuzefovich requested a review from msirek September 22, 2022 21:38
Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

Nice find! LGTM, but I'll defer to @msirek for approval.

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @msirek)


-- commits line 6 at r1:
nit: I believe it was introduced in 1540ea8 during 22.2 cycle, so no stable release with this bug and the release note is not needed.


pkg/sql/opt/table_meta.go line 448 at r1 (raw file):

// GetRegionsInDatabase finds the full set of regions in the multiregion
// database owning the table described by `tm`, or returns ok=false if not

nit: s/ok/hasRegionNames/.

Copy link
Contributor

@msirek msirek left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!
:lgtm:

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

@ajwerner ajwerner force-pushed the ajwerner/fix-negative-region-cache branch from f42e0ee to 48529af Compare September 23, 2022 03:17
Copy link
Contributor Author

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

TFTR!

bors r+

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


-- commits line 6 at r1:

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: I believe it was introduced in 1540ea8 during 22.2 cycle, so no stable release with this bug and the release note is not needed.

Ack, gotta get that backport merged soon 🤞


pkg/sql/opt/table_meta.go line 448 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: s/ok/hasRegionNames/.

Done.

@craig
Copy link
Contributor

craig bot commented Sep 23, 2022

Build succeeded:

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: wasted CPU generating errors in SynthesizeRegionConfig
4 participants