-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add memory leak tests, and fix memory leaks related to repeated table creation/destruction #5537
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly nitpicks
first_line = True | ||
for line in stdout.splitlines(): | ||
if first_line: | ||
first_line = False | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first_line = True | |
for line in stdout.splitlines(): | |
if first_line: | |
first_line = False | |
continue | |
for line in stdout.splitlines()[1:]: |
for i in range(len(rss)): | ||
memory = rss[i] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for i in range(len(rss)): | |
memory = rss[i] | |
for memory in rss: |
differences = [] | ||
for i in range(1, len(rss)): | ||
differences.append(rss[i] - rss[i - 1]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
differences = [] | |
for i in range(1, len(rss)): | |
differences.append(rss[i] - rss[i - 1]) | |
differences = [rss[i] - rss[i - 1] for i in range(1, len(rss))] |
print("------------------------------------------------") | ||
print(proc.stderr.read().decode('utf8')) | ||
exit(1) | ||
ps_proc = subprocess.Popen(f'ps -o rss= -p {pid}'.split(' '), stdout=subprocess.PIPE, stderr=subprocess.PIPE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use check_output
?
if len(line.strip()) == 0: | ||
continue | ||
splits = line.rsplit('\t', 1) | ||
if test_filter == '*' or test_filter in splits[0]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could probably use the fnmatch
module for this?
Fixes #5501
Memory Leak Tests
This PR adds a new type of test - the memory leak test. These tests do not look for traditional memory leaks (i.e. forgetting to call free) - but rather detect increases of memory usage by the system when running specific queries or operations in a loop. The former (forgetting to call free) almost never happens in DuckDB as we use RAII and smart pointers to avoid having to manually clean up resources. The latter, however, can happen in case objects are not cleaned up early enough (even if they do get cleaned up eventually when the database shuts down).
These tests are written using the C++ API, and look like this:
Note two structures:
--test-memory-leaks
) - this is checked for using theTestMemoryLeaks
at the top. If the flag is not passed the tests are skipped.while(true)
loop - this is why this flag is necessary. Running the test using the standard unittest program means the test will never terminate.Instead, to run these tests the
test_memory_leaks.py
script must be run, e.g.:This script runs the the test until either (a) the program's memory usage stabilizes/stops growing for more than 10 seconds straight, or (b) the timeout is reached (in which case the test is marked as a failure). By default the timeout is 60 seconds.
This PR also includes fixes for two memory leaks found through these tests.
Rolling Back Tables
This PR fixes an issue where tables that were created and then rolled back were not properly erased from the catalog - leading to an increase in memory usage. The reason this happened was that the catalog is set up with a layer of indirection where we have entries sitting in a separate vector from the name map. This layer of indirection exists to support ACID renaming of entries in the catalog set.
This PR makes this less error prone by creating a dedicated
EntryIndex
class that uses RAII to correctly destroy the entries as soon as no more mapping points to them, rather than leaving them in the catalog until the database is shut down.Eviction Queue Flooding
This PR also fixes #5501 - where creating and dropping temporary tables repeatedly causes memory usage to increase. The problem here is that the eviction queue of the buffer manager was flooded with blocks from the temporary tables that were no longer actually present in the database (as the temporary table had been erased). This PR fixes that by calling
PurgeQueue
also when blocks are destroyed - rather than only when they are inserted into the queue.