sql/row: fix updates of single-composite-column families #131869
Conversation
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 writes an incorrect value to the primary index. This bug has existed since v22.2.
michae2
requested review from
stevendanna,
fqazi,
a team and
mgartner
and removed request for
a team
|
This change is |
michae2
added
backport-23.1.x
|
(I noticed this while reviewing #131645.) |
| } | ||
|
|
||
| if marshaled.RawBytes == nil { | ||
| if overwrite { | ||
| if len(marshaled.RawBytes) == 0 { |
There was a problem hiding this comment.
If you fancy: !marshaled.IsPresent()
| if marshaled.RawBytes == nil { | ||
| if overwrite { | ||
| if len(marshaled.RawBytes) == 0 { | ||
| if overwrite || oth.IsSet() { |
There was a problem hiding this comment.
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)
}| 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.
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.
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 writes an incorrect value to the primary index. This bug has existed since v22.2.