Skip to content

add pull command for declarative schema exports - #67

Merged
Kiran01bm merged 2 commits into
mainfrom
kiran01bm/pull-cli
Sep 1, 2026
Merged

add pull command for declarative schema exports#67
Kiran01bm merged 2 commits into
mainfrom
kiran01bm/pull-cli

Conversation

@Kiran01bm

Copy link
Copy Markdown
Collaborator

Add pg-sprite pull: export every table in a schema as desired-state .sql files, one file per table, with per-table outcomes.

Why

Onboarding a live database to the declarative workflow needs a baseline export. The engine surface already exists — the introspect→render path (#52) and the render refusals for constructs that cannot be expressed as desired state (#55) — but there was no command driving it across a whole schema.

What

  • PullCmd (Kong): pg-sprite pull --url=… [--schema public] [-o|--out schema], sharing the existing DB connection flags.
  • Enumerates ordinary and partitioned tables in the schema from pg_catalog, then per table: Introspect → Render → write <table>.sql (create-only, refuses to overwrite existing files).
  • Not fail-fast: each table reports PULLED/REFUSED/ERROR plus a summary line; render refusals map to the refusal exit code, any hard error exits non-zero, all-success exits zero.
  • Unit tests for the orchestration (aggregation, overwrite refusal, unsafe table names) and a PostgreSQL integration test covering successful pulls and a partitioned-parent refusal, with rendered files verified against the linter.

Before / after

Before                                   After
┌───────────────────────────────┐       ┌───────────────────────────────────┐
│ live DB ──▶ Introspect/Render │       │ pg-sprite pull                    │
│ (engine-only; per-table calls │       │  └─ for each table in schema:     │
│  wired up by hand, no CLI)    │       │     Introspect ─▶ Render ─▶ file  │
│                               │       │     PULLED / REFUSED / ERROR      │
│ no baseline export path       │       │  └─ summary + typed exit code     │
└───────────────────────────────┘       └───────────────────────────────────┘

Enumerates every table in one schema and drives each through the
introspect -> render path, one desired-state file per table, reporting
per-table outcomes (pulled / refused / error) instead of failing fast.
Review found INHERITS children exported as silently lossy baselines
(flattened columns, zero diff). Introspection now records inheritance
edges both ways and Render refuses both sides. Also: extension-owned
tables excluded, missing schema errors instead of exiting 0, truncated
files removed on Close failure, case-collision detection, exhaustive
status switch, partition children filtered, create-only contract
documented.
@Kiran01bm Kiran01bm changed the title Add pull command for declarative schema exports add pull command for declarative schema exports Aug 31, 2026
@aparajon

Copy link
Copy Markdown
Collaborator

🤖 Adversarial correctness review, requested by @aparajon and performed by their agent. Reviewed at head 39aff8d2.

Verdict: pull is well-built — create-only writes, per-table isolation of failures, a fail-closed unknown-status branch, and a distinct exit code for refusals. Two things undercut the project's central promise that a refusal is always preferred to a silently wrong result: Diff has no inheritance guard even though this PR adds the model fields and the docs claim, and pull reads each table in its own transaction so a concurrent DDL can leave behind a baseline the same run would have refused a moment later.

The diff path does not refuse classic inheritance

docs/capabilities.md now lists tables that "participate in classic table inheritance" among the shapes the declarative door answers with "a typed refusal rather than a silently lossy description". That is true of Render, which this PR gates on both directions. It is not true of Diff. pkg/schemadiff/diff.go fails closed on PartitionKey, IsPartition and Unlogged mismatches with ErrUnsupportedChange, and the new InheritsParents / InheritanceChildren fields are not compared at all.

Reproduced against a live server — parent (id bigint NOT NULL), child (note text) INHERITS (parent), diffed against the desired file a user would hand-write for child:

live child columns: [id note]  parents=[…parent]
Diff err=<nil>
planned change: {SQL:ALTER TABLE "…"."child" DROP COLUMN "id" Kind:drop-column Destructive:true}

The inherited column is invisible in the desired file — there is no grammar for INHERITS — so it reads as a column the user removed, and the plan is a destructive drop that PostgreSQL will refuse at execution anyway (cannot drop inherited column). The parent side is worse in kind if not in blast radius: an ADD COLUMN planned against a parent cascades to every child without the plan saying so. The guard is three lines beside the two already there, and the model now carries exactly the facts it needs.


pull is N+1 snapshots, not one

Introspect opens and rolls back its own transaction per call, and listTables runs outside all of them, so a pull of n tables observes n+1 independent read-committed snapshots. The interesting interleaving is not a dropped table — that surfaces as ErrTableNotFound and gets reported. It is a relationship created between two reads: add a foreign key from bar to foo after foo has been exported and before bar is, and foo.sql lands on disk as a clean baseline for a table that the same command refuses one iteration later, while bar is reported as REFUSED. The operator sees a partial success and a file that is silently wrong about the topology it belongs to.

The fix is small because the shape already exists: introspectInTx takes a pgx.Tx, so exporting a variant that accepts one and running the whole pull — listTables included — inside a single REPEATABLE READ, read-only transaction makes the exported set a coherent point-in-time baseline. That is the property a declarative baseline needs, more than any single file's correctness.


A cancelled pull reports as if it were complete

pullTables checks ctx.Err() at the top of each iteration, appends one error result for the table it was about to attempt, and breaks. Every table after that appears nowhere — not in the per-table lines, not in the counts. TestPullTablesStopsAfterContextCancellation pins that behavior: three tables in, two results out. So an interrupted export prints Summary: 1 pulled, 0 refused, 1 errors and nothing distinguishes it from a schema that only had two tables. The remaining count is already known at that point; carrying it into the summary (… , 47 not attempted) costs a field and removes the ambiguity.


Action items

  1. Fail closed on InheritsParents / InheritanceChildren in Diff, beside the partition and persistence guards, so the capability matrix's claim holds for both declarative doors.
  2. Run the whole pull inside one REPEATABLE READ read-only transaction, listTables included, so the exported set is a single snapshot.
  3. Report the tables a cancelled pull never attempted, rather than ending the report early.
Verified (tried to break, couldn't)

The create-only guarantee holds at the syscall, not by a prior existence check: O_WRONLY|O_CREATE|O_EXCL makes the refusal atomic against a concurrent writer, and both partial-write paths remove the file and join the close/remove errors rather than leaving a truncated baseline behind. I went looking for a way around the in-run case-collision guard, since it only tracks names seen in the current run, and could not find one — a file left by an earlier run on a case-insensitive filesystem still fails the O_EXCL open, so the on-disk case is covered by the syscall rather than the map, and the map covers the case the syscall cannot see (two tables in one run before either file exists).

tableOutputPath rejects anything whose filepath.Base is not itself, which closes traversal (../outside) and separators without an allowlist that would have to guess at legal identifier characters. listTables correctly excludes partitions (NOT relispartition) so a partition is never attempted separately from its parent — the integration test asserts metrics_2026 appears nowhere in the report — and excludes extension-owned relations through pg_depend deptype = 'e', which is the right catalog test rather than a name heuristic. relkind IN ('r','p') keeps views and sequences out, and Introspect re-checks relkind, so the two do not have to agree.

The unknown-status branch in writePullText is genuinely fail-closed — it returns an error rather than printing an unlabeled line — and it is tested. pullResultsError's precedence is right: an operational error outranks a refusal, and refusal maps to verdict.ErrRefused, which cmd/pg-sprite/main.go turns into exit code 2, so a caller can distinguish "some tables are not supported yet" from "the export failed" without parsing text. pullOneTable distinguishes an introspection failure from a render refusal through a private renderRefusal wrapper with Unwrap, so the refusal classification survives errors.As while the underlying typed sentinel still reaches a caller using errors.Is — the report and the exit code stay in agreement.

The inheritance query is correct on the subtlety that matters: declarative partitions share pg_inherits, and both arms filter on NOT child.relispartition — the child side, which is where relispartition actually lives — so a partitioned parent keeps its existing partition refusal instead of acquiring a second, wronger one. Both directions are read in one query with a discriminator column, ordered deterministically, and the scan's default arm returns an error rather than silently dropping a row. The schema-existence check runs before MkdirAll, so a typo'd --schema leaves no directory behind, and the test asserts exactly that.

This review was generated by Claude Code (claude-opus-5).

@aparajon

Copy link
Copy Markdown
Collaborator

🤖 Second-pass review through two lenses — OSS adoption ease and SchemaBot integration — requested by @aparajon and performed by their agent. Reviewed at head 39aff8d2. The correctness pass is above.

Verdict: pull is the command a new adopter runs first, and it is shaped well for that — one file per table, create-only, a readable per-table report, and an exit code that separates "not supported yet" from "failed". Two things get in the way: the limitations table now contains entries that are not refusals, in a table whose opening sentence promises they all are; and pull is the only command in the tree without --json, which is the one an automation wrapper most needs.

Adoption lens

The limitations table just acquired two silent losses, and its own introduction says there are none. The table opens by promising every listed shape is answered "with a typed refusal — never a silently wrong or incomplete result". The two new rows do not work that way:

| Table and column comments | Comments are metadata outside the table-shape model. Desired files and exports do not carry them … |
| Storage parameters (fillfactor, etc.) | Storage parameters are not represented in desired files or exports … |

Both are dropped from an export without any refusal, warning, or report line. Documenting them is the right instinct and I would not want them removed — but they belong under a heading that says "silently not carried", not inside the table that promises the opposite. This matters most precisely at the moment pull exists for: an adopter exports a baseline, commits it, and only discovers months later that fillfactor and their column comments left the managed model on day one. A short "Not carried by exports" section, and a line in pull's own output when a table had comments or non-default storage parameters, would make the loss visible when it happens rather than when it bites.

The README row promises more than the command delivers. | pull | required | Introspect each supported table in a schema and create one desired-state file per table; existing files are never overwritten | — "each supported table" is doing a lot of quiet work. On a realistic schema, foreign keys alone (refused in both directions) will refuse a large fraction of the tables, and the command exits 2. An adopter's first run producing a partial baseline and a non-zero exit is fine behavior and bad first impression, purely because nothing set the expectation. One clause — "tables that the declarative model cannot express are reported as refusals and the command exits 2" — turns a surprise into a documented outcome.

The flag help is the best-written part of this and should be the model for the docs. Output is create-only: a second run into a populated directory fails per table; delete or move existing files to refresh them. — that is exactly the sentence someone needs, in exactly the place they hit the problem. The README row and docs/capabilities.md are noticeably less direct about the same behavior.

SchemaBot integration lens

pull is the missing onboarding primitive, and the file layout matches. SchemaBot's declarative schema root for a PostgreSQL database is a directory of per-table .sql files; pull --schema <s> --out <dir> produces exactly that, and the integration test lints every exported file and asserts zero errors — which is the property that actually matters, since SchemaBot will parse these files through the same front door. --schema covers databases that do not use public. Create-only is the right default for a tool that will be pointed at a directory already under version control.

pull is the only command in the tree with no --json. migrate, diff, lint, suggest and status all carry one (cli.go lines 109, 147, 178, 191, 201). An onboarding wrapper that wants to know which tables were refused, and why, has to scrape PULLED , REFUSED , ERROR prefixes and split on : — and the error text is free-form, so the reason is not machine-classifiable at all. The data is already structured: pullResult has table, path, status and error, and writePullText is already a pure function over []pullResult. A writePullJSON beside it, keyed on the same pullStatus values, is a small addition that makes the difference between "a human runs this once" and "onboarding automation can act on it". Given the refusal set is the interesting output for a partially-supported schema, I would treat this as the highest-value follow-up of the two lenses.

The refusal surface is the real gate on PostgreSQL onboarding, and this PR makes it legible for the first time. Before pull, finding out which tables in a schema the declarative model can express meant trying them one at a time. Running pull against a candidate database now answers that in one command, with a summary line that is a genuine readiness signal. That is worth saying plainly in the docs, because it reframes a partial result from a failure into the intended use: pull is the survey, not just the exporter.

Action items

  1. Move the comments and storage-parameter rows out of the typed-refusal table into a section that says they are silently not carried, and consider reporting them per table in pull's output.
  2. Add a clause to the README pull row about refusals and exit code 2, so a partial first run reads as expected behavior.
  3. Add --json to PullCmd, emitting the []pullResult set the text report already renders.
  4. Say in the docs that pull doubles as the readiness survey for managing a schema declaratively.

This review was generated by Claude Code (claude-opus-5).

@aparajon aparajon left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Approving on @aparajon's behalf after the adversarial correctness review above. The findings there are yours to pick up as follow-ups — flagging them, not gating on them.

This stamp was left by Claude Code (claude-opus-5).

@Kiran01bm
Kiran01bm marked this pull request as ready for review August 31, 2026 21:35
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@Kiran01bm
Kiran01bm merged commit 910af25 into main Sep 1, 2026
14 checks passed
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.

2 participants