sql/row: use Put instead of CPut when updating value of secondary index #137805
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
This comment was marked as resolved.
This comment was marked as resolved.
|
This change is |
michae2
force-pushed
the
secondary_put
branch
from
f8e766b to
939f5ff
Compare
michae2
force-pushed
the
secondary_put
branch
from
939f5ff to
001ac72
Compare
michae2
changed the title
michae2
force-pushed
the
secondary_put
branch
from
001ac72 to
2555746
Compare
|
I think this is RFAL. |
yuzefovich
approved these changes
Jan 11, 2025
There was a problem hiding this comment.
Nice!
Reviewed 2 of 2 files at r1, 7 of 7 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @arulajmani, @michae2, and @stevendanna)
pkg/sql/opt/exec/execbuilder/testdata/secondary_index_column_families line 231 at r1 (raw file):
# Ensure that updates only touch the changed column families. query T kvtrace(CPut,prefix=/Table/107/2/)
nit: let's update the directive to match the Put.
stevendanna
approved these changes
Jan 14, 2025
Comment on lines
428
to
435
| if traceKV { | ||
| k := keys.PrettyPrint(ru.Helper.secIndexValDirs[i], newEntry.Key) | ||
| v := newEntry.Value.PrettyPrint() | ||
| if expValue != nil { | ||
| log.VEventf(ctx, 2, "CPut %s -> %v (replacing %v, if exists)", k, v, oldEntry.Value.PrettyPrint()) | ||
| if sameKey { | ||
| log.VEventf(ctx, 2, "Put %s -> %v", k, v) | ||
| } else { | ||
| log.VEventf(ctx, 2, "CPut %s -> %v (expecting does not exist)", k, v) | ||
| log.VEventf(ctx, 2, "CPut %s -> %v", k, v) | ||
| } |
There was a problem hiding this comment.
Not for this PR, but if we ever do a cleanup pass of this function, it would be nice to re-organize things so the logging doesn't take up so much attention. Also of interest is that we log ForcePut's differently.
When an UPDATE statement changes the value but not the key of a secondary index (e.g. an update to the stored columns of a secondary index) we need to write a new version of the secondary index KV with the new value. We were using a CPutAllowingIfNotExists to do this, which verified that if the KV existed, the expected value was the value before update. But there's no need for this verification. We have other mechanisms to detect a write-write conflict with any other transaction that could have changed the value concurrently. We can simply use a Put to overwrite the previous value. This also matches what we do for the primary index when the PK doesn't change. Epic: None Release note: None
michae2
force-pushed
the
secondary_put
branch
from
2555746 to
d690dad
Compare
|
TFTRs! bors r=yuzefovich,stevendanna |
|
Build failed (retrying...):
|
CPutAllowingIfNotExists with empty expValue is equivalent to CPut with empty expValue, so change a few spots to use regular CPut. This almost gets rid of CPutAllowingIfNotExists entirely, but there is at least one spot in backfill (introduced in cockroachdb#138707) that needs to allow for both a non-empty expValue and non-existence of the KV. Also drop "(expecting does not exist)" from CPut tracing, as CPut with empty expValue is now overwhelmingly the most common use of CPut. And this matches the tracing in cockroachdb#138707. Epic: None Release note: None
michae2
force-pushed
the
secondary_put
branch
from
d690dad to
4850f7f
Compare
|
Trying again... bors r=yuzefovich,stevendanna |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
sql/row: use Put instead of CPut when updating value of secondary index
When an UPDATE statement changes the value but not the key of a secondary index (e.g. an update to the stored columns of a secondary index) we need to write a new version of the secondary index KV with the new value.
We were using a CPutAllowingIfNotExists to do this, which verified that if the KV existed, the expected value was the value before update. But there's no need for this verification. We have other mechanisms to detect a write-write conflict with any other transaction that could have changed the value concurrently. We can simply use a Put to overwrite the previous value.
This also matches what we do for the primary index when the PK doesn't change.
Epic: None
Release note: None
sql: change CPutAllowingIfNotExists with nil expValue to CPut
CPutAllowingIfNotExists with empty expValue is equivalent to CPut with empty expValue, so change a few spots to use regular CPut. This almost gets rid of CPutAllowingIfNotExists entirely, but there is at least one spot in backfill (introduced in #138707) that needs to allow for both a non-empty expValue and non-existence of the KV.
Also drop "(expecting does not exist)" from CPut tracing, as CPut with empty expValue is now overwhelmingly the most common use of CPut. And this matches the tracing in #138707.
Epic: None
Release note: None