-
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,crosscluster: correctly handle NULLABLE columns in KV writer #131645
Conversation
63d8693
to
6e3fa52
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.
Left only questions :D
@@ -212,6 +214,42 @@ func (rh *RowHelper) encodeSecondaryIndexes( | |||
return rh.indexEntries, nil | |||
} | |||
|
|||
// encodePrimaryIndexValuesToBuf encodes the given values, writing |
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 guess we needed to clean this up earlier than we thought :)
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.
:D I'm still not sure I consider this "cleaned up", but it's a baby step.
} | ||
ret := roachpb.Value{} | ||
if len(rd.rawValueBuf) > 0 { | ||
if family.ID == 0 || len(rd.rawValueBuf) > 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.
why did you add this conditional for Family.ID == 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.
This is an area where we've made it a little hard to test because we don't support column families in a lot of our tooling. But here is my understanding. Consider the case where all of the not-null columns in the primary index are contained in the primary key and there are not composite-encoded columns in the primary key. In this case, the key contains all of the data we want to write. Here, there are two cases:
- For family 0, our expected value should still contain a non-empty value. If it didn't it would look like a tombstone. So, we put in an empty tuple.
> select crdb_internal.pretty_key(key, -1), crdb_internal.pretty_value(value) from
-> crdb_internal.scan(crdb_internal.table_span('foo'::regclass::oid::int));
crdb_internal.pretty_key | crdb_internal.pretty_value
----------------------------+-----------------------------
/Tenant/2/Table/112/1/1/0 | /TUPLE/
- For other column families, if there are no values to store into the family, then we don't expect a key to exist at all, so our expectation should be an empty byte slice.
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.
every day i wake up grateful that i'm not a sql engineer.
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 prevValue.IsPresent() { | ||
expValue = prevValue.TagAndDataBytes() | ||
if !oth.PreviousWasDeleted { | ||
prevValue, err := rd.encodeValueForPrimaryIndexFamily(family, values) |
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.
why is this extra logic around oth.PreviousWasDeleted
only on the delete path, and not on the insert/update path?
Also, it's only possible for !oth.PreviousWasDeleted && !prevValue.IsPresent()
if the prev value is null right?
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.
Good question, I'll write some words about this, maybe I overlooked something this morning.
Here is the issue. A parsed rangefeed event for a delete contains some of the columns. Namely, it contains any columns in the primary key. This is also true for a phantom delete. But, for a phantom delete, the correct expected value is nothing. We need the columns from the key to construct the key for the delete, but when constructing the expectation we want to make sure that we expect no key (as opposed to an existing key with a sentinel body).
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.
yuck. given that the write will eventually succeed on a bogus expected value, i wonder it if is even worth having this extra logic. At the very least, perhaps we document this here?
6e3fa52
to
f9d41aa
Compare
f9d41aa
to
99516bb
Compare
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.
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 1 of 6 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @msbutler and @stevendanna)
pkg/sql/row/writer.go
line 145 at r3 (raw file):
// Skip any values with a default ID not stored in the primary index, // which can happen if we are adding new columns. if skip := helper.SkipColumnNotInPrimaryIndexValue(family.DefaultColumnID, values[idx]); skip {
The fact that this skip depends on the value of the column means we also need to calculate skip separately for the old value and the new value.
Hmm... actually there's an existing bug here! This shouldn't always be continue
. I've opened #131860 and have a fix going in #131869
pkg/sql/row/writer.go
line 226 at r3 (raw file):
// set to NULL, so delete it. if oth.IsSet() { oth.DelWithCPut(ctx, batch, kvKey, expBytes, traceKV)
We only need to do this if len(expBytes) > 0
. (If expBytes is zero, the previous KV shouldn't exist.)
Hmm, although now that I write that, I suppose we still need to issue the DelWithCPut to confirm that the previous KV doesn't exist and to check LWW. So scratch that.
Hmm! Which makes me realize, for this special LWW CPut mode, even if family.ID != 0 && len(rawValueBuf) == 0 && !overwrite
, we probably still need to perform the DelWithCPut just in case there's a conflicting KV already there. So the condition above should be if overwrite || oth.IsSet() {
.
(Sorry, I keep forgetting not to use Reviewable.) |
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.
:D No need to be sorry, feel free to use whatever you prefer.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @michae2 and @msbutler)
pkg/sql/row/writer.go
line 145 at r3 (raw file):
Previously, michae2 (Michael Erickson) wrote…
The fact that this skip depends on the value of the column means we also need to calculate skip separately for the old value and the new value.
Hmm... actually there's an existing bug here! This shouldn't always be
continue
. I've opened #131860 and have a fix going in #131869
Oh interesting. I didn't appreciate that composite encoding is value-dependent and not just type dependent.
pkg/sql/row/writer.go
line 226 at r3 (raw file):
Previously, michae2 (Michael Erickson) wrote…
We only need to do this if
len(expBytes) > 0
. (If expBytes is zero, the previous KV shouldn't exist.)Hmm, although now that I write that, I suppose we still need to issue the DelWithCPut to confirm that the previous KV doesn't exist and to check LWW. So scratch that.
Hmm! Which makes me realize, for this special LWW CPut mode, even if
family.ID != 0 && len(rawValueBuf) == 0 && !overwrite
, we probably still need to perform the DelWithCPut just in case there's a conflicting KV already there. So the condition above should beif overwrite || oth.IsSet() {
.
Nice catch. It'll be a little hard to test this case end-to-end since LDR doesn't yet support multiple column families.
99516bb
to
362f303
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 (waiting on @michae2 and @msbutler)
pkg/sql/row/writer.go
line 226 at r3 (raw file):
Previously, stevendanna (Steven Danna) wrote…
Nice catch. It'll be a little hard to test this case end-to-end since LDR doesn't yet support multiple column families.
If fixed this up, but also: I added an assertion at the top of the file to make sure we can't use these multi-column family code paths until we do more thinking here.
362f303
to
47e16ab
Compare
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.
47e16ab
to
55ccbff
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 1 of 2 files at r3, 2 of 3 files at r4, 1 of 1 files at r5, 1 of 1 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @msbutler)
if prevValue.IsPresent() { | ||
expValue = prevValue.TagAndDataBytes() | ||
if !oth.PreviousWasDeleted { | ||
prevValue, err := rd.encodeValueForPrimaryIndexFamily(family, values) |
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.
yuck. given that the write will eventually succeed on a bogus expected value, i wonder it if is even worth having this extra logic. At the very least, perhaps we document this here?
} | ||
ret := roachpb.Value{} | ||
if len(rd.rawValueBuf) > 0 { | ||
if family.ID == 0 || len(rd.rawValueBuf) > 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.
every day i wake up grateful that i'm not a sql engineer.
131869: sql/row: fix updates of single-composite-column families r=stevendanna a=michae2 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. Co-authored-by: Michael Erickson <[email protected]>
Previously, we were skipping encoding values based on whether the _new_ value was NULL, producing an incorrect _previous_ value. Further, if _all_ columns were NULL we were not correctly writing a sentinel value. Epic: none Release note: None
We need to write tests for multi-column families as these will not be hit by the end-to-end tests. Epic: none Release note: None
55ccbff
to
dc9291c
Compare
bors r+ |
Previously, we were skipping encoding values based on whether the new value was NULL, producing an incorrect previous value.
Further, if all columns were NULL we were not correctly writing a sentinel value.
Epic: none
Release note: None