-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
opt: fix assertion failure due to lax empty key #43722
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] the issue referenced in the commit/PR message (Fixes #43532) isn't an issue -- it's the original PR that created the issue. Is there an issue you meant to reference?
Reviewed 3 of 3 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball)
PR cockroachdb#43532 removed the concept of lax constant functional dependencies. There is a left-over case when we downgrade a key: if we had a strong empty key, the result is a lax empty key which is no longer a concept. This change fixes this by removing the key altogether in this case. Fixes cockroachdb#43651. Release note (bug fix): fixes "expected constant FD to be strict" internal error.
c523f38
to
0af511c
Compare
Thanks, fixed the issue number. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball)
There was a problem hiding this 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 - I missed this case.
Reviewable status:
complete! 2 of 0 LGTMs obtained (waiting on @andy-kimball)
bors r+ |
43703: storage/batcheval/result: perform various cleanup on LocalResult struct r=nvanbenschoten a=nvanbenschoten This PR contains a series of cleanups to the `result.LocalResult` struct. It leaves the structure in a good position to be extended to include information about newly written, updated, and removed intents, which are hooked into the `concurrency` package in future commits the same way that `UpdatedTxns` is currently hooked into the TxnWaitQueue. The changes are broken into a series of incremental steps to make them easier to review in isolation. 43722: opt: fix assertion failure due to lax empty key r=RaduBerinde a=RaduBerinde PR #43532 removed the concept of lax constant functional dependencies. There is a left-over case when we downgrade a key: if we had a strong empty key, the result is a lax empty key which is no longer a concept. This change fixes this by removing the key altogether in this case. Fixes #43651. Release note (bug fix): fixes "expected constant FD to be strict" internal error. 43734: pgwire: deflake TestAuthenticationAndHBARules r=knz a=knz Fixes #43733. This was my mistake to fix, I had forgotten that cluster settings propagate asynchronously. Release note: None Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com> Co-authored-by: Radu Berinde <radu@cockroachlabs.com> Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
Build succeeded |
PR #43532 removed the concept of lax constant functional dependencies.
There is a left-over case when we downgrade a key: if we had a strong
empty key, the result is a lax empty key which is no longer a concept.
This change fixes this by removing the key altogether in this case.
Fixes #43651.
Release note (bug fix): fixes "expected constant FD to be strict"
internal error.