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/norm: propagate kv errors from cast folding #87614

Merged
merged 1 commit into from
Sep 9, 2022

Conversation

ajwerner
Copy link
Contributor

@ajwerner ajwerner commented Sep 8, 2022

Swallowing KV errors here leads to incorrect results. Writes can be missed and serializability can be silently violated. This comes up in the context of the randomized schema change testing.

May deal with #85677
relates to #80764

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ajwerner
Copy link
Contributor Author

ajwerner commented Sep 8, 2022

@mgartner how does this make you feel?

@ajwerner
Copy link
Contributor Author

ajwerner commented Sep 8, 2022

happy to put a TODO around this to say that we ought to mark the function as leakproof, or not, and propagate errors differently because of it.

@ajwerner ajwerner force-pushed the ajwerner/fix-fold-cast branch from eabbba8 to efc5104 Compare September 8, 2022 19:28
@ajwerner ajwerner marked this pull request as ready for review September 8, 2022 19:53
@ajwerner ajwerner requested a review from a team as a code owner September 8, 2022 19:53
@mgartner mgartner force-pushed the ajwerner/fix-fold-cast branch from efc5104 to 7aaa348 Compare September 8, 2022 21:11
Copy link
Collaborator

@mgartner mgartner left a comment

Choose a reason for hiding this comment

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

LGTM. I added the same logic to FoldCast and added TODOs. PTAL.

@mgartner
Copy link
Collaborator

mgartner commented Sep 8, 2022

Thanks for do this, btw! I had started working on this but I didn't love my solutions. I like this the best so far.

Swallowing KV errors here leads to incorrect results. Writes can be missed
and serializability can be silently violated. This comes up in the context
of the randomized schema change testing.

Release note: None
@ajwerner
Copy link
Contributor Author

ajwerner commented Sep 9, 2022

Thanks!

@ajwerner
Copy link
Contributor Author

ajwerner commented Sep 9, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Sep 9, 2022

Build failed:

@ajwerner
Copy link
Contributor Author

ajwerner commented Sep 9, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Sep 9, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Sep 9, 2022

Build succeeded:

@blathers-crl
Copy link

blathers-crl bot commented Sep 9, 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 b075b30 to blathers/backport-release-21.2-87614: 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 21.2.x 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.

4 participants