Fix the bugs found by the CI suite by roed-math · Pull Request #60 · roed314/psycodict

roed-math · 2026-07-20T03:39:12Z

Fixes every bug in #58's xfail inventory — all 43 strict markers are gone, because every one of them now passes. The suite is 518 passed / 0 xfailed without Sage and 519 passed under sage -python (the mode-specific tests trade places), plus 21/21 devmirror tests against the live mirror in both modes. +247/−246 over 16 files: the markers bought their fixes roughly line for line.

The headline: `count()` tells the truth now

meta_tables.total was only ever written inside if self.stats.saving: blocks, and saving defaults to False — so a table created and filled through psycodict reported count() == 0 from any process, forever, and rewrite()/update_from_file() refused to run (their label validation compares count_distinct() against the stale count()).

The new PostgresStatsTable._set_total() updates the in-memory total and meta_tables on every insert_many/upsert/delete/copy_from, exactly mirroring what _break_stats already does unconditionally on the same table on the same code paths — so the precedent for writing meta_tables outside the saving guard is the library's own. saving now gates only what its docstring says: recording statistics in the counts table. reload recounts the swapped-in table just before reload_final_swap re-reads meta_tables, and refresh_stats(total=True) persists the total it recomputes rather than updating memory only.

Verified end-to-end: insert 200 → count() 200 on the same connection, 200 from a fresh connection, 150 after deleting 50 — the exact repro that opened the bug.

For LMFDB (saving=True) the observable behavior is unchanged; the cost is one idempotent UPDATE meta_tables per write batch, on paths that already write meta_tables via _break_stats.

One judgment call to review: `max()`/`min()` on no rows

The xfail asserted None; the docstring says "Will raise an error if there are no non-null values." Those can't both be right, and I resolved in favor of the docstring: a deliberate ValueError("no non-null values of %s in %s") replaces the accidental TypeError escaping from fetchone() on an empty cursor. Both tests (the xfailed empty-table pair and the passing constraint-no-match one) now assert that ValueError. If you'd rather have None, it's a three-line change — say so and I'll flip it.

Encoding (verified through real COPY, not just string assertions)

