Skip to content

[Dev] Initialize new buffers with garbage data if DESTROY_UNPINNED_BLOCKS is set #11270

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
Merged
merged 6 commits into from
Apr 5, 2024
Merged

[Dev] Initialize new buffers with garbage data if DESTROY_UNPINNED_BLOCKS is set #11270

merged 6 commits into from
Apr 5, 2024

Conversation

Tishj
Copy link
Contributor

@Tishj Tishj commented Mar 20, 2024

This PR extends the use of DESTROY_UNPINNED_BLOCKS.

What it currently does is replace the buffer of an unpinned block, and fill the old buffer with garbage data, to find issues where buffers were being used after they had been unpinned.

What this PR adds is filling newly allocated buffers with garbage data, to find issues where we implicitly rely on new buffers being clean (zero initialized).

Thankfully it has found no issues when I enabled this and ran the fast tests on release mode, but none the less I think it's good to add this memory verification.

@Tishj Tishj requested a review from lnkuiper March 20, 2024 13:32
lnkuiper
lnkuiper suggested changes Mar 20, 2024
View reviewed changes
Copy link
Contributor

@lnkuiper lnkuiper left a comment

Choose a reason for hiding this comment

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

Good idea. Could we perhaps unify the memset code? It's now implemented 3x, once in VerifyZeroReaders, once in RegisterSmallMemory, and once in RegisterMemory.

@Mytherin
Copy link
Collaborator

Nightly CI that actually runs the verification here - https://github.com/Tishj/duckdb/actions/runs/8362180952

@github-actions github-actions bot marked this pull request as draft March 26, 2024 18:25
@Tishj Tishj marked this pull request as ready for review March 27, 2024 13:14
@Tishj
Copy link
Contributor Author

Tishj commented Mar 30, 2024

https://github.com/Tishj/duckdb/actions/runs/8493017123

@Tishj
Copy link
Contributor Author

Tishj commented Apr 2, 2024 •
edited
Loading

Sneaky CI failure:

[1244/2989] (41%): test/sql/table_function/duckdb_keywords.test                 
/home/runner/work/duckdb/duckdb/src/common/vector_operations/generators.cpp:70:61: runtime error: signed integer overflow: 9223372036854775807 * 2 cannot be represented in type 'long int'
[1245/2989] (41%): test/sql/table_function/test_range_function.test

Cant reproduce this locally or in my "local" Linux VM (both are ARM)
It might only reproduce on x86_64?
build:

DISABLE_STRING_INLINE=1 DESTROY_UNPINNED_BLOCKS=1 ALTERNATIVE_VERIFIY=1 make debug

It looks like it should be deterministic if we look at the values it's triggering on

EDIT:
I'm not sure if this is worth investigating as part of this PR, as they seem entirely unrelated

Even on x86_64 it seems to be nondeterministic because I cant reproduce it:

ubuntu:~/duckdb$ ./build/debug/test/unittest test/sql/table_function/test_range_function.test
Filters: test/sql/table_function/test_range_function.test
[1/1] (100%): test/sql/table_function/test_range_function.test                  
===============================================================================
All tests passed (100 assertions in 1 test case)

ubuntu:~/duckdb$ uname -a
Linux 6.5.0-1014-aws #14~22.04.1-Ubuntu SMP Thu Feb 15 15:27:06 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux

@Tishj Tishj requested a review from lnkuiper April 3, 2024 14:34
lnkuiper
lnkuiper approved these changes Apr 4, 2024
View reviewed changes
Copy link
Contributor

@lnkuiper lnkuiper left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot marked this pull request as draft April 4, 2024 13:42
@Tishj Tishj marked this pull request as ready for review April 4, 2024 13:42
@Mytherin Mytherin merged commit 0e3a51a into duckdb:main Apr 5, 2024
@Mytherin
Copy link
Collaborator

Mytherin commented Apr 5, 2024

Thanks!

github-actions bot pushed a commit to duckdb/duckdb-r that referenced this pull request Apr 5, 2024
@krlmlr
chore: Update vendored sources to duckdb/duckdb@0e3a51a
446286a 
Merge pull request duckdb/duckdb#11270 from Tishj/initialize_with_garbage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@lnkuiper lnkuiper lnkuiper approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Tishj @Mytherin @lnkuiper