-
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
sql/row: fix updates of single-composite-column families #131869
Conversation
(I noticed this while reviewing #131645.) |
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.
Great catch. In the other PR I'm going to try to write some tests for these multi-column-family cases even though we can't hit them in production yet.
pkg/sql/row/writer.go
Outdated
} | ||
|
||
if marshaled.RawBytes == nil { | ||
if overwrite { | ||
if len(marshaled.RawBytes) == 0 { |
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.
If you fancy: !marshaled.IsPresent()
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.
done
pkg/sql/row/writer.go
Outdated
if marshaled.RawBytes == nil { | ||
if overwrite { | ||
if len(marshaled.RawBytes) == 0 { | ||
if overwrite || oth.IsSet() { |
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.
I can always do this in my follow-up, but I wonder what you think about this, just so future us have a reminder of why we were doing this. I think all of the family.ID != 0 paths are suspect at the moment as they aren't covered by our LDR tests since we disable multiple column families.
if oth.IsSet() {
// If using OriginTimestamp'd CPuts, we _always_ want to issue a Delete
// so that we can confirm our expected bytes were correct.
oth.DelWithCPut(ctx, batch, kvKey, oldVal, traceKV)
} else if overwrite {
// If the new family contains a NULL value, then we must
// delete any pre-existing row.
insertDelFn(ctx, batch, kvKey, traceKV)
}
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.
Ah, nice! Done.
old, err := valueside.MarshalLegacy(typ, oldValues[idx]) | ||
if err != nil { | ||
return nil, err | ||
if !couldBeComposite || oldValues[idx].(tree.CompositeDatum).IsComposite() { |
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.
I think we can't go wrong with a comment here, because to be honest I'm finding it hard to follow. Let me try to think this through in realtime here in this comment:
We are encoding a single-column, non-family-0 column family. So, our expected bytes should be:
- Nil if the value is NULL.
- Nil is the value is non-NULL but exists complete in the primary key (i.e. is in the primary key and is not a composite value).
- Non nil otherwise.
Unless I'm reading wrongly, I think this code probably is not quite right for case 1. since I think TagAndDataBytes
will explode if old was encoded with just nil, so we might have to wrap that in an 'IsPresent()' call.
EDIT: I'm thinking that we should fix this up if possible but that we also err towards getting some of these fixes merged an perhaps putting an assertion at the top of this function that returns an error if we call it the OriginTimestampCPutHelper set and > 1 column families just to reflect the fact that we clearly need some follow-up testing.
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.
Added a little comment, and wrapped the call to TagAndDataBytes
in a check for IsPresent
.
(I'll leave the assertions up to you in your PR.)
When updating a single-column family which contains what could be a composite value from the primary key, we still need to issue a Del even if the new value for the column is not composite, because the previous value might have been composite. Fixes: cockroachdb#131860 Informs: cockroachdb#131645 Release note (bug fix): Fix a rare bug in which an update of a primary key column which is also the only column in a separate column family can sometimes fail to update the primary index. This bug has existed since v22.2.
Not sure what order we want to merge, but I'll just go ahead... bors r=stevendanna |
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 1672db9 to blathers/backport-release-23.1-131869: 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.1.x failed. See errors above. error creating merge commit from 1672db9 to blathers/backport-release-23.2-131869: 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. error creating merge commit from 1672db9 to blathers/backport-release-24.1-131869: 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 24.1.x failed. See errors above. error creating merge commit from 1672db9 to blathers/backport-release-24.2-131869: 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 24.2.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
When updating a single-column family which contains what could be a composite value from the primary key, we still need to issue a Del even if the new value for the column is not composite, because the previous value might have been composite.
Fixes: #131860
Informs: #131645
Release note (bug fix): Fix a rare bug in which an update of a primary key column which is also the only column in a separate column family can sometimes fail to update the primary index. This bug has existed since v22.2.