-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
release-23.2: ttljob: fix job to handle composite PKs #117512
Conversation
Thanks for opening a backport. Please check the backport criteria before merging:
If your backport adds new functionality, please ensure that the following additional criteria are satisfied:
Also, please add a brief release justification to the body of your PR to justify this |
c53891a
to
c32d8e3
Compare
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.
Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru, @rafiss, and @yuzefovich)
-- commits
line 11 at r1:
Do you think it would be worth removing the mention of decimal here, since it shouldn't actually cause skipped rows?
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.
Reviewed 6 of 8 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru and @DrewKimball)
Previously, DrewKimball (Drew Kimball) wrote…
Do you think it would be worth removing the mention of decimal here, since it shouldn't actually cause skipped rows?
Also perhaps mention that the bug was introduced when TTL feature was added (with concrete version).
Maybe hold off on merging this for a few days to let flakes like #117543 shake out on master? |
I agree, I was planning to wait a bit |
Certain types, like decimals and collated strings, store their contents in the value of the encoded KeyValue. The ttljob code that decodes the span bounds into datums for the purpose of creating SQL query bounds previously did not take this into account. Release note (bug fix): Fixed a bug in the row-level TTL job that would cause it to skip expired rows if the primary key of the table included columns of the collated string type. This bug was present since the initial release of row-level TTL in v22.2.0.
c32d8e3
to
1eb1ead
Compare
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! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @adityamaru, @DrewKimball, and @mgartner)
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Also perhaps mention that the bug was introduced when TTL feature was added (with concrete version).
updated on both accounts
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 (and 1 stale) (waiting on @adityamaru and @DrewKimball)
Backport 2/2 commits from #116988 and #117510
/cc @cockroachdb/release
Release justification: high priority bug fix
Certain types, like decimals and collated strings, store their contents in the value of the encoded KeyValue. The ttljob code that decodes the span bounds into datums for the purpose of creating SQL query bounds previously did not take this into account.
fixes #116845
Release note (bug fix): Fixed a bug in the row-level TTL job that would
cause it to skip expired rows if the primary key of the table included
columns of the collated string type. This bug was present since the
initial release of row-level TTL in v22.2.0.