fix(sql): evaluate now() / current_timestamp at plan time (#33)#35
Merged
farhan-syah merged 3 commits intomainfrom Apr 15, 2026
Merged
fix(sql): evaluate now() / current_timestamp at plan time (#33)#35farhan-syah merged 3 commits intomainfrom
farhan-syah merged 3 commits intomainfrom
Conversation
…n time Introduce `nodedb-sql/src/planner/const_fold.rs` — a plan-time constant folder that evaluates literal expressions and registered zero-arg scalar functions (`now()`, `current_timestamp`, `date_add(now(), '1h')`, etc.) by dispatching through the shared `nodedb_query::functions::eval_function` evaluator. Wire the folder into the `INSERT`/`UPSERT` VALUES path (`dml.rs`) and the SELECT projection path (`select.rs`), replacing the inline duplicate `eval_constant_expr` logic in both. All three paths now share a single evaluator, closing the drift that caused `now()` in a VALUES clause to store the literal string `"now()"` instead of the current timestamp. Semantics follow the Postgres contract: `now()` and `current_timestamp` resolve once per statement. Folding at plan time satisfies this and eliminates per-row runtime dispatch overhead. Functions absent from the `FunctionRegistry` fall back to the existing string passthrough so no other callers are affected.
The `parse_inline_value` helper in the pgwire DDL path was converting any unrecognised token directly to a string, causing `now()` and `current_timestamp` in UPSERT payloads to be stored as their literal text rather than the current timestamp. Introduce `try_eval_scalar_function` which routes bare SQL keywords (`current_timestamp`, `current_date`, etc.) and parenthesised call expressions through the same `nodedb_sql::planner::const_fold` evaluator used by the INSERT/UPSERT VALUES planner path. Unknown names fall through to the existing string behaviour.
Add `NodeHandle::metadata_group_leader()` which reads the observed metadata-group leader id from the node's local Raft state, returning `0` while an election is still in progress. Wire a leader-stability barrier into `ClusterHarness::spawn_three()` that polls until every node reports the same non-zero leader id before returning control to the test. This closes the race where CPU pressure delayed the initial Raft heartbeats past topology convergence, producing `not leader (leader hint: None)` errors on the first DDL or descriptor-lease call issued by the test. Also add the `pgwire_gateway_migration` binary to the nextest cluster test group so it runs serialised with the other 3-node integration tests and does not compete for threads.
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
Fixes #33:
now()/current_timestampinINSERT/UPSERTVALUESwere stored as the literal string"now()", surfacing as1970-01-01T00:00:00(epoch 0) when the target column wasTIMESTAMPin a STRICT collection — and as the literal string in schemaless / KV / columnar engines. BareSELECT now()also returnednull. The bug was three independent code paths (planner SELECT projection, planner DMLVALUES, pgwire UPSERT fast-path) each silently failing to dispatch zero-arg scalar functions to the existingnodedb_query::functions::eval_functionevaluator.Fix
A single shared constant-folding evaluator routes all three paths through the same scalar-function dispatcher. Postgres-compatible semantics:
now()snapshots once per statement (matchesCURRENT_TIMESTAMPin the SQL standard).nodedb-sql/src/planner/const_fold.rs(new):fold_constant(expr, registry)recursively folds literals, unary/binary arithmetic, and registry-gated scalar function calls vianodedb_query::functions::eval_function. Aggregates/window functions rejected; unknown functions returnNoneso callers keep their fallbacks. Plusdefault_registry()/fold_constant_default()for call sites that don't thread a registry.nodedb-queryadded as dep ofnodedb-sql.nodedb-sql/src/planner/select.rs:eval_constant_exprcollapsed to a one-line delegate. Fixes bareSELECT now().nodedb-sql/src/planner/dml.rs: Function fallthrough inexpr_to_sql_valuenow folds viaconvert_expr+fold_constant_defaultbefore the legacy string fallback. Fixes the planner VALUES path for STRICT, schemaless, KV, and COLUMNAR engines.nodedb/src/control/server/pgwire/ddl/sql_parse.rs:parse_sql_value(the UPSERT fast-path's hand-rolled tokenizer that bypasses sqlparser entirely) now detects bare SQL identifiers likecurrent_timestampvia registry lookup andname(args)forms by re-parsing through sqlparser + folding. Fixes the UPSERT path.Cluster harness fix (in scope: would otherwise be hidden by the new test coverage)
Adding new integration tests surfaced two latent flakes in the multi-node test harness:
.config/nextest.toml:pgwire_gateway_migrationwas missing from theclustertest group despite spawning a 3-node cluster, so it ran in parallel with the unit-test pool and missed its 10s SWIM convergence deadline. Added.nodedb/tests/common/cluster_harness/{cluster.rs,node.rs}:spawn_three()previously returned as soon astopology_size == 3and rolling-upgrade compat exit converged, but said nothing about Raft election state. Under host load (e.g. when one cluster test exits and the unit-test pool ramps back up), the firstacquire/proposecould race the initial heartbeat window and surface asraft error: not leader (leader hint: None). Now waits for every node to report the same non-zero metadata-group leader id viacluster_observer.group_status— symmetric to the existing rolling-upgrade-compat wait. (Earlier draft used the unsetraft_status_fnstub onSharedState, which always returnedNoneand broke every cluster test; corrected before commit.)Test plan
scripts/test.sqlcovering STRICT, schemaless, KV, COLUMNAR, and bare SELECT — all show real 2026 timestamps post-fix (was epoch 0 / literal"now()")const_fold.rs(now/current_timestamp/literal arithmetic/unknown function/column ref)cargo nextest run --no-fail-fast: 4939/4939 passed, 0 failed, 3 known-flaky cluster retries within nextest's retry budget (down from 8 flakies + 1 hard fail before the harness fix)cargo clippy --all-targets -- -D warnings: cleancargo fmt --all --check: clean