Python: reuse implicit pybind prepared statements for repeated parameterized execute() calls#38
Merged
Merged
Conversation
ericyuanhui
force-pushed
the
Prepare_Cached
branch
from
21a822a to
b5613cc
Compare
Contributor
Author
|
@adsharma The CI pipeline is stuck right now. How can I trigger it? |
Contributor
Author
|
Contributor
|
Probably some github issue a few hours ago. Manually triggered. |
Signed-off-by: ericyuanhui <285521263@qq.com>
Two bugs fixed in the test: 1. Add close() method to _FakeResult — QueryResult.close() calls self._query_result.close() internally; the missing method caused AttributeError during teardown on C-API CI. 2. Fix _get_pybind_connection monkeypatch to store fake_pybind in self._py_connection — the bare lambda returned fake_pybind but never stored it, so Connection.close() never called fake_pybind_connection.close(), causing 'assert fake_pybind_connection.closed is True' to fail. The test runs on both pybind and C-API CI jobs. These changes ensure it passes on C-API without affecting the pybind behavior.
Two failure modes were present in the implicit prepared-statement cache
on Connection:
1. C-API path: every parameterized execute() called _prepare() which
triggered a fresh lbug_connection_prepare, never reusing the
previously prepared statement. RSS would grow without bound on
workloads that re-issued the same query.
2. pybind path: the cached PreparedStatement was reused, but when a
caller's parameter *shape* changed (e.g. MAP -> STRUCT, or two
STRUCTs with different field names), the upstream C++ plan
(cachedPreparedStatementManager) stayed bound to the first shape's
plan while the prepared statement's parameterMap was rebound in
place. canReuseCachedPlanWith then returned true on the third call
and the executor ran the stale plan against the new shape,
segfaulting in Value::copyFromRowLayout. Reproduced by
test_parameter.py::test_dict_conversion (MAP, STRUCT, STRUCT).
The C-API's "fix" of binding to a fresh temp per execute (commit
4afc0ad) traded correctness for cost: it always re-prepares, defeating
the cache entirely.
Fix:
* Cache key is (query, param_signature) instead of just query. The
signature is a structural hash of the parameter *type* (not values):
MAP vs STRUCT, STRUCT field names, MAP key/value element types. Two
callers passing the same query with different shapes get independent
prepared statements whose plans stay consistent with their bound
values, avoiding the upstream mismatch.
* Per-connection RLock (_prepared_cache_lock) serializes prepare + bind
+ execute so concurrent threads/async tasks cannot tear the cached
entry's mutable bound state. The C++ side already takes its own mtx
inside executeWithParams; the Python lock only needs to cover the
C-API _bound_values map and the cache itself.
* close() walks the cache and calls close() on each entry to destroy
the C++ lbug_prepared_statement; the C-API wrapper has no __del__ so
the entries would otherwise leak their _prepared_statement and
_bound_values C++ allocations.
* _get_or_prepare_pybind_statement and _execute_with_pybind hold the
lock around prepare+bind+execute.
* Tests:
- test_pybind_implicit_prepare_cache.py: updated for the new tuple
cache key and added two tests that pin the new
shape-segregation behavior (3 distinct shapes -> 3 prepares; same
shape, different values -> 1 prepare).
- test_pybind_implicit_prepare_cache_threading.py: NEW. Reproduces
the corruption in the pybind backend with 2/8/32 threads on a
single shared connection. Currently FAILS on the pybind backend
(to be debugged separately) and PASSES on the C-API backend.
test_fsm_reclaim_node_table_delete and test_fsm_reclaim_rel_table_delete
were failing because get_used_page_ranges() pulled in PK index pages
(start_page_idx 31..51 in the vPerson fixture) and asserted they were
a subset of the post-checkpoint free pages. match ... delete only
reclaims data pages; the table and its persistent hash index stay
around for future inserts, so those pages legitimately remain in use.
Add get_used_node_data_page_ranges() that filters storage_info by
table_type = 'NODE' and switch the two _delete tests to use it. The
drop-table tests keep the original get_used_page_ranges() since drop
removes the index along with the table.
Likely broken by ladybug 9cd338e3f ('Fix hash index storage accounting
and show built-in PK indexes'), which gave the hash index a
getStorageEntries() implementation -- combined with 8a8c0f13f ('Report
ART index storage in info UDFs') that wired appendStorageInfoForIndex
into storage_info, this caused PK index pages to start showing up in
storage_info output for the first time. The test was originally added
in 8a55bc6 / C++ eaf97b755 ('Reclaim pages for fully-deleted node
groups') when storage_info returned only NODE/REL data pages.
…ndex In numpy_scan.cpp, the OBJECT/STRING scan path uses pos = i + offset (the absolute row index in the source array) as the output vector index for setNull() when handling None or NaN values: ```cpp // BEFORE (buggy): outputVector->setNull(pos, true /* isNull */); // pos = i + offset ``` The output vector is sized for DEFAULT_VECTOR_CAPACITY (= 2048) entries and expects 0-based indices within each batch. But pos is the absolute dataframe row index. When the COPY spans multiple batches (i.e., when offset >= DEFAULT_VECTOR_CAPACITY), pos exceeds the null mask buffer, causing a heap buffer overrun → the segfault / free(): invalid next size / munmap_chunk(): invalid pointer crashes seen on Linux.
…s_equal NaN-aware The test generated float32 values by taking random 32-bit patterns and reinterpreting them as IEEE 754 floats. About 0.4% of those patterns are NaN (all-ones exponent, nonzero mantissa), so the floatcol contained dozens of NaN values at the larger scale factors. pyarrow.Array == Array follows the Arrow spec (NaN != NaN), so tables_equal()'s element-wise comparison would report two tables that differ only by NaN positions as not-equal. Locally, pa.Table.from_pandas converts NaN to None when building the expected patable, masking the issue. In CI (different pyarrow/pandas versions) the NaN survives in both the expected and scanned tables and the comparison fails with 'tables are not equal'. Two-part fix: - generate_primitive() for float32 now retries until it gets a non-NaN bit pattern. Subnormals, +/-Inf, signed zero, etc. are all still exercised, just not NaN. - tables_equal() routes floating columns through numpy.array_equal(equal_nan=True), which is the correct semantic for a round-trip scan test. Non-floating columns keep the cheap pyarrow.Array != Array check. Related: ladybug #647 (data corruption in the DataFrame scan path on Linux, fixed in 530c5ec for numpy_scan.cpp). This is a different bug in the test itself, not in the scan path.
Contributor
|
@ericyuanhui - please review the rest of the commits and open an issue if you see anything still broken. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
This PR fixes a memory growth issue in the Python API when using the pybind backend for parameterized string queries.
Previously, repeated calls to:
conn.execute(query, params)
would implicitly prepare the same query text on every execution. Since the native prepared statement cache does not currently evict these entries, long-running workloads could accumulate prepared statements and show steadily increasing RSS.
This PR changes the pybind execution path to reuse implicitly prepared statements per connection.
Root cause
Before this change, _execute_with_pybind() effectively behaved like:
prepared = py_connection.prepare(query, parameters)
return py_connection.execute(prepared, parameters)
for every parameterized string query.
As a result:
identical query text was prepared repeatedly
each prepare created a new native cached prepared statement
the native cache grew without bound for this workload pattern
Solution
This PR adds a small connection-local cache for implicitly prepared pybind statements.
Behavior after this change:
only applies to pybind execution of string queries with non-empty parameters
the normalized query string is used as the cache key
the first execution prepares and stores the statement
subsequent executions of the same query reuse the cached prepared statement
the cache is cleared when the Connection is closed
This keeps the fix narrowly scoped to the buggy path and avoids changing explicit prepare() / execute(prepared_stmt, params) behavior.
Why this approach
This fix is intentionally implemented at the Python layer rather than by changing native prepared statement manager behavior.
Reasons:
it directly addresses the source of the repeated implicit prepares
it minimizes behavioral change outside the affected pybind path
it keeps explicit prepared statement semantics unchanged
it avoids introducing a broader eviction policy change in native code
Tests
This PR adds regression tests covering:
repeated execution of the same parameterized query only prepares once
different query strings do not share cached prepared statements
non-parameterized queries still use the direct query() path
closing the connection clears the implicit prepared statement cache
Result
With this change, the repro workload no longer allocates one new native prepared statement per batch in the implicit pybind path, so the monotonic RSS growth is substantially reduced.
fix issue :#37