Array quoting: elements are quoted whenever the array parser needs it (empty, NULL, braces, commas, quotes, backslashes, whitespace), with array-level escaping nested inside field-level escaping in the order COPY undoes them. 27/27 exact round trips through actual COPY FROM — embedded tabs, newlines, quotes, backslashes, unicode, [None, 1], 2-D arrays.
None in an array is the array literal NULL, not the field marker \N (which COPY reads as the letter N).
datetime dispatches before date (it is a date), so datetimes round-trip instead of being tagged __date__ and failing to parse.
__time__/__datetime__ parse both with- and without-microsecond forms; the stored format is unchanged, so existing data keeps decoding.
Json.extract recurses to match prep, so nested tagged values decode. Recursing exposed a bonus bug: the __Rational__ branch called Rational(*data), whose second positional argument is the base — 3/7 silently decoded as 3. Now passes the tuple.
Booleans emit t/f (they're int subclasses and fell into the numeric branch); bytea is no longer Python-2 code.

Query layer

$maxgte emits (SELECT max(unnested) FROM unnest(col) ...) >= %s inline. Checked against production first: the LMFDB servers define array_max as exactly this subquery (IMMUTABLE, planner-inlined) and have zero functional indexes on it, so this is behavior-identical there and newly-working everywhere else.
Nested $raw passes the expression rather than the {"$raw": ...} wrapper — the documented {'$lte': {'$raw': 'q^g'}} form works.
_split_ors no longer crashes sorting range-dict branches; _parse_projection copies instead of emptying the caller's dict; random() returns None on an empty table; random_sample(repeatable=N) had two bugs stacked — int() of an already-rebound SQL object, and then REPEATABLE %s without the parentheses the grammar requires.

Schema operations

create_constraint's builders compared against a misspelled "NON NULL" and generated no SQL; _revert_meta had its format() arguments in the wrong order (SELECT <table> FROM <columns>); drop_indexes dropped constraints as indexes, which Postgres refuses; the _tmp/_oldN suffix guard used re.match on $-anchored patterns and so never fired; create_table_like silently widened integer ids to bigint; rename_table left the stale attribute on the database object.

create_extra_table had never been run. Behind the single-column UPDATE (has_extras) = (%s) that PostgreSQL 10+ rejects were, in order: calls to drop_constraints/restore_constraints — methods that have never existed — a NamedTemporaryFile.unlink that should be os.unlink, a restore_pkeys() that tried to give the search table a second primary key, and extra_cols/search_cols bookkeeping that was never updated. All fixed; the test creates a real extras table and verifies the split. Note for #59: that PR deletes the extras machinery — expect conflicts here, and resolve them by taking #59's deletion; these fixes matter only for however long the code exists.

utils / config

Open-ended slices work as IdentifierWrapper's docstring always said; its rejection path raises the intended ValueError (a stray %-conversion in the message was turning it into a formatting TypeError); QueryLogFilter compares basenames so database.py no longer endswith("base.py"); DelayCommit(silence=True) sets the flag on the object base.py reads — verified against the real logging machinery, not a mock; Configuration honors defaults["postgresql_password"] (was postgres_password) and type-coerces options whose dest has no underscore, so a store_true default is a bool rather than the truthy string "False".

Semantic changes worth knowing about (all deliberate, all pinned by tests)

jsonb None → SQL NULL on insert_many/update/upsert, matching copy_dumps and making the documented {col: None} query work. Rows written before this change hold jsonb 'null' and still won't match {col: None} — new writes will.
Nested tagged jsonb decodes on read. Code that learned to unpack raw {"__tag__": ...} dicts by hand will now receive decoded objects.
max()/min() raise ValueError on no non-null values (see above).
Totals are maintained — count({}), info["number"], rewrite(), update_from_file() all become correct for default-settings tables.

Verification


full suite, no Sage	518 passed, 24 skipped, 0 xfailed
full suite, `sage -python` 10.7	519 passed, 23 skipped
devmirror live, both modes	21/21 each
array quoting through real `COPY FROM`	27/27 exact round trips
fresh-process `count()` repro	200 → 200 → 150 ✓
`ruff check`	clean (and one baseline entry retired: the dead `search_order` line fell out of the `create_table_like` fix)

The strict xfail markers were the completeness check: a fixed bug with a leftover marker fails the suite, so 0 xfails means nothing in the inventory was missed and nothing was fixed without its test asserting so.

Json round trips: datetime is now dispatched before date (it is a date subclass, so datetimes were tagged __date__ and failed to parse back); __time__ and __datetime__ accept both with- and without-microsecond forms on read, since prep omits ".%f" at zero microseconds; and extract now recurses to match prep, so nested tagged values decode instead of coming back as raw {"__tag__": ...} dictionaries. Recursing exposed one more: the __Rational__ branch called Rational(*data), whose second positional argument is the BASE, not the denominator -- 3/7 silently decoded as 3. It now passes the tuple, like the __QQList__ branch. copy_dumps: array elements are quoted whenever PostgreSQL's array parser needs it -- empty strings, the literal NULL, and anything containing braces, commas, quotes, backslashes or whitespace -- with array-level escaping applied inside the field-level escaping in the order COPY undoes them; a None element becomes the array literal NULL rather than the field marker \N (which COPY reads as the letter N); booleans take the boolean branch instead of the int branch they fell into as int subclasses; and bytea uses hexlify on the buffer rather than iterating it, which has been Python-2 code all along. Verified beyond the unit tests by driving every nasty value through an actual COPY FROM and back: 27 of 27 round trips exact, including embedded tabs, newlines, quotes, backslashes, unicode, empty and NULL strings, None elements, and two-dimensional arrays. Behaviour notes for existing data: the write-side changes only affect values that could not round-trip before; on the read side, nested tagged jsonb now decodes into objects where code previously received the raw tagged dictionaries. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

IdentifierWrapper accepts the open-ended slices its docstring has always documented (an empty bound now means what it means in Python), and its rejection message no longer contains a stray %-conversion that turned the intended ValueError into a formatting TypeError. QueryLogFilter compares the basename, so psycodict's own database.py no longer matches a filter meant for base.py. DelayCommit(silence=True) sets the flag on the object that base.py actually reads (obj._db), so silencing works when the context is entered on a table. Configuration reads defaults["postgresql_password"] -- the one key that was spelled "postgres_password" -- and type coercion now finds options whose argparse dest has no underscore, so a store_true default comes back as a bool rather than the truthy string "False". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

count() with an empty query is answered from meta_tables.total, but that total was only ever updated inside `if self.stats.saving:` blocks, and saving defaults to False -- so for any table created and filled through psycodict with default settings, count() returned 0, from a fresh process, forever. LMFDB never saw it because its stats subclass sets saving = True; everyone else saw insert_many(rows) followed by count() == 0, and rewrite()/update_from_file() refusing to run because their label-column validation compares count_distinct() against the stale count(). The new _set_total() updates the in-memory total and meta_tables on every insert_many, upsert, delete and copy_from, mirroring what _break_stats already does unconditionally on the same table in the same circumstances; `saving` now gates only what it was meant to gate, the recording of statistics in the counts table. reload recounts the swapped-in table just before reload_final_swap rereads meta_tables, and refresh_stats(total=True) persists the total it recomputes instead of updating it in memory only. Also in statstable: max() and min() on a table with no matching rows raise the error their docstrings have always promised -- a deliberate ValueError, replacing the accidental TypeError that escaped from fetchone() on an empty cursor -- and count(groupby=...) no longer prints its SQL statement to stdout. In insert_many: the caller's dictionaries are no longer mutated (the "we don't want to alter the input" comment sat directly above a shallow list copy whose dicts then had ids added and jsonb values wrapped, so inserting the same list twice raised), and an explicit None in a jsonb column is stored as SQL NULL rather than the jsonb value 'null', which the documented {col: None} query could never match. _parse_values gets the same None rule, so update() and upsert() agree with insert_many and copy_dumps. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Queries: $maxgte emits an inline unnest subquery instead of calling array_max(), a function that exists on the LMFDB's servers (as exactly this subquery, IMMUTABLE, with no functional indexes on it) but on no stock PostgreSQL; a nested $raw passes the expression to filter_sql_injection instead of the {"$raw": ...} wrapper, making the documented {'$lte': {'$raw': 'q^g'}} form work; _split_ors falls back to a deterministic string ordering when branch values are range dictionaries with no natural order; _parse_projection works on a copy rather than emptying the caller's dict; random() returns None on an empty table (max_id is -1 there, and the old == 0 test sent it into randint(0, -1)); and random_sample(repeatable=N) reads the seed before rebinding the variable to an SQL fragment, and parenthesizes it, since the grammar is REPEATABLE (seed). Schema operations: create_constraint's statement builders test for "NOT NULL" as the validator spells it, not the "NON NULL" they were comparing against, which made the constraint type generate no SQL at all; _revert_meta passes format() its arguments in the right order instead of producing SELECT <table> FROM <columns>; drop_indexes drops constraints as constraints, which Postgres requires; the restricted _tmp/_oldN suffix check uses re.search, since its patterns are $-anchored and re.match could only ever find them at the start of the name; create_table_like passes the source table's id type through instead of silently widening integer ids to bigint; and rename_table removes the old attribute from the database object as drop_table does. create_extra_table had never been run: behind the single-column UPDATE form that PostgreSQL 10+ rejects were calls to drop_constraints and restore_constraints, methods that have never existed (now the real drop_indexes, dropping what references the departing columns before DROP COLUMN removes it behind meta's back), a NamedTemporaryFile.unlink that is os.unlink, restore_pkeys trying to give the search table a second primary key (only the new extras table needs one), and extra_cols/search_cols bookkeeping that was never updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

roed-math

I found five issues that should be addressed before merging:

[P1] Staged reload publishes the temporary row count. psycodict/statstable.py:1615

refresh_stats(suffix="_tmp") is used while preparing a reload, including reload(final_swap=False), but _set_total always updates the live meta_tables row. A staged table with a different size therefore makes fresh connections report the temporary count before any swap. Only persist totals for the live suffix; reload_final_swap already recounts after swapping.
[P1] Concurrent writers lose row-count updates. psycodict/statstable.py:322

Write paths pass an absolute value computed from each object's cached self.stats.total. If two connections both load total N and each inserts one row, both write N + 1, leaving meta_tables one short. Use an atomic delta update with RETURNING for insert/delete/copy paths, while retaining a separate absolute setter for recounts.
[P1] Moving constrained columns targets the wrong table. psycodict/table.py:2470

search_cols is changed before drop_indexes(columns). Consequently _get_constraint_data classifies a constraint on a moved column as belonging to the newly created extras table: UNIQUE and CHECK fail because that constraint does not exist there, while NOT NULL is silently removed from metadata and not preserved. Drop or migrate constraints before changing the bookkeeping, then recreate them on the extras table.
[P2] Inactive DelayCommit permanently silences logging. psycodict/utils.py:216

silence is applied even when active=False, but exit restores it only when active=True. reload_all(sequential_swap=True) uses exactly DelayCommit(..., silence=True, active=False), so the database remains silenced afterward. Gate this assignment on active, or restore the flag regardless.
[P2] insert_many no longer returns assigned IDs. psycodict/table.py:1463

Copying every input dictionary prevents the later SD["id"] assignment from reaching the caller, contradicting this method's documented API. Preserve the ID mutation on the original dictionary while using a separate copy only for Json wrapping.

Five findings from review, each verified before fixing. refresh_stats(suffix="_tmp") runs while a reload is staged, and _set_total was publishing the staged table's count as the live total. Suffixed recounts no longer touch self.total or meta_tables (the same gate is added to _record_count's own meta update, which had the same flaw); reload_final_swap recounts after the swap, which is when the staged count becomes the truth. The write paths passed absolute totals computed from each connection's cached value, so two concurrent writers could both write N + 1 and lose a row. They now use _update_total, which applies the delta in the database itself (GREATEST(total + %s, 0) ... RETURNING) and refreshes the in-memory value from the returned row; _set_total remains for the recount cases. create_extra_table changed search_cols before dropping the constraints that touch the moved columns, and constraint classification keys off search_cols -- so the drops targeted the extras table. The bookkeeping now changes only after the physical move, and the touched constraints are captured first and recreated afterwards, landing on the extras table where the columns now live. DelayCommit applied silence= even when active=False but only restored it when active, so reload_all's DelayCommit(silence=True, active=False) silenced the database permanently. The flag is now gated on active. insert_many's documented API stamps the assigned ids onto the caller's dictionaries; the copy that stopped Json wrappers leaking out was also stopping the ids. The ids are stamped on the originals again, and only the wrapping stays internal. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

