Rebuild in resort(), stop row-level writes from resorting, drop finalize_changes

[roed-math]

Fourth of the rc3 stabilization PRs: B0, B1, B2, B4, B6.

Stacked on #140–#142; the diff contains their commits too.

B1 — resort() rebuilds instead of renumbering in place

resort() was a disabled no-op: print("resorting disabled"); return None. The
implementation below that early return renumbered every id with an in-place
UPDATE, which stalls replication and — the part that matters — leaves the rows
in their old physical order, so the whole point of id-ordering (sequential
primary-key reads) was never achieved.

It now rebuilds. The table is dumped without ids and reloaded; reload assigns
ids 1..N in sort order via a new private _generate_sorted_ids, a physical
rebuild (INSERT ... SELECT ... ORDER BY into a fresh table that replaces the
original), not an in-place UPDATE. Everything else — primary key, indexes,
constraints, grants, counts/stats companions, ANALYZE, and the _oldN backup
— comes from reload's one replacement path, so resort() adds almost no
orchestration of its own. The ids change, and this is documented on the
method and in DataManagement.md.

test_resort.py checks the rows end up in sort order on disk (via ctid,
not just logically) — an in-place renumber would fail that — plus contiguous ids
from 1, the backup, rebuilt indexes, the already-ordered short-circuit,
force=, descending sorts, the empty table, and that reload does not recurse
back into public resort().

Avoiding recursion

reload and non-inplace update_from_file call _generate_sorted_ids directly,
never public resort(). The renumber moved to before the keys are rebuilt,
since it replaces the table and the table must have no pkey/indexes at that
point.

B2 — row-level writes no longer resort

Per your decision, resort=True raises (pointing at resort()) on
insert_many, update, copy_from and in-place update_from_file;
resort=False is unaffected. The replacement operations that rebuild the whole
table anyway — reload, rewrite, non-inplace update_from_file — still
establish order. rewrite's resort now defaults to the replacement path only
(so an in-place rewrite doesn't trip the guard), and a staged table's in-place
writes drop the request. An in-place update that changes a sort key records
out_of_order rather than pretending it reordered.

B4 — finalize_changes() removed

It was a documented public no-op. The write methods already leave total, the
order flag and stats_valid correct on return — which test_write_invariants.py
now pins for every path (total exact even without stat saving; the B0 order-flag
rules per write; stats invalidation and restoration).

B6 — id-order audit

scripts/audit_id_order.py: read-only, streams each id_ordered table in sort
order through a server-side cursor (constant client memory) and reports OK /
MISMATCH / error. --all or named tables. test_id_order_audit.py includes a
table that lies about being ordered, to prove the audit catches what the flag
hides.

Not done here, by design

Track J (auditing and rebuilding the 46 production tables) is operational and
yours; this PR ships the tooling it needs. The runbook is in the handoff.

Full suite: 1361 passed, 36 skipped, 1 xfailed. Ruff and the -W docs build
clean.

🤖 Generated with Claude Code

[read-the-docs-community]

Documentation build overview

📚 psycodict | 🛠️ Build #33922249 | 📁 Comparing eb4e33c against latest (a161f5b)

  🔍 Preview build  

13 files changed · ± 13 modified

± Modified

• DataManagement.html
• genindex.html
• api/database.html
• api/statstable.html
• api/table.html
• api/validation.html
• _modules/psycodict/base.html
• _modules/psycodict/database.html
• _modules/psycodict/dbdiff.html
• _modules/psycodict/searchtable.html
• and 3 more...

[roed-math]

Pushed 7eb9eb7 (plus a documentation follow-up in a39070f), addressing the four correctness problems from the review. The architecture is unchanged: resort() is still an explicit dump/rebuild/swap, row-level writes still reject resort=True, replacement writes still establish order while building their replacement, finalize_changes() stays removed, and the audit stays read-only and streaming.

1. scripts/audit_id_order.py was unrunnable, and left transactions open

main() did from psycodict import db, which the package does not export, so both documented commands failed before auditing anything. It now builds a PostgresDatabase() from the usual PSYCODICT_CONFIG / config.ini path, and takes a database as an argument so the tests audit their own fixture connection rather than opening a second one.

The named-cursor read also never ended its transaction. A successful multi-table run held one MVCC snapshot from the first table until process exit, and after a PostgreSQL error the connection stayed in an aborted transaction, so every table audited afterwards reported InFailedSqlTransaction instead of being audited: one broken table turned a whole --all run into a wall of false errors. Each table now audits in a bounded read transaction, ended on every exit path (including the early MISMATCH return, which leaves the streaming generator suspended and so only closes the cursor if the audit closes it deliberately). Cleanup never masks the audit's own error, and the connection is left IDLE.

id_ordered with no configured sort is now ERROR rather than an OK obtained by ordering on id itself, which would have passed by construction.

2. resort=False no longer means "the sort was not touched"

update_from_file used one variable for two facts, and the sort-column check ran only when resort is None, so an explicit resort=False suppressed _break_order() along with the rebuild. That reached further than it looks: rewrite(inplace=True) forces resort=False, and the staged wrapper forces it on every staged write. So a staged sort-key rewrite left staged._out_of_order false, _staged_commit carried that false through _staged_swap_in, and the replacement went live asserting that ORDER BY id was equivalent to the configured sort when it was not — something search code is allowed to act on.

sort_changed is now computed from the file's columns independently of the option. resort=False means do not rebuild; a write that touches a sort column records the broken ordering either way, on the in-place path, on the unrebuilt replacement path (where the flag survives the swap, which only ever clears it via _set_ordered), and through a staged commit. Since rewrite dumps every search column and func may return anything, an unrebuilt rewrite always marks the table out of order. rewrite(inplace=True, resort=True) now raises before func runs over the table rather than after it and the dump.

3. _generate_sorted_ids follows the relation it is rebuilding

It built its column list from self.search_cols, but under reload(adjust_schema=True) the _tmp relation comes from the file header. An added column was omitted from the insert into the _resort clone and came back NULL for every row; a removed column was still named and failed with an undefined column; and a metafile changing the sort got ids assigned by the sort it was replacing, followed by metadata claiming the new one.

The helper now reads the target relation's own columns from the catalog, and takes the sort to number by as an argument — reload passes the sort the metafile will install, not the one it is losing. Before the first destructive statement it checks that the relation has an id, has columns besides id (an id-only target is refused with an explanation instead of INSERT INTO t (id, ) SELECT ...), and has every intended sort column, so a data file and a metafile that disagree leave the relation as it was rather than half rebuilt. The pre-index placement, the physical INSERT ... SELECT ... ORDER BY, the backup behaviour and the cleanup on failure are unchanged.

4. Changed data never goes live vouched for by caches that describe the old rows

update_from_file(..., restat=False) invalidated nothing on either path, rewrite inherited that, and reload(..., restat=False) could swap changed search data in beside cloned or untouched counts and stats while keeping stats_valid = true. A count cached before such a write kept being served, which is exactly what #141 set out to stop.

The decision now lives in one place, _set_stats_validity, called in the same transaction as the update or the swap. The table ends up valid only when the caches that go live were built from the data that goes live with them: recomputed here (restat, with saving on) or supplied whole by an export carrying both countsfile and statsfile. Everything else — a partial cache set, companions merely cloned or left in place — is marked invalid. A metafile carries a stats_valid describing the database it was exported from; reload_meta replaces the whole meta_tables row, so that value is now overridden with a message rather than installed over caches this reload never built. total is untouched: it is maintained separately on every write and stays exact.

Tests

Regression tests for all four, in test_id_order_audit.py, test_write_invariants.py, test_staged_writes.py, test_resort.py and test_stats_validity.py — behaviour through the public methods, not just the private helpers. Each new test was run against the reviewed head (5072dd6) to confirm it fails there: 5 for the audit, 6 for the order bookkeeping, 5 for the schema-driven rebuild, 6 for stats validity. The remaining new tests pin behaviour that had to be preserved — the constant-memory named cursor, a resorted replacement still being marked ordered, an export carrying its own caches staying valid, and a failed swap leaving no validity behind.

Full suite green (1397 passed, 36 skipped, 1 xfailed) over three consecutive runs on PostgreSQL 18, ruff check clean, and sphinx -W --keep-going clean. CHANGELOG.md and DataManagement.md updated for the two behaviour changes.

One place the stats rule is deliberately pessimistic

resort() renumbers the rows without changing them, so its cached counts would still be accurate — but it goes through the replacement path, which can only tell the caches match when it rebuilt them, and it rebuilds them only with saving on. So a resort on a table without saving now ends stats_valid = false and wants a refresh_stats(). That is the safe direction (a miss recomputes; a false true serves a wrong answer), so I left the behaviour alone and made it a documented, tested quantity instead: a note in resort()'s docstring and the changelog, plus tests pinning both halves. Say the word if you would rather resort() carried "the data is unchanged" through to the swap and kept the flag.

Note for the stack: #144, #145 and #146 sit on top of this branch and will want a merge from it.

[roed-math]

Both re-review edges closed in eb4e33c. I reproduced each one against a39070f first, so these are confirmed rather than inferred.

1. Cache files that were never loaded no longer count as fresh

Confirmed exactly as described. With saving off, a reload given both cache files left the live counts describing the old data and marked them valid:

before: cached count of {"flag": True} = 67
after reload of a 5-row search file (saving=False, both cache files given):
  stats_valid = True   quick_count = 7   truth = 2

Freshness now asks what is actually going live, not what was named on the call:

loaded_both_caches = (
    countsfile is not None
    and statsfile is not None
    and self.stats.counts in tables
    and self.stats.stats in tables
)
stats_fresh = loaded_both_caches or (self.stats.saving and restat)

Membership in tables is the load-and-swap condition, so this covers the saving=False case you found and stays conservative for partial files and for a manual reload_final_swap(). Same probe now reads stats_valid = False, quick_count = None, count = 2.

On your open question — reject or ignore: I left it ignored but loud. reload now prints that the files are not being loaded and that the existing caches are being marked invalid, rather than dropping them on the floor. Raising instead would turn a call that is accepted today into a failure mid-release-candidate, and I would rather not do that to a downstream caller on my own initiative. Say the word and I will make it a hard error with a validation test instead.

2. A metafile claiming order with no sort is refused

Also confirmed: the reload numbered the rows by the sort being replaced, then installed sort = None, id_ordered = True, out_of_order = False — the state the audit reports as an error.

reload now raises before anything is rebuilt or swapped:

<metafile> claims id_ordered = true and out_of_order = false but installs no sort, so there is nothing for ascending id to follow. Give the metafile a sort, or let it record out_of_order = true.

One thing beyond the literal ask: the check does not depend on resorting. The metafile is read whenever one is supplied, not only when this reload renumbers, because the non-resorting path installs the same impossible row through reload_meta — a search file carrying ids resorts nothing and still reached it. There is a test for that path specifically.

I went with the validation rather than a sentinel default on _generate_sorted_ids. sort=None meaning "use the configured sort" is documented and is what every internal caller wants; the incoherent input is a property of the metafile, so rejecting it where the metafile is read keeps the helper's contract simple and fails earlier.

Only that combination is refused — a metafile that drops the sort and the id_ordered claim still loads, with a test pinning it.

Tests and validation

Six new tests. Four fail against a39070f (the two fixes, plus the no-resort path and an audit-level statement of the invariant); two are positive controls that pass on both and must keep passing — a partial cache pair still not trusted, and a sortless metafile that also clears id_ordered still loading.

Full suite green (1405 passed, 36 skipped, 1 xfailed) over three consecutive clean-database runs, ruff check clean, sphinx -W --keep-going clean. CHANGELOG.md, DataManagement.md and the reload docstring updated for both behaviours.

[roed314]

GPT signed off.