feat(postgres): report the executor's live step from apply progress - #1336
feat(postgres): report the executor's live step from apply progress#1336Kiran01bm wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The terminal progress publish path uses a cancellable apply context for reading tracker position, which can cause terminal step/statement metadata to fall back to the planned “step 1” when the apply ends due to cancellation/timeout.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves PostgreSQL apply progress reporting by wiring pg-sprite’s progress.Tracker through the apply execution path, allowing Progress to report the executor’s current step number, total steps, and the (sanitized) SQL statement for the in-flight step rather than always showing step: 1.
Changes:
- Thread a per-apply
progress.Trackerthrough optimistic execution and overlay tracker-derivedstep/steps_total/statementonto progress snapshots. - Preserve terminal apply progress by folding the tracker’s last-known position into the final published result, and avoid re-reading tracker state after terminal publish.
- Expand/update unit + integration tests to assert step/statement reporting and to refine invalid-index outcome messaging (including a new “other table” refusal test).
File summaries
| File | Description |
|---|---|
| pkg/engine/postgres/postgres.go | Adds trackedApply and stores tracker + logger alongside published progress state for each apply. |
| pkg/engine/postgres/apply.go | Creates/threads a tracker through execution, overlays tracker position into Progress, and refines invalid-index refusal/operational classification. |
| pkg/engine/postgres/apply_test.go | Adds unit tests for tracker-backed step/statement reporting and extends invalid-index refusal/decision coverage. |
| pkg/engine/postgres/postgres_integration_test.go | Updates integration assertions for terminal step/statement metadata; adds new invalid-index-on-other-table refusal test. |
| go.mod | Bumps github.com/block/pg-sprite to v0.3.1. |
| go.sum | Updates checksums for the pg-sprite bump. |
| e2e/consumermodule/go.mod | Updates consumer module indirect pg-sprite version to v0.3.1. |
| e2e/consumermodule/go.sum | Updates checksums for the pg-sprite bump in the consumer module. |
Review details
- Files reviewed: 6/8 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
f98b369 to
b1a1807
Compare
Progress reported step 1 for the whole apply because nothing observed pg-sprite's step transitions. The engine now hands each apply a pg-sprite progress tracker, runs the executor's tracked variants, and reads the step position and statement back on every poll; terminal results fold in the final position so a failed create names the step that failed.
The tracker read behind Progress queries the session pg-sprite reserves for a concurrent build's failure verdict, so it now runs on its own short-deadline context detached from the caller's cancellation — the same guard pg-sprite applies to every other use of that session. Tests cover the Apply-level tracker wiring, the read's error contract, and the poll's last-known-position fallback; drive-layer comments no longer describe the engine probe as purely in-memory.
b1a1807 to
891f9cd
Compare
PostgreSQL apply progress now reports the step pg-sprite's executor is actually running, and the statement text of that step, instead of a fixed
step: 1.Why
The engine published progress only at accept and at the terminal outcome, so a three-step create set reported
step 1 of 3from start to finish and a create that failed on its second index still said step 1. pg-sprite's executor already records every step it starts on aprogress.Tracker; the engine just never gave it one.What
Applycreates oneprogress.Trackerper apply and threads it throughexecuteOptimistic→ExecuteCreateWithProgress/ExecuteNativeWithProgress/BuildIndexConcurrentlyWithProgress.trackedApply{result, tracker, logger};publishProgressupdates only the result.Progresssnapshots the record under the engine lock, releases it, then overlaysstep,steps_total, and a sanitizedstatementfrom the tracker. The tracker read can touch the server's index-build progress view, so it never runs undere.mu; a failed read keeps the last-known position and logs under the apply's identifiers.step 1 / steps_totalfrom the plan until the executor reports.Not in scope, tracked as follow-ups: plumbing
step/steps_total/statementthrough the drive layer andGET /api/progress/apply/{apply_id}so an operator surface actually renders them (today they terminate in the engine's progress record), and theattemptand concurrent-build work counters (blocks/tuples) the tracker also exposes.Before / after