sql,crosscluster: correctly handle NULLABLE columns in KV writer #131645

stevendanna · 2024-10-01T05:22:32Z

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

cockroach-teamcity · 2024-10-01T05:22:42Z

This change is

msbutler

Left only questions :D

msbutler · 2024-10-01T13:52:41Z

pkg/sql/row/helper.go

@@ -212,6 +214,42 @@ func (rh *RowHelper) encodeSecondaryIndexes(
 	return rh.indexEntries, nil
 }

+// encodePrimaryIndexValuesToBuf encodes the given values, writing


i guess we needed to clean this up earlier than we thought :)

:D I'm still not sure I consider this "cleaned up", but it's a baby step.

msbutler · 2024-10-01T13:56:21Z

pkg/sql/row/deleter.go

 	}
 	ret := roachpb.Value{}
-	if len(rd.rawValueBuf) > 0 {
+	if family.ID == 0 || len(rd.rawValueBuf) > 0 {


why did you add this conditional for Family.ID == 0?

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.

msbutler · 2024-10-01T14:03:41Z

pkg/sql/row/deleter.go

-			if prevValue.IsPresent() {
-				expValue = prevValue.TagAndDataBytes()
+			if !oth.PreviousWasDeleted {
+				prevValue, err := rd.encodeValueForPrimaryIndexFamily(family, values)


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?

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).

msbutler · 2024-10-01T14:15:04Z

pkg/ccl/crosscluster/logical/lww_kv_processor_test.go

+		return err
+	}
+
+	t.Run("one-NULL-to-not-NULL", func(t *testing.T) {


for all these test cases, if we randomly send a stale previous value, everything should just work, right? I.e. we could randomly choose to send kv1 instead of kv2 as the previous value.

Yup, added some code that uses a random previous value about half of the time.

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

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() {.

michae2 · 2024-10-04T00:28:35Z

(Sorry, I keep forgetting not to use Reviewable.)

stevendanna

: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 be if 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.

stevendanna

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.

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.

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

stevendanna requested review from a team as code owners October 1, 2024 05:22

stevendanna requested review from jeffswenson and DrewKimball and removed request for a team October 1, 2024 05:22

stevendanna requested review from michae2 and msbutler and removed request for jeffswenson and DrewKimball October 1, 2024 05:22

stevendanna force-pushed the ssd/nullable branch 2 times, most recently from 63d8693 to 6e3fa52 Compare October 1, 2024 06:16

msbutler reviewed Oct 1, 2024

View reviewed changes

stevendanna force-pushed the ssd/nullable branch from 6e3fa52 to f9d41aa Compare October 1, 2024 16:06

mgartner requested a review from a team October 1, 2024 17:27

stevendanna force-pushed the ssd/nullable branch from f9d41aa to 99516bb Compare October 3, 2024 07:28

michae2 mentioned this pull request Oct 4, 2024

sql/row: fix updates of single-composite-column families #131869

Open

michae2 reviewed Oct 4, 2024

View reviewed changes

stevendanna commented Oct 4, 2024

View reviewed changes

stevendanna force-pushed the ssd/nullable branch from 99516bb to 362f303 Compare October 4, 2024 12:52

stevendanna commented Oct 4, 2024

View reviewed changes

stevendanna force-pushed the ssd/nullable branch from 362f303 to 47e16ab Compare October 4, 2024 13:55

stevendanna added 2 commits October 4, 2024 19:17

sql: add assertion failure to prevent use of untested code path

55ccbff

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

stevendanna force-pushed the ssd/nullable branch from 47e16ab to 55ccbff Compare October 4, 2024 18:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql,crosscluster: correctly handle NULLABLE columns in KV writer #131645

sql,crosscluster: correctly handle NULLABLE columns in KV writer #131645

stevendanna commented Oct 1, 2024

cockroach-teamcity commented Oct 1, 2024

msbutler left a comment

msbutler Oct 1, 2024

stevendanna Oct 1, 2024

msbutler Oct 1, 2024

stevendanna Oct 1, 2024 •

edited

Loading

msbutler Oct 1, 2024

stevendanna Oct 1, 2024 •

edited

Loading

msbutler Oct 1, 2024

stevendanna Oct 1, 2024

michae2 left a comment

michae2 commented Oct 4, 2024

stevendanna left a comment

stevendanna left a comment

sql,crosscluster: correctly handle NULLABLE columns in KV writer #131645

Are you sure you want to change the base?

sql,crosscluster: correctly handle NULLABLE columns in KV writer #131645

Conversation

stevendanna commented Oct 1, 2024

cockroach-teamcity commented Oct 1, 2024

msbutler left a comment

Choose a reason for hiding this comment

msbutler Oct 1, 2024

Choose a reason for hiding this comment

stevendanna Oct 1, 2024

Choose a reason for hiding this comment

msbutler Oct 1, 2024

Choose a reason for hiding this comment

stevendanna Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

msbutler Oct 1, 2024

Choose a reason for hiding this comment

stevendanna Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

msbutler Oct 1, 2024

Choose a reason for hiding this comment

stevendanna Oct 1, 2024

Choose a reason for hiding this comment

michae2 left a comment

Choose a reason for hiding this comment

michae2 commented Oct 4, 2024

stevendanna left a comment

Choose a reason for hiding this comment

stevendanna left a comment

Choose a reason for hiding this comment

stevendanna Oct 1, 2024 •

edited

Loading

stevendanna Oct 1, 2024 •

edited

Loading