Skip to content
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.

Sign up for GitHub

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

Jump to bottom

internal/rangekey: fix defragmentation of unsets/dels with ScanInternal #3058

Closed
itsbilal opened this issue Nov 10, 2023 · 0 comments · Fixed by #3069
Closed

internal/rangekey: fix defragmentation of unsets/dels with ScanInternal #3058

itsbilal opened this issue Nov 10, 2023 · 0 comments · Fixed by #3069
Assignees
@itsbilal
Labels
A-storage T-storage

Comments

@itsbilal
Copy link
Contributor

Currently, ScanInternal creates a UserIteratorConfig for range keys with onlySets set to false to also surface range key unsets/dels. However, the coalescing / defragmentation logic there is flawed, and crucially does not check for key kinds, only suffixes and values. A set (of an empty value) followed by an adjoining unset for the same suffix would get defragmented, losing information from the kind of one of those two spans.

To fix this, a variant of UserIteratorConfig could be used that correctly defragments all key kinds, similar to compactions, instead of relying on onlySets=false.

See comment in #3054 (review) for context and details.
@itsbilal itsbilal mentioned this issue Nov 10, 2023
Merged
@jbowens jbowens mentioned this issue Nov 10, 2023
Open
itsbilal added a commit to itsbilal/pebble that referenced this issue Nov 14, 2023
@itsbilal
*: stop using UserIteratorConfig for ScanInternal
dcb84e0 
This change moves the `scanInternalIterator` away from using
a UserIteratorConfig to coalesce range keys, and towards
something that's more in-line with rangeKeyCompactionTransform
i.e. just calling rangekey.Coalesce() on the relevant slice of
range keys if any. This preserves expected defragmenting
behaviour for cases where we have abutting range keys that
are identical in everything except kinds.

Fixes cockroachdb#3058.
@itsbilal itsbilal mentioned this issue Nov 14, 2023
Merged
itsbilal added a commit to itsbilal/pebble that referenced this issue Nov 15, 2023
@itsbilal
*: stop using UserIteratorConfig for ScanInternal
b72fe97 
This change moves the `scanInternalIterator` away from using
a UserIteratorConfig to coalesce range keys, and towards
something that's more in-line with rangeKeyCompactionTransform
i.e. just calling rangekey.Coalesce() on the relevant slice of
range keys if any. This preserves expected defragmenting
behaviour for cases where we have abutting range keys that
are identical in everything except kinds.

Fixes cockroachdb#3058.
itsbilal added a commit to itsbilal/pebble that referenced this issue Nov 16, 2023
@itsbilal
*: stop using UserIteratorConfig for ScanInternal
3107abf 
This change moves the `scanInternalIterator` away from using
a UserIteratorConfig to coalesce range keys, and towards
something that's more in-line with rangeKeyCompactionTransform
i.e. just calling rangekey.Coalesce() on the relevant slice of
range keys if any. This preserves expected defragmenting
behaviour for cases where we have abutting range keys that
are identical in everything except kinds.

Fixes cockroachdb#3058.
itsbilal added a commit to itsbilal/pebble that referenced this issue Nov 16, 2023
@itsbilal
*: stop using UserIteratorConfig for ScanInternal
cc89bda 
This change moves the `scanInternalIterator` away from using
a UserIteratorConfig to coalesce range keys, and towards
something that's more in-line with rangeKeyCompactionTransform
i.e. just calling rangekey.Coalesce() on the relevant slice of
range keys if any. This preserves expected defragmenting
behaviour for cases where we have abutting range keys that
are identical in everything except kinds.

Fixes cockroachdb#3058.
This was referenced Nov 22, 2023
Closed
Merged
pav-kv pushed a commit to pav-kv/pebble that referenced this issue Nov 28, 2023
@itsbilal
*: handle non-Sets correctly in UserIteratorConfig
8199731 
This change moves the `scanInternalIterator` use case of
UserIteratorConfig over to something that's closer to
rangeKeyCompactionTransform, where internal keys are coalesced
using rangekey.coalesce, key order is maintained at ByTrailerDesc,
and defragmentation happens by internal key using
keyspan.DefragmentInternal. The onlySets var is renamed to internalKeys
and its meaning is reversed.

Fixes cockroachdb#3058.
RaduBerinde pushed a commit to RaduBerinde/pebble that referenced this issue Nov 30, 2023
@itsbilal
*: handle non-Sets correctly in UserIteratorConfig
a0732ce 
This change moves the `scanInternalIterator` use case of
UserIteratorConfig over to something that's closer to
rangeKeyCompactionTransform, where internal keys are coalesced
using rangekey.coalesce, key order is maintained at ByTrailerDesc,
and defragmentation happens by internal key using
keyspan.DefragmentInternal. The onlySets var is renamed to internalKeys
and its meaning is reversed.

Fixes cockroachdb#3058.
@jbowens jbowens moved this to Done in [Deprecated] Storage Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@itsbilal itsbilal

Labels
A-storage T-storage
Projects
No open projects
[Deprecated] Storage
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

*: handle non-Sets correctly in UserIteratorConfig itsbilal/pebble
1 participant
@itsbilal