Skip to content

Validate function graph operands once - #338

Closed
Pino de Candia (pinodeca) wants to merge 3 commits into
pinodeca/batch-node-insertsfrom
pinodeca/validate-operands-once
Closed

Validate function graph operands once#338
Pino de Candia (pinodeca) wants to merge 3 commits into
pinodeca/batch-node-insertsfrom
pinodeca/validate-operands-once

Conversation

@pinodeca

Copy link
Copy Markdown
Contributor

Stacked PR

This PR is not intended to merge as-is. It is temporarily based on #337 (pinodeca/batch-node-inserts) so Step 5 can be reviewed while Step 4 is still in review.

After #337 merges, this branch will be rebased onto main and the PR's base branch will be changed to main.

Summary

  • avoid reparsing an increasingly large left accumulator in df.seq(), df.join(), and df.race() when it is the exact output of the previous binary composition in the same backend
  • continue validating and normalizing newly arrived operands, including non-future-helper misuse and malformed Durofut envelopes
  • retain path-aware whole-graph validation in flatten_graph for corrupt accumulators
  • add regression coverage for sibling-key injection, helper attribution, accumulator error paths, and join/race folds
  • document the validate-on-entry boundary in docs/nested-graph-design.md

Decision Gate

This is the experimental Step 5 branch. It should be accepted only if Step 4-to-Step 5 measurements show a repeatable, material end-to-end improvement under realistic graph sizes. If the improvement is not material, Step 5 should not merge.

Validation

  • cargo fmt -p pg_durable -- --check
  • cargo clippy --features pg17
  • ./scripts/test-unit.sh (285 passed, 16 ignored)
  • ./scripts/test-e2e-local.sh 09_graph_and_validation (1 passed)

Flatten Durofut graphs through one iterative pass shared by df.start()
and df.explain(). Assign IDs as children are discovered so both callers
consume the same FunctionNode representation without recursive walkers.

Validate node types, depth, and node count while flattening. Include
child paths in errors, preserve the existing df.nodes.query format, and
document why pre-order insertion relies on deferred foreign keys.
Insert graph nodes in bounded multi-row statements to reduce SPI
round trips while preserving legacy and current schema layouts.

Cover both SQL shapes and exercise a graph that crosses the batch
boundary without launching its activities.
@pinodeca

Pino de Candia (pinodeca) commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Performance results and recommendation

I ran two independent benchmark sessions on the same environment using PostgreSQL 17.10, pgrx 0.16.1, and release builds. The comparisons were incremental:

What the benchmarks did

Composition microbenchmark: constructed a left-deep sequence by repeatedly passing the accumulated serialized graph and one newly arrived plain-SQL operand to df.seq(). It measured graph construction and serialization only: no df.explain() and no df.start(). Each row retained 30 samples after 5 warmups. This is the workload directly targeted by #338 because it repeatedly exercises validation of the growing accumulator.

Start/persistence microbenchmark: loaded an already constructed, shallow-wide graph and called df.start(). It measured parsing, validation, flattening, instance reservation, node persistence, variable capture, and orchestration submission, while excluding graph composition. Calls ran inside transactions that were rolled back so rows and submitted work could not accumulate between samples. Each row retained 15 samples after 5 warmups. This is the workload used to isolate #337. Nested df.nodes INSERT calls were counted separately with pg_stat_statements.

End-to-end microbenchmark: constructed the left-deep sequence and passed it directly to df.start() in the same measured transaction. It therefore included both composition and the complete start/persistence path. This is the decision-relevant comparison for #338: it shows whether avoiding repeated accumulator validation materially changes the latency users observe when they build and start a realistic graph. It also retained 15 samples after 5 warmups.

What #337 changed

Compared with #335, #337 reduced the number of node INSERT statements from one per node to one per 1,000-node chunk. The measured counts were exactly 1, 1, and 2 INSERTs for 999, 1,000, and 1,001 nodes, and 5 INSERTs for 5,000 nodes.

That structural reduction produced a large end-to-end df.start() improvement for already constructed graphs:

Nodes #335 median #337 median Improvement
100 30.937 ms 9.269 ms 3.34x
1,000 272.070 ms 61.392 ms 4.43x
5,000 1,347.918 ms 256.265 ms 5.26x

This establishes #337 as the correct baseline for evaluating #338: after batching, persistence is much cheaper, so #338 must demonstrate an additional user-visible improvement rather than only a faster composition loop.

What #338 changed

Compared with #337, #338 produced a strong and highly repeatable composition-only improvement:

Operands Session 1 Session 2
100 26.3% faster 26.1% faster
250 32.0% faster 31.2% faster
500 32.4% faster 31.4% faster
1,000 39.7% faster 39.3% faster

The optimization is therefore real in the narrow loop it targets. Composition also remains measurable after #337: at 250 operands, composition took about 10 ms of a 46-47 ms end-to-end start.

However, the end-to-end comparison of #338 against #337 did not reproduce:

Operands Session 1 Session 2
10 0.1% faster 7.7% faster
50 7.8% faster 12.2% slower
100 7.1% faster 2.8% faster
250 8.6% faster 2.1% faster

At the realistic 100- and 250-operand sizes, the second session reduced the apparent improvement to 2-3%. The 50-operand result changed direction entirely. That spread indicates the incremental end-to-end effect is not materially larger than run-to-run noise.

The safety tests for #338 pass, including sibling-key injection handling, non-future-helper attribution, and path-aware corrupt-accumulator reporting. Its batching behavior also remains identical to #337. The concern is not correctness; it is whether the additional validation trust boundary buys enough user-visible performance to justify itself.

Conclusion: #338 is not worth merging. It consistently speeds up the composition microbenchmark, but the improvement does not carry through as a repeatable, material reduction in end-to-end df.start() latency when compared with #337. I recommend closing #338 and retaining #337's simpler validation behavior.

@pinodeca
Pino de Candia (pinodeca) force-pushed the pinodeca/batch-node-inserts branch 2 times, most recently from 0ff1570 to 87ee524 Compare August 5, 2026 21:22
@pinodeca
Pino de Candia (pinodeca) deleted the pinodeca/validate-operands-once branch August 5, 2026 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant