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.

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

colblk: Bitmap, BitmapBuilder bug fixes #4112

Merged
merged 2 commits into from
Oct 25, 2024

Conversation

jbowens
Copy link
Collaborator

@jbowens jbowens commented Oct 25, 2024

Previously, Invert used slices.Grow to grow the b.words slice if necessary. If len(b.words) < cap(b.words), words in the unused capacity were not initialized to zero and were assumed to be zero. Most of the time this was okay, because Reset zeroed b.words up to len(b.words).

However if the BitmapBuilder was previously used to Finish fewer rows than were Set, it's possible Finish may have truncated b.words to exclude words with non-zero bits. Since Reset only reset words within len(b.words), the truncated words were left unzeroed.

This commit updates Invert to make no assumptions as to the value of words beyond len(b.words). It also removes the zeroing in Reset which obscured this bug in most cases, instead relying on Set, Finish and Invert to properly initialize memory as necessary as it's used.

Fix #4103.

@jbowens jbowens requested a review from a team as a code owner October 25, 2024 18:16
@jbowens jbowens requested a review from itsbilal October 25, 2024 18:16
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@jbowens jbowens changed the title colblk: fix Invert to initialize memory colblk: Bitmap, BitmapBuilder bug fixes Oct 25, 2024
Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! slices.Grow does append(s[:cap(s)], make([]E, n)...):len(s)] and from that I incorrectly assumed any new elements are zero - which is not true for elements between len(s) and cap(s). Plus the doc doesn't make any guarantees anyway.

:lgtm:

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @itsbilal and @jbowens)


sstable/colblk/bitmap_test.go line 160 at r1 (raw file):

		buildSize := size + rng.IntN(5)
		v := make([]bool, buildSize)
		for i := 0; i < len(v); i++ {

[nit] := range v

Previously, Invert used slices.Grow to grow the b.words slice if necessary. If
len(b.words) < cap(b.words), words in the unused capacity were not initialized
to zero and were assumed to be zero. Most of the time this was okay, because
Reset zeroed b.words up to len(b.words).

However if the BitmapBuilder was previously used to Finish fewer rows than were
Set, it's possible Finish may have truncated b.words to exclude words with
non-zero bits. Since Reset only reset words within len(b.words), the truncated
words were left unzeroed.

This commit updates Invert to make no assumptions as to the value of words
beyond len(b.words). It also removes the zeroing in Reset which obscured this
bug in most cases, instead relying on Set, Finish and Invert to properly
initialize memory as necessary as it's used.

Fix cockroachdb#4103.
Fix an off-by-one on the summary table end bound. This could cause SeekSetBitGE
to return the wrong index in bitmaps with more than 4096 represented bits.
Copy link
Collaborator Author

@jbowens jbowens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense!

TFTR!

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @itsbilal and @RaduBerinde)

@jbowens jbowens merged commit d844f6f into cockroachdb:master Oct 25, 2024
23 checks passed
@jbowens jbowens deleted the fix-bitmap-invert branch October 25, 2024 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

github.com/cockroachdb/pebble/internal/metamorphic: TestMeta failed
3 participants