feat: cache query plans for queries using query-module functions - #4461
Merged
Conversation
Queries calling user-defined (query-module) functions were marked non-cacheable at parse time, so the whole query was re-parsed and re-planned on every execution. The gate existed because a cached plan baked in the function's callable plus a shared_ptr to its module: that would run stale code after a reload and pin the module, which reload refuses to unload while use_count != 1. Resolve module functions per-execution instead. The Function AST node now stores only is_user_defined_ and a slot index into a per-execution table built in the PullPlan constructor from AstStorage::user_functions_. The table holds the module references for the query's duration only, so idle cached plans never block a reload and the next execution re-resolves and picks up new code automatically. A cold-path fallback resolves on demand for evaluators without the table. The CALL procedure gate is left as-is; its YIELD result-name binding is a separate concern. Tests: unit coverage that a magic-function query is now cacheable and survives the AST cache/clone path, plus an e2e reload test asserting the cached query is not blocked on reload and re-resolves to the new impl.
DavIvek
added
Docs - changelog only
DavIvek
marked this pull request as draft
July 21, 2026 21:12
DavIvek
added
CI -build=coverage -test=core
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enables query plan caching for Cypher queries that call query-module (user-defined) functions by removing module/shared_ptr callables from the cached AST and instead resolving UDF callables per execution (keeping modules alive only for the execution lifetime). This addresses repeated re-planning overhead while preserving safe module reload behavior.
Changes:
- Refactors
FunctionAST nodes to store only a UDF flag + slot id (no module references), and records distinct UDF names inAstStorage::user_functions_. - Adds per-execution resolution of user-defined functions into
EvaluationContext::resolved_user_functions, and updates evaluators to dispatch via the resolved table (with on-demand fallback). - Adds unit + e2e coverage to validate cacheability and safe module reload with cached plans.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/cypher_main_visitor.cpp | Updates UDF expectations and adds a cacheability/unit-storage test for magic functions. |
| tests/e2e/python_query_modules_reloading/test_reload_query_module.py | Adds an e2e test ensuring cached UDF queries don’t block mg.load and pick up reloaded code. |
| tests/e2e/python_query_modules_reloading/procedures/reload_func_module.py | New query module used by the e2e reload test. |
| tests/e2e/python_query_modules_reloading/procedures/CMakeLists.txt | Ensures the new module file is copied into the e2e environment. |
| src/query/interpreter.cpp | Resolves user-defined functions once per execution into the evaluation context. |
| src/query/interpret/eval.hpp | Adds per-call selection logic to use per-execution resolved UDFs or resolve on demand. |
| src/query/interpret/awesome_memgraph_functions.hpp | Introduces ResolvedUserFunctions and APIs for resolving UDFs. |
| src/query/interpret/awesome_memgraph_functions.cpp | Implements per-execution and on-demand UDF resolution via the module registry. |
| src/query/frontend/ast/cypher_main_visitor.cpp | Removes the parse-time “UDF => non-cacheable” gate; assigns UDF slot ids into AST storage. |
| src/query/frontend/ast/ast.hpp | Refactors Function to avoid embedding module refs/callables for UDFs; stores UDF metadata instead. |
| src/query/frontend/ast/ast_storage.hpp | Adds user_functions_ intern table and AddUserFunction helper. |
| src/query/cypher_query_interpreter.cpp | Copies user_functions_ through the AST cache clone path. |
| src/query/context.hpp | Adds resolved_user_functions to EvaluationContext. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Trigger::GetPlan copied the AST name tables into a fresh AstStorage but dropped user_functions_, and Trigger::Execute built its own ExecutionContext without calling ResolveUserFunctions. A trigger whose statement calls a query-module function therefore fell onto the per-call on-demand resolution path, and a concurrent module reload could swap the implementation between rows. Copy user_functions_ in GetPlan and resolve it into the trigger's EvaluationContext in Execute, matching PullPlan. Also correct the Function::user_function_id_ comment to reference EvaluationContext.
- collapse ResolvedUserFunctions to a single vector<user_func> (drop the parallel callables/modules invariant; modules were never indexed) - SelectedFunction wraps a variant<const func_impl*, user_func> instead of an out-param whose returned pointer aliased a caller local - MG_ASSERT the resolved-slot invariant (this branch is user-defined-only) - assign user_function_id_ in the planner-v2 AST builder too, so v2 uses the resolved table instead of the per-call fallback - drop the now-dead module_fwd.hpp include and a stale module-lock comment Tests: function removed by reload raises, multiple user functions in one query, and a trigger calling a module function across a reload.
DavIvek
marked this pull request as ready for review
July 22, 2026 12:03
|
imilinovic
approved these changes
Jul 23, 2026
DavIvek
added
CI -build=coverage -test=clang_tidy
and removed
CI -build=coverage -test=core
DavIvek
enabled auto-merge
July 23, 2026 12:08
Contributor
Author
Tracking
Standard development
CI Testing Labels
Documentation checklist
|
seuros
pushed a commit
to seuros/memgraph
that referenced
this pull request
Jul 28, 2026
…queries (memgraph#4482) ## Problem `CALL mod.proc() YIELD ...` queries were non-cacheable, so every execution re-parsed and re-planned. Parsing a `CALL` bakes the procedure's result fields (including `YIELD *`), its `is_write` classification and its `required_privilege` into the AST. The last two are consumed before execution starts, to pick the storage accessor and to authorize, so a reload needs a re-parse, not just a re-plan. Procedure counterpart to memgraph#4461, which made query-module functions cacheable via per-execution binding. ## Changes - **Cache procedure queries.** `ModuleRegistry` gains an atomic generation counter, bumped on any change to the loaded module set. Cached ASTs and plans are stamped at build and rebuilt when stale, so a reload re-parses and re-plans on next execution. The plan inherits the AST's generation, so a reload between parse and prepare cannot stamp a stale plan as fresh. - **Scope invalidation to dependent queries.** A global stamp invalidated every cached AST and plan on any module load, including queries naming no module. `AstStorage` now records `call_procedures_` next to `user_functions_`, and `DependsOnModules()` gates the check. Queries touching no module survive a reload untouched, and procedures and module functions end up under one rule. - **Two crash paths closed.** `CallProcedureCursor` dereferenced a past-the-end iterator when a yielded field was missing. The user-function slot bounds check was an unconditional assert, aborting the process in release. Both now throw. - **Derive a plan's required indices once**, at construction, instead of re-walking the plan on every cache hit. Benefits all cached queries. ## Not addressed Triggers have no generation check and never re-parse, so a signature or privilege change is picked up only on recreate. Pre-existing; needs trigger re-parse, which touches durability. --------- Co-authored-by: Gareth Andrew Lloyd <gareth.lloyd@memgraph.io>
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.
Problem
Any query that calls a query-module (user-defined) function — e.g.
RETURN mymodule.myfunc(x)— was marked non-cacheable at parse time (is_cacheable &= !IsUserDefined()). The whole query was re-parsed and re-planned on every execution, which is expensive for large queries (#1611: ~200ms re-plan per run for a query containing a magic function).The gate existed for a real reason: a cached plan baked the function's callable and a
shared_ptrto its module into theFunctionAST node. Caching that would (a) run stale code after a module reload and (b) pin the module —mg.loadrefuses to reload a module whileuse_count != 1.Fix
Resolve module functions per-execution instead of baking them into the plan:
Functionstores onlyis_user_defined_+ a slot index (user_function_id_); no module reference.AstStorage::user_functions_records the distinct module-function names (same shape aslabels_/properties_).PullPlanresolves them once per execution intoEvaluationContext::resolved_user_functions, which keeps the backing modules alive for the query's duration only.PrimitiveLiteralExpressionEvaluator).Net effect: module-function queries are cached (no re-parse/re-plan), while reload stays safe — idle cached plans hold no module reference (never block a reload), and the next execution re-resolves and picks up new code automatically.
The
CALL proceduregate is intentionally left in place; itsYIELDresult-name binding is a separate concern.Closes #1611