fix: sql --commit executed DML twice and reported COMMITTED on aborted transactions (PGC-104)#17
Merged
Conversation
…ED (PGC-104) Root cause: the single-DML-no-RETURNING write path executed the user statement twice in one transaction — once bare for the affected count, once RETURNING-wrapped for the sample. Both effects committed (2 rows for a 1-row INSERT). When the second execution hit a unique violation the transaction aborted; the wrap error was silently swallowed (Err(_) => Ok(None)) and COMMIT — which Postgres downgrades to ROLLBACK on an aborted tx without raising — reported success. Net: false "COMMITTED" with zero rows written, the worst failure for a trust tool. Fix: - Execute each user statement exactly once. Single DML without RETURNING now uses one writable-CTE query that yields both the count and the sample (count(*) + json_agg over the CTE; the CTE materializes once). - No swallowed errors: any DB error inside the transaction rolls back and propagates, framed "ROLLED BACK — nothing changed" → exit 10 (human + JSON), never COMMITTED. - Harden COMMIT against deferred-constraint silent-rollback: run SET CONSTRAINTS ALL IMMEDIATE before COMMIT and propagate any violation. - Built: rewritten execute_in_tx / new execute_dml_with_sample + sample_from_json + rolled_back helpers in src/commands/sql_cmd.rs; 7 non-idempotent regression tests (INSERT vs UNIQUE) in tests/commands/sql.rs. - Validation: cargo fmt --check clean, clippy clean on changed files, cargo test --no-fail-fast 767 passed / 0 failed (baseline 760 + 7). Live verified against PG18 mirroring the solitaire repro: single execution, honest ROLLED BACK + exit 10 on abort, data unchanged. - Notes: every user-SQL path now executes once (EXPLAIN cost-gate plans only, no ANALYZE). Board status update left to orchestrator.
- Fixed: dry-run/commit sample columns were alphabetized (to_jsonb + sorted-map key inference). Switched the CTE wrap to row_to_json (text, order-preserving) and an explicit ordered column-name array so previews show columns in statement/table-definition order. - Fixed: SKILL.md didn't reflect the new behavior — a dry-run of a constraint-violating statement now fails (exit 10, ROLLED BACK) rather than reporting a clean DRY RUN. Documented exactly-once + honest abort (including deferred constraints) in the writes section. - Added: regression test asserting the dry-run sample preserves table-definition column order. - Noted: empirically verified exactly-once via an insert-trigger side-effect counter (3-row insert -> 3 trigger fires) and nextval observation; deferred INITIALLY DEFERRED unique violation under --commit caught by SET CONSTRAINTS ALL IMMEDIATE (exit 10, table unchanged, never COMMITTED). ON CONFLICT, UPDATE ... FROM, volatile functions, multi-statement and non-transactional error paths all execute once / report honestly.
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.
Critical fix to the guarded gateway, found via live dogfooding (solitaire data migration): the single-DML sample path executed the user statement twice in one transaction (wrapped for samples + bare for count), and an aborted transaction's COMMIT was silently downgraded by Postgres to ROLLBACK while pgcrate printed COMMITTED.
Not in any published release (crates.io 0.6.0 predates the gateway). Blocks 0.7.0.