roed-math · 2026-07-20T15:20:48Z

Superseded by a one-PR-per-issue split, as requested — 27 PRs, each independently reviewable and mergeable, each green with the rest of the bug inventory still xfailed (the strict markers guarantee no cross-issue leakage). The partition was verified exact: every changed line of this PR's diff appears in exactly one of the split PRs, none missing, none duplicated.

The five review comments above are addressed, with regression tests, in the split:

staged reload publishing the temporary count → count() returns a stale total for default-settings tables #61 (suffixed recounts no longer touch the live total; same gate added to _record_count's own meta update, which had the same flaw)
concurrent writers losing count updates → count() returns a stale total for default-settings tables #61 (_update_total applies the delta in the database with RETURNING; two-writer regression test)
constrained columns moving to the wrong table → create_extra_table has never worked #81 (drop before the bookkeeping changes, recreate on the extras table afterwards; migration test)
inactive DelayCommit silencing permanently → DelayCommit silencing is broken in two ways #82 (silence= gated on active)
insert_many no longer returning ids → insert_many corrupts the caller's dictionaries #64 (ids stamped on the caller's dictionaries per the docstring; only the Json wrapping stays internal)

The index, in suggested review order (heaviest first):

PR	issue
#61	`count()` stale totals (+ concurrency, staged reloads, `rewrite`/`update_from_file`)
#67	`copy_dumps` array corruption through COPY (verified 27/27 through a real `COPY FROM`)
#66	`Json.extract` doesn't invert `prep` (datetime/time/nested/`Rational(*data)` base bug)
#65	jsonb `None` stored as `'null'` instead of SQL NULL (semantic change)
#64	`insert_many` corrupts the caller's dictionaries
#81	`create_extra_table` has never worked (five bugs; includes #77; conflicts with #59 — if #59 lands first, close unmerged)
#62	`max()`/`min()` crash → the docstring's promised error (judgment call flagged inside)
#82	`DelayCommit` silencing broken two ways
#77	`drop_indexes` drops constraints as indexes
#69	`$maxgte` emits nonexistent `array_max()`
#74	`random_sample(repeatable=)` always raises
#84	`IdentifierWrapper` rejects documented slices
#63 #70 #71 #72 #73 #75 #76 #78 #79 #80 #83 #85 #86	small single-function fixes
#87	test-only: Sage-clean DB-backed tests

roed314 and others added 4 commits July 19, 2026 23:37

roed-math commented Jul 20, 2026

View reviewed changes

This was referenced Jul 20, 2026

Configuration ignores the password default #85

Merged

type coercion misses options with a bare dest #86

Merged

make the database-backed tests Sage-clean #87

Merged

roed-math closed this Jul 20, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the bugs found by the CI suite#60

Fix the bugs found by the CI suite#60
roed-math wants to merge 5 commits into
roed314:masterfrom
roed-math:fix-ci-bugs

roed-math commented Jul 20, 2026

Uh oh!

roed-math left a comment

Uh oh!

roed-math commented Jul 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

roed-math commented Jul 20, 2026

The headline: count() tells the truth now

One judgment call to review: max()/min() on no rows

Encoding (verified through real COPY, not just string assertions)

Query layer

Schema operations

utils / config

Semantic changes worth knowing about (all deliberate, all pinned by tests)

Verification

Uh oh!

roed-math left a comment

Choose a reason for hiding this comment

Uh oh!

roed-math commented Jul 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

The headline: `count()` tells the truth now

One judgment call to review: `max()`/`min()` on no rows