fix: re-executed parameterized queries returned stale rows on the cached-plan fast path (#877) - #878
Merged
Merged
Conversation
…hed-plan fast path (#877) Re-executing the same parameterized query string (the recommended execute(query, params) form, which reuses the cached physical plan) returned the first execution's rows whenever the plan contained a sort, top-k, join, cartesian product, OPTIONAL MATCH, UNION, EXISTS subquery, LIMIT/SKIP or recursive extend. Clean output, no error — callers could not detect it. Two root causes, both from per-execution state surviving across executions of the cached operator tree (same family as #841 / #870): 1. Whole sub-pipelines vanished from the cloned plan template. Several operator copy() implementations only cloned children[0] and dropped the children attached later by the plan mapper (build sides, sort sinks, union collectors). ProcessorTask::run() and the fast path both clone through copy(), so the corresponding sink pipelines never ran again on later executions and operators kept serving execution 1's data: - OrderByScan / OrderByMerge / TopKScan (ORDER BY, top-k) - HashJoinProbe (build side — traversals, OPTIONAL MATCH, EXISTS, SIP) - Intersect, CrossProduct, PathPropertyProbe - UnionAllScan (UNION / UNION ALL) - RecursiveExtend, TableFunctionCall (FTable scans, recursive extend) - Profile, DummySimpleSink 2. Shared states accumulated per-execution state that was never reset: - SortSharedState kept payload tables / sorted key blocks / string key col info; KeyBlockMergeTaskDispatcher kept active merge tasks. - Limit / Skip counters stayed exhausted after the first execution. - HashJoinSharedState kept the previous execution's rows and hash slots. - UnionAllScanSharedState kept the previous scan cursors. - SemiMaskerSharedState re-merged previous local masks into the global node-offset masks (recursive extend). - RecursiveExtendSharedState kept its limit counter and factorized-table pool contents. - ResultCollector: internal collectors (union branches, cross-product / accumulate / SIP builds) are only read by other operators of the same plan, so they are now always cleared in place instead of being replaced with a fresh table the readers cannot see. Only the plan root's table (handed to the client via getQueryResult()) keeps the use_count-based fresh-table behavior for overlapping executions. Fixes: - copy() implementations preserve all children. - Per-execution state is reset on the hooks that run once per execution: initGlobalStateInternal() (Limit, Skip, HashJoinBuild, UnionAllScan, BaseSemiMasker, SortSharedState::init, KeyBlockMergeTaskDispatcher::init) and prepareForReuse() (RecursiveExtend). - ResultCollector distinguishes internal vs client-facing result tables. The Python AsyncConnection.execute(PreparedStatement, ...) UnboundLocalError mentioned in the issue does not reproduce on current main (fixed earlier by the explicit conn_index handling in async_connection.py). Regression test: ApiTest.RepeatedParameterizedCachedPlanExecution877 fails on unpatched main and passes with this change. Validation: full Python suite (153 passed), e2e suite (1961 passed; the single dictionary_bug~orb383 failure pre-exists on clean main), repeated overlap/concurrency tests on AsyncConnection pools.
adsharma
force-pushed
the
fix/877-cached-plan-reuse-state
branch
from
August 31, 2026 16:26
938bcf9 to
c660661
Compare
This was referenced Aug 31, 2026
adsharma
force-pushed
the
fix/877-cached-plan-reuse-state
branch
from
August 31, 2026 17:15
c660661 to
dad01f6
Compare
… switch
Add a global (client-context) configuration option to gate the cached-
physical-plan fast path for parameterized prepared statements:
CALL enable_cached_prepared_statement='reads' -- cache read plans only
CALL enable_cached_prepared_statement='writes' -- cache write plans only
CALL enable_cached_prepared_statement='both' -- cache all (default)
CALL enable_cached_prepared_statement='none' -- disable plan caching
Values are case-insensitive; anything else fails with an error listing the
supported inputs. The setting is registered like other client settings, so
it round-trips through current_setting('enable_cached_prepared_statement')
and is settable from every API (SQL CALL, Python/Java/Node set via SQL).
This is a safety valve for latent state-reuse bugs in the plan cache (the
class of bugs fixed by #841, #870 and #877): users who hit a misbehaving
query shape can disable or narrow the optimization without a code change,
instead of having to choose between wrong results and giving up on
parameterized queries. The default, BOTH, preserves the current behaviour
(parameterized reads and writes both take the fast path).
Enforcement lives in ClientContext::executeNoLock(), gating both cache
reuse and cache population, so a disabled scope never serves or fills the
plan cache regardless of which execute path is taken. A statement rejected
by the scope simply maps its physical plan fresh on every execution.
Regression coverage in prepare_test.cpp:
- setting round-trip via current_setting, invalid value rejected
- per-scope cache-population checks (read vs write statements, via
CachedPreparedStatement::physicalPlanCache):
BOTH caches both, READS caches reads only, WRITES caches writes only,
NONE caches nothing
- repeated executions return correct results in every scope, including
NONE where every execution re-maps the plan
adsharma
force-pushed
the
fix/877-cached-plan-reuse-state
branch
2 times, most recently
from
August 31, 2026 20:56
0a3ad1d to
05873a6
Compare
Contributor
Author
|
The test failures are pre-existing in main and tracked by #881 |
Mark the ten tests that intermittently SIGSEGV or lose CSR metadata in the linux minimal-test CI job as skipped, with a reference to the tracking issue: - ArrowTest.queryAsArrow, getArrowResult - ArrowTest.queryAsArrowDirectCSRRowIDProjection (+ ...WithFourThreads) - ArrowTest.queryAsArrowTracksCSRMetadataWithoutRelIDs / WithRelIDsAndExtraColumns / DoesNotTrackCSRMetadataForNonCSRShape - ProjectGraphCsrTest.materializesArrowCsr, materializedCsrSurvivesConsumingQueries - ReadOnlyTest.ProjectGraphOnReadOnlyDatabase The crash is a timing-dependent data race that pre-exists on main: worker threads execute a corrupted task clone in the arrow result collector path (worker threads race the task clone between creation and execution). Verified by reproducing the identical SIGSEGV on pristine main with debug instrumentation in a clean ASAN build. Diagnosis and repro recipe in #881; re-enable these tests once the race is fixed.
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
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
Fixes #877.
Re-executing the same parameterized query string — the recommended
execute(query, params)form, which reuses the cached physical plan — returned the first execution's rows whenever the plan contained a sort, top-k, join, cartesian product,OPTIONAL MATCH,UNION,EXISTSsubquery,LIMIT/SKIP, or a recursive extend. The output was well-formed and no error was raised, so callers could not detect it.Reproducer from the issue (three nodes with ids 1, 2, 3):
[1, 2, 3][1, 2, 3]ORDER BY/SKIP[1, 1, 1]/ no rows[1, 2, 3]OPTIONAL MATCH[1, 1, 1][1, 2, 3]LIMIT,UNION ALL[1, None, None][1, 2, 3]Root causes
Both are per-execution state surviving across executions of the cached operator tree — the same family as #841 and #870.
1. Whole sub-pipelines vanished from the cloned plan template
Several operator
copy()implementations only clonedchildren[0]and dropped the children the plan mapper attaches afterward (build sides, sort sinks, union collectors).ProcessorTask::run()and the cached-plan fast path inClientContext::executeNoLock()both clone throughcopy(), so the corresponding sink pipelines never ran again on later executions:OrderByScan/OrderByMerge/TopKScan— ORDER BY and top-k scans lost their sort sinksHashJoinProbe— dropped the build side (traversals, OPTIONAL MATCH, EXISTS, SIP-collector variants)Intersect,CrossProduct,PathPropertyProbeUnionAllScan— UNION / UNION ALLRecursiveExtend,TableFunctionCall(FTable scans),Profile,DummySimpleSinkVerified empirically: on the second execution of a top-k query the
TOP_Ksink task was never created at all — the sort was simply never re-run.2. Shared states kept per-execution state that was never reset
Limit/Skipcounters stayed exhausted after the first execution → "first call right, later calls return no rows"HashJoinSharedStatekept the previous execution's rows and hash slots (probe served stale rows)SortSharedStatekept payload tables / sorted key blocks;strKeyColsInfoaccumulated a duplicate entry per execution;KeyBlockMergeTaskDispatcherkept active merge tasksUnionAllScanSharedStatekept the previous scan cursors → no rows after the first callSemiMaskerSharedStatere-merged previous executions' local masks into the global node-offset masks (recursive extend)RecursiveExtendSharedStatekept its limit counter and factorized-table pool contentsResultCollector: internal collectors (union branches, cross-product / accumulate / SIP builds) are only read by other operators of the same plan, which hold references to the same table object — so they must be cleared in place, never replaced. Replacing them with a fresh table (the Fix state reuse on the cached-physical-plan fast path (0-row scans, live-result clobbering) #870use_countheuristic) left plan-internal readers (FTable scans,UnionAllScanSharedState,CrossProductLocalState) pointing at the stale table. Only the plan root collector — the one whose table is handed to the client viagetQueryResult()— keeps theuse_count-based fresh-table behavior for overlapping executions.Note on parameter values: the cached-plan path already re-reads parameter values live at
resolveResultVectortime (Phase 1 work), so values propagate correctly once the plan structure and state resets are fixed.Changes
copy()implementations preserve all childreninitGlobalStateInternal():Limit,Skip,HashJoinBuild,UnionAllScan,BaseSemiMasker,SortSharedState::init(),KeyBlockMergeTaskDispatcher::init()prepareForReuse():RecursiveExtendResultCollectordistinguishes internal vs client-facing result tables (flag propagated throughcopy())The
AsyncConnection.execute(PreparedStatement, ...)UnboundLocalErrormentioned in the issue does not reproduce on current main (theconn_indexhandling inasync_connection.pyalready covers it).Testing
ApiTest.RepeatedParameterizedCachedPlanExecution877(covers ORDER BY, top-k, LIMIT, SKIP, joins, cartesian, OPTIONAL MATCH, UNION ALL, EXISTS, var-length extend, repeated through the cached-plan fast path). Fails on unpatched main, passes with this change.test_async_prepare_and_execute_concurrent, the Fix state reuse on the cached-physical-plan fast path (0-row scans, live-result clobbering) #870 regression test)dictionary_bug~orb383_relationship_projection_obfuscated.AnonymousParquetDeleteReload) was verified to fail identically on clean mainAsyncConnectionpool stress with overlapping executions and held live results — no corruption, no crashes (an early draft of this change that didn't propagate the internal-table flag throughcopy()segfaulted here; the flag propagation fixes it)