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: stale descriptor state could be observed in crdb_internal tables #76381

Merged
merged 1 commit into from
Feb 11, 2022

Conversation

fqazi
Copy link
Collaborator

@fqazi fqazi commented Feb 10, 2022

Fixes: #75825

Previously, when an uncommitted descriptor was added
to a descriptor collection, we would not reset the cache of
all immutable descriptors (used by some crdb_internal functions).
This was inadequate because this could result in
incorrect results from crdb_internal tables which returned
information related to constraints, columns. To address this,
this patch will reset the descriptor cache for all
fetched descriptors when an uncommitted descriptor is
added into the collection.

Release note: None

@fqazi fqazi requested review from ajwerner, postamar and a team February 10, 2022 17:03
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@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.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @fqazi and @postamar)


pkg/sql/catalog/descs/collection.go, line 212 at r1 (raw file):

	// Invalidate all the cached descriptors since a stale copy of this may be
	// included.
	tc.kv.releaseAllDescriptors()

my sense is we should remove the code that calls this on the read path

@fqazi fqazi force-pushed the collectionReleaseIssue branch from 587656c to 5e466a8 Compare February 10, 2022 17:33
Fixes: cockroachdb#75825

Previously, when an uncommitted descriptor was added
to a descriptor collection, we would not reset the cache of
all immutable descriptors (used by some crdb_internal functions).
This was inadequate because this could result in
incorrect results from crdb_internal tables which returned
information related to constraints, columns. To address this,
this patch will reset the descriptor cache for all
fetched descriptors when an uncommitted descriptor is
added into the collection.

Release note: None
@fqazi fqazi force-pushed the collectionReleaseIssue branch from 5e466a8 to bb0d19e Compare February 10, 2022 17:43
@fqazi fqazi requested a review from ajwerner February 11, 2022 04:01
Copy link
Contributor

@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.

:lgtm:

Reviewed 2 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @postamar)

@fqazi
Copy link
Collaborator Author

fqazi commented Feb 11, 2022

@ajwerner TFTR!

@fqazi
Copy link
Collaborator Author

fqazi commented Feb 11, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Feb 11, 2022

Build succeeded:

@craig craig bot merged commit fb98d7b into cockroachdb:master Feb 11, 2022
@rafiss
Copy link
Collaborator

rafiss commented Feb 11, 2022

thanks for the fix! any chance for a 21.2 backport? i think it might be affecting #70008 (comment) too

@rafiss
Copy link
Collaborator

rafiss commented Feb 14, 2022

blathers backport 21.2

@blathers-crl
Copy link

blathers-crl bot commented Feb 14, 2022

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 bb0d19e to blathers/backport-release-21.2-76381: POST https://api.github.com/repos/cockroachlabs/cockroach/merges: 403 Resource not accessible by integration []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 21.2 failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

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.

builtins: pg_get_constraintdef broken during DROP CONSTRAINT in txn
4 participants