Fix state reuse on the cached-physical-plan fast path (0-row scans, live-result clobbering) - #870
Merged
Merged
Conversation
The cached-physical-plan fast path (Phase 1-3, #78e1ccfd4/e103a49f2/ 80fa473) clones the template operator tree per execution and calls prepareForReuse(). Two pieces of mutable state were not handled: 1. Table-function scans silently returned 0 rows on re-execution. TableFunctionCall::copy() shares the TableFuncSharedState, and prepareForReuse() calls sharedState->resetState() to reset the scan position. FTableScanSharedState overrides resetState(), but SimpleTableFuncSharedState - used by pandas/polars/arrow scans and most simple table functions - inherited the no-op base implementation, so curRowIdx stayed at numRows and every getMorsel() returned an invalid morsel. Concretely: executing "LOAD FROM df" (which the Python layer rewrites to "LOAD FROM $df") followed by "LOAD FROM $df RETURN *" returned 0 rows / "No more tuples in QueryResult". Fix: override resetState() to reset curRowIdx, mirroring FTableScanSharedState. 2. ResultCollector::prepareForReuse() clobbered live QueryResults. The plan template shares the ResultCollectorSharedState (and its FactorizedTable) with every executed clone, and prepareForReuse() unconditionally clear()ed that table. A QueryResult from a previous execution holds a shared_ptr to the same table, so overlapping executions (e.g. AsyncConnection's pool) corrupted live results: queries returned other queries' rows or empty tables (test_async_prepare_and_execute_concurrent asserted [96] == [1]). Fix: clear-and-reuse the table only when no QueryResult still references it (use_count() == 1); otherwise give the execution a fresh table with the same schema and let the old one live until its QueryResult is destroyed. The sequential loop case keeps the Phase 2 block-reuse fast path. Verified: full Python suite (280 tests) passes including test_scan_pandas and test_async_connection; api_test, c_api_test, and 734 e2e tests pass (remaining failures pre-exist on main). Unblocks CI on LadybugDB/ladybug-python#53.
adsharma
added a commit
that referenced
this pull request
Aug 31, 2026
… 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
added a commit
that referenced
this pull request
Aug 31, 2026
…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
added a commit
that referenced
this pull request
Aug 31, 2026
… 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
added a commit
that referenced
this pull request
Aug 31, 2026
… 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
added a commit
that referenced
this pull request
Aug 31, 2026
… 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
added a commit
that referenced
this pull request
Aug 31, 2026
…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
added a commit
that referenced
this pull request
Aug 31, 2026
… 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
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
1 participant
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
Two bugs in the cached-physical-plan fast path (Phases 1-3) cause wrong results on the second execution of the same query string:
1. Table-function scans return 0 rows on re-execution
TableFunctionCall::copy()shares theTableFuncSharedState, andprepareForReuse()callssharedState->resetState()to rewind the scan position.FTableScanSharedStateoverridesresetState(), butSimpleTableFuncSharedState(pandas/polars/arrow scans and most simple table functions) inherited the no-op base, socurRowIdxstayed atnumRowsand everygetMorsel()returned an invalid morsel.User-visible effect:
LOAD FROM dffollowed byLOAD FROM $df RETURN *returned 0 rows / "No more tuples in QueryResult" (the Python layer rewrites both to the same string, so the second execution takes the fast path). Reproduced down togetMorsel()logging: freshinitSharedStatewas never called on the fast path and the stale cursor served invalid morsels to all worker threads.Fix: override
resetState()to resetcurRowIdx = 0, mirroringFTableScanSharedState.2.
ResultCollector::prepareForReuse()clobbered live QueryResultsThe plan template shares the
ResultCollectorSharedState(and itsFactorizedTable) with every executed clone, andprepareForReuse()unconditionallyclear()ed that table. AQueryResultfrom a previous execution holds ashared_ptrto the same table, so overlapping executions (e.g.AsyncConnection's pool runningRETURN $1concurrently across 4 connections) corrupted live results: queries returned other queries' rows or empty tables —test_async_prepare_and_execute_concurrentasserted[96] == [1].Fix: clear-and-reuse the table only when no external
QueryResultstill references it (use_count() == 1, which keeps the Phase 2 block-reuse fast path for sequential loops); otherwise hand the execution a fresh table with the same schema and let the old table live until itsQueryResultis destroyed.Test plan
test_scan_pandas::test_scan_pandasandtest_async_connection::test_async_prepare_and_execute_concurrent.api_test,c_api_test, and 734 filtered e2e tests pass (thePartitionRoutingTest.MixedLocalRemoteScanRejectedanddictionary_bug~orb383failures pre-exist on clean main and are untouched by this change).test_fsm.pyfailures on the same CI run are a test-side adaptation to Bug: COPY forces a checkpoint that auto_checkpoint and checkpoint_threshold cannot disable #755 (COPY no longer force-checkpoints underauto_checkpoint=false) and are fixed in Add regression test for issue ladybug#866 (SIGSEGV re-executing parameterized write) ladybug-python#53.Unblocks CI on LadybugDB/ladybug-python#53 (regression test for #866).