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

catalog: fix tracking of id and name state #119007

Merged
merged 1 commit into from Feb 9, 2024

Conversation

rafiss
Copy link
Collaborator

@rafiss rafiss commented Feb 9, 2024

This allows us to re-enable the COMMENT ON statements in the
schemachange workload.

The previous code only updated the entries that were already in the
byIDState map. However, some descriptor IDs may not be in that map, so
instead we should add everything we just read into the map.

fixes #116795
Release note (bug fix): Fixed a bug where COMMENT statements could
fail with an "unexpected value" error if multiple COMMENT statements
were running concurrently.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rafiss rafiss marked this pull request as ready for review February 9, 2024 16:02
@rafiss rafiss requested review from a team as code owners February 9, 2024 16:02
@rafiss rafiss requested review from herkolategan, renatolabs and fqazi and removed request for a team February 9, 2024 16:02
@rafiss rafiss added the backport-23.2.x Flags PRs that need to be backported to 23.2. label Feb 9, 2024
Copy link
Collaborator

@fqazi fqazi left a comment

Choose a reason for hiding this comment

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

Amazing find and this must have led to poor cache utilization in some cases too :lgtm_strong:

Reviewed 4 of 4 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @herkolategan, @rafiss, and @renatolabs)


pkg/sql/catalog/internal/catkv/catalog_reader_cached.go line 203 at r1 (raw file):

	c.hasScanNamespaceForDatabases = true
	c.hasScanAllComments = true
	if err := read.ForEachDescriptor(func(desc catalog.Descriptor) error {

Can we add a data driven test inside pkg/sql/catalog/internal/catkv/catalog_reader_test.go? I think we should also see fewer scans in some cases.

This allows us to re-enable the COMMENT ON statements in the
schemachange workload.

The previous code only updated the entries that were already in the
byIDState map. However, some descriptor IDs may not be in that map, so
instead we should add everything we just read into the map.

Release note (bug fix): Fixed a bug where COMMENT statements could
fail with an "unexpected value" error if multiple COMMENT statements
were running concurrently.
Copy link
Collaborator Author

@rafiss rafiss 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! 1 of 0 LGTMs obtained (waiting on @fqazi, @herkolategan, and @renatolabs)


pkg/sql/catalog/internal/catkv/catalog_reader_cached.go line 203 at r1 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Can we add a data driven test inside pkg/sql/catalog/internal/catkv/catalog_reader_test.go? I think we should also see fewer scans in some cases.

done

@craig
Copy link
Contributor

craig bot commented Feb 9, 2024

Build succeeded:

@craig craig bot merged commit 5c84682 into cockroachdb:master Feb 9, 2024
7 of 9 checks passed
Copy link

blathers-crl bot commented Feb 9, 2024

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

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

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


error creating merge commit from 978a826 to blathers/backport-release-23.2-119007: 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.


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.2.x Flags PRs that need to be backported to 23.2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

workload/schemachange: enable COMMENT ON
3 participants