Packaging, generic defaults, and an export-format marker - #144
Packaging, generic defaults, and an export-format marker#144roed-math wants to merge 10 commits into
Conversation
…random edge cases Four independent defects found reviewing rc2, each with regression tests that fail on rc2 and pass here. max_id/min_id formatted their table argument into the statement as text. A name needing quotes was a syntax error and a name carrying its own statement ran it; both compose an Identifier now. While there, the empty sentinel is documented: max_id returns -1, and 0 is a real id, which is what random() got wrong below. _approx_most_common read reltuples from a hard-coded public.nf_fields but read frequencies from the owning table, so every table other than nf_fields got its own frequencies scaled by an unrelated row count. The row count now comes from the table the statistics are about, looked up by name in the current schema, and the column type goes through column_type_sql rather than being concatenated. update_from_file's logging default was a shared dictionary literal that the method wrote logid and aborted into. Consecutive default calls saw the previous call's values and a caller's dictionary came back modified. An AST scan of the package confirms this was the only mutable default anywhere that is actually mutated, so nothing else needed changing. random() raised IndexError from random.choice([]) when pick_first found no values, reported a table whose only row has id 0 as empty, and discarded rows whose projection was falsy -- a table of zeros exhausted maxtries and raised "Random selection failed!". random_sample returned None for an unrecognized mode, which reads like an empty result, and reseeded the global random module when asked for a repeatable sample.
Write paths called _break_stats, but read paths queried the cache tables regardless, so a count cached before a restat=False write kept being served afterwards. Reproduced against PostgreSQL 18: a query counted at 67, every matching row then changed so that none satisfy it, and quick_count still answered 67. Every lookup that would serve a cached answer now goes through one predicate, _may_use_cache, and reports a miss while stats_valid is false -- quick_count, quick_count_distinct, _quick_statistic, _has_stats, _has_numstats and null_counts, which between them are what make count, max, min, sum, column_counts and numstats recompute rather than return a stored value. Three things are deliberately outside the rule, and the predicate's docstring says why: the empty-query total, which is maintained on every write and stays exact; a suffixed table, whose caches are its own and which stats_valid says nothing about; and the _status/status/extra_counts inventory, which reports what the cache contains rather than answering a question about the data -- refresh_stats uses it to discover what to recompute, so gating it would make an invalid table forget what statistics it is supposed to have. The flag had no way back to true. _restore_stats is the counterpart of _break_stats, called at the end of refresh_stats inside the same transaction that rebuilt the caches, so a refresh that fails part-way leaves the table marked invalid rather than claiming a cache it does not have. A suffixed refresh does not touch the live flag. Separately, bulk paths now run PostgreSQL's own ANALYZE, which is a different thing from psycodict's statistics: a bulk-loaded relation has none until autovacuum reaches it and the planner costs it as though it were tiny. The _tmp copies are analyzed before the swap and outside its transaction, since the catalog entry follows the relation through a rename -- one call in _swap_in_tmp covers reload, rewrite, non-inplace update_from_file and staged commits, which all funnel through it. copy_from analyzes the live table. The existing statistics fixtures insert rows, which invalidates, and then assume a usable cache; they now say so with _restore_stats, which is true of them (nothing is cached yet and the total is maintained by the insert) and is the state a freshly loaded table is in.
Unqualified DDL and DML went wherever search_path happened to point, while catalog inspection was a mixture of hard-coded 'public' and no filter at all. With a relation of the same name in two schemas the answers came from both: _column_types unioned their columns (or raised "Type mismatch"), an index or constraint in the other schema counted as present, _all_tablenames listed the name twice, and table_sizes reported only public whatever the session was using. PostgresDatabase now takes schema="public", validates it as an identifier once in the constructor, and pins search_path to it in _configure_session -- which runs for the first connection and for every replacement, so a reconnect cannot come back pointing somewhere else. It is deliberately not part of _connect_kwargs: psycopg.connect has no such parameter, and passing it there would reach the driver. Every catalog query is then filtered to that schema, binding it as a value rather than interpolating it: _table_exists, _all_tablenames, _index_exists, _list_indexes, _relation_exists, _constraint_exists, _list_constraints, _column_types, _relation_columns, refresh_tables' column discovery, the read-only and knowls capability probes, _grantees, the legacy-extras check, table_sizes, tablespaces, _check_tmp_leftovers' two probes, the metadata bootstrap's existing-table set, _approx_most_common and dbdiff's column reader. _schema_relations and _approx_most_common previously asked the server with current_schema(); they now bind the same value as everything else, so there is one notion of which schema this is. The userdb.users grant probe keeps its own schema: that one is deliberately about a different schema, not about this database's.
…ize_changes resort() was a disabled no-op: the old implementation renumbered every id with an in-place UPDATE, which stalls replication and leaves the rows in their old physical order, so the point of id-ordering -- sequential disk 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, which is a physical rebuild (INSERT ... SELECT ... ORDER BY into a fresh table that replaces the original) rather than an in-place UPDATE. Everything else -- the 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. reload and non-inplace update_from_file call _generate_sorted_ids directly rather than public resort(), so there is no recursion, and the renumber now runs before the keys are rebuilt (it replaces the table, so the table must have no pkey/indexes at that point). Because a resort is a full-table rebuild, it is no longer a side effect of a small write. resort=True on insert_many, update, copy_from and in-place update_from_file raises, pointing at resort(); resort=False is unaffected. rewrite defaults resort to the replacement path only, and a staged table's in-place writes drop the request. An in-place update that changes a sort key now records out_of_order rather than pretending it could reorder. finalize_changes() was a documented public no-op; it is removed. The write methods already leave total, the order flag and stats_valid correct on return, which test_write_invariants now checks for every path. scripts/audit_id_order.py is a read-only check that streams each id_ordered table in sort order (server-side cursor, constant client memory) and reports whether the ids actually increase, since the flag can drift and must not be trusted blindly during the production audit.
Track E: psycopg becomes a plain dependency, so `pip install psycodict` gets a working package (pure-Python driver on the system libpq). pgbinary adds the bundled binary build and pgc a locally compiled one; the pgsource extra is removed, since plain install replaces it, and a <4 ceiling keeps an unreviewed driver major out of a 1.x environment. CI's "import must fail without psycopg" step is inverted to "plain install must import", and both smoke installs now assert importlib.metadata.version == __version__ and run pip check. Track F: search-data export files gain an optional `# psycodict-export-format: N` marker. One shared reader (_read_header_lines) accepts a marked file, accepts an unmarked file as format 0 so every older export still loads, and refuses a version it does not understand before loading any data; the writers (copy_to via _write_header_lines, and rewrite's inline header) emit the current marker. This is the data-file format, kept distinct from the meta_* metadata format; Versioning.md states the realistic promise and decouples both format numbers from the package major. Track G: the default database name is the generic `postgres` rather than `lmfdb` (LMFDB names its own), and the unused `secretsfile` argument to PostgresDatabase is removed -- no caller in LMFDB or seminars passes it. Track H: MANIFEST.in makes the sdist a complete, testable checkout (tests with conftest.py, scripts, guides, metadata), and CI unpacks it and runs its database-free tests so a dropped file fails the build. The release workflow's third-party actions are pinned to commit SHAs.
Review follow-ups to the export-format marker and the packaging changes. Marker recognition. `_read_header_lines` decided on `startswith`, but a column may be called anything printable, so a format-0 names row can begin with the marker prefix -- `# psycodict-export-format: 1|another_column` is a valid two-column header, and a one-column header may be a valid marker character for character. Both were refused, breaking the promise that every older export still loads. The reader now looks ahead: a format-0 header is names/types/blank and a marked one marker/names/types/blank, so a blank third physical line means the first line was column names. Only then is the line parsed as a marker, against the whole grammar (`export_format_version`), which also rejects the negative versions `int()` used to accept as "not newer than the current format". The blank-line error no longer names a line number that is right for only one of the two layouts. Reindex threshold. `copy_from`'s automatic choice counted physical lines against 1003, so a marked file of exactly 1000 rows reindexed although the documented policy is *more than* 1000, and moving the constant would only shift the bug to format-0 files. New `_count_data_rows` consumes the header through the shared reader (honoring `sep`) and counts what is left. Tests. `test_create_table_from_header_reads_the_marker` asserted only that an export starts with the marker and never entered the header-reading path it was named for; it is replaced by one that reloads with `adjust_schema=True` from a file naming a strict subset of the columns, so the reloaded table proves the header drove its creation. Added: the two marker-like legacy headers, a negative version, the grammar itself, custom separators, and the 1000/1001 reindex boundary for both formats (asked of `drop_indexes`, since both branches leave the same indexes behind). Docs. MetadataFormats.md, the `META_FORMAT` comment and DataManagement.md still said the metadata format number tracks the package major version; Versioning.md now says otherwise, and it is the source of truth -- a compatible additive revision may ship in a minor release, one that raises `min_compat` past a supported client may not. CONTRIBUTING.md and the psycopg migration note still recommended the removed `pgsource` extra; README now states the psycopg range as `>=3.2.4,<4`. `postgres` is described as the conventional maintenance database rather than as libpq's default, which is actually the connection user name. Packaging. `config.ini.example` was tracked, changed by this branch and absent from the sdist; MANIFEST.in names it, and the unpacked-sdist CI step now asserts the files no test would miss. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Pushed 1. Legacy exports whose first column resembles the marker (high)Correct:
One deliberate refinement on the plan: the lookahead runs for any first line beginning with the prefix, not only syntactically valid markers. That way a one-column legacy file named
Tests (all confirmed to fail against the pre-fix reader): both marker-like legacy headers end to end, a negative version, 2. Reindex threshold counts data rows (medium)Correct, and confirmed at the boundary:
New 3. A test that actually invokes
|
…keeping Four correctness problems in the reviewed head, none of them changing the architecture the PR set out: resort() stays an explicit dump/rebuild/swap, row-level writes still reject resort=True, replacement writes may establish order while building their replacement, finalize_changes stays removed, and the audit stays read-only and streaming. 1. scripts/audit_id_order.py could not start: it imported a configured `db` object the package does not export. main() now builds a PostgresDatabase from the usual configuration, and takes one as an argument so the tests audit their own fixture connection instead of opening a second one. Its named-cursor read also left a transaction open, so a successful multi-table run held one MVCC snapshot until exit, and a table that errored left the connection in an aborted transaction -- every table after it then reported InFailedSqlTransaction rather than being audited. Each table now audits in a read transaction of its own, closed on every exit path (including the early mismatch return) without masking the audit's own error. id_ordered with no configured sort is an error rather than an OK obtained by ordering on id itself. 2. update_from_file overloaded `resort` for two different facts: whether the caller wants a rebuild, and whether the input touches a sort key. Since the sort-column check ran only when resort was None, an explicit resort=False suppressed the _break_order bookkeeping as well as the rebuild -- so an in-place or unrebuilt replacement update of a sort column, a rewrite(inplace=True) (whose default forces resort=False), and every staged write (the staged wrapper forces it too) left out_of_order = false standing. On a staged sort-key rewrite that false travelled through _staged_commit into the live meta_tables row, which search code is allowed to act on by replacing ORDER BY <sort> with ORDER BY id. sort_changed is now computed from the file's columns, independently of the option: resort=False means do not rebuild, not pretend the sort was untouched. rewrite(inplace=True, resort=True) raises before func runs over the table rather than after. 3. _generate_sorted_ids built its INSERT ... SELECT 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 copy and loaded as NULL, a removed one failed with an undefined column, and a metafile changing the sort got ids assigned by the sort it was replacing and then metadata claiming the new one. The helper now reads the target relation's own columns, takes the sort to number by as an argument (reload passes the one the metafile will install), and validates the relation has an id, has columns besides id, and has every sort column -- before the first destructive statement, so a file and a metafile that disagree leave the relation as it was. 4. update_from_file, rewrite and reload with restat=False swapped changed data in beside cloned, partial or untouched counts and stats while leaving stats_valid true, so a count cached before the write was still served after it -- defeating the enforcement added in the preceding PR. The decision now lives in one place, _set_stats_validity, called inside the same transaction as the update or the swap: valid only when the caches live were built from the data live with them (recomputed here, or supplied whole by an export carrying both countsfile and statsfile). A metafile no longer installs a stats_valid describing the database it came from. Regression tests for all four, in test_id_order_audit, test_write_invariants, test_staged_writes, test_resort and test_stats_validity; each new test was checked to fail against the reviewed head. Full suite green (1397 passed), ruff clean, docs build clean under -W. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
GPT signed off. |
…istic resort() renumbers the rows without changing them, so the cached counts and statistics would still be accurate afterwards; but it goes through the replacement path, which can only tell that the caches match when it rebuilt them, and it rebuilds them only with `saving` on. On a table that does not save statistics a resort therefore ends with stats_valid false and needs a refresh_stats() to serve cached counts again. That is the safe direction (a miss recomputes; a false true serves a wrong answer), so leave the behavior alone and make it a documented, tested quantity rather than a surprise: a note in resort()'s docstring and in the CHANGELOG, and a pair of tests pinning both halves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rectly The test reloaded with resort=False and a comment saying why: renumbering selected the pre-reload table's columns, which the narrowed relation adjust_schema had built no longer had. roed314#143 now reads the target relation's columns from the catalog, so the default (resort, since the file carries no ids) works, and the test pins the interaction from this side by checking that the ids were renumbered in sort order. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Follow-up on the For the record, that was not pre-existing. On
Merged Full suite on the merged branch: 1426 passed, 36 skipped, 1 xfailed. Ruff clean. |
2 participants
Fifth of the rc3 stabilization PRs: E, F, G1, G3, H.
Stacked on #140–#143; the diff contains their commits too.
E — psycopg is a plain dependency
dependencies = ["psycopg>=3.2.4,<4"], sopip install psycodictgets a workingpackage (pure-Python driver on the system
libpq).pgbinaryadds the bundledbinary build,
pgca locally compiled one;pgsourceis removed (plaininstall replaces it). The
<4ceiling keeps an unreviewed driver major out of a1.x environment.
CI's old "importing without psycopg must fail" step is now inverted — a plain
install must import — and both smoke installs assert
importlib.metadata.version("psycodict") == psycodict.__version__and runpip check. Verified locally: plain wheel install pulls psycopg 3.3.4 andimports;
pip checkclean.CONTRIBUTING.md, the psycopg migration note inCHANGELOG.mdandREADME.mddescribe the final installation contract, with no remaining recommendation of
the removed extra and the supported range stated as psycopg 3.x, 3.2.4 or later.
F — export-format marker
Search-data files gain an optional
# psycodict-export-format: Nfirst line.One shared reader (
_read_header_lines, used by both_check_header_linesand_create_table_from_header) accepts a marked file, accepts an unmarked file asformat 0 so every older export still loads, and refuses a version it does
not understand before loading any data. The writers —
copy_tovia_write_header_lines, andrewrite's inline header — emit the current marker.Deciding on the prefix alone would not have kept that promise: a column may be
called anything printable, so a format-0 names row may itself begin with the
marker prefix (
# psycodict-export-format: 1|another_columnis a validtwo-column header) or be a valid marker character for character. The reader
therefore looks ahead — a format-0 header is
names / types / blank, a markedone
marker / names / types / blank, so a blank third physical line means thefirst line was column names — and only then parses the line against the whole
marker grammar, which also rejects the negative versions
int()would accept.copy_from's automaticreindexcounts data rows, through the same reader,rather than physical lines: a marked header is one line longer than a format-0
one, so a line count put the documented "more than 1000 rows" boundary in a
different place for each format.
This is the data-file format, kept distinct from the
meta_*metadata format.Versioning.md (F2/F3) now states the realistic promise — a newer 1.x reader
reads older files, not necessarily the reverse — and decouples both format
numbers from the package major version;
MetadataFormats.md, theMETA_FORMATcomment and
DataManagement.mdstate that same policy, keyed to what a revisiondoes to
min_compatrather than to the major version.tests/test_export_format.py: round-trip a marked file, load a format-0 file(built by stripping the marker off a real export, so it is byte-faithful),
load both marker-like legacy headers, reject a future version, a malformed
marker and a negative version before any load, exercise custom separators, pin
the 1000/1001 reindex boundary for both formats against
drop_indexesitself,and reload with
adjust_schema=Truefrom a file naming a strict subset of thecolumns, so the reloaded table proves the header drove its creation.
G — generic defaults
postgres, notlmfdb: the maintenance databaseconventionally created with a PostgreSQL cluster, hence the one most likely to
exist. (It is not libpq's own default, which is the connection user name.)
LMFDB passes
dbname="lmfdb"explicitly, so nothing there changes.secretsfileargument toPostgresDatabase.__init__is removed —it was accepted and ignored, and no caller in LMFDB or seminars passes it
(I checked both).
(G2, the
webservertimeout, is not here: it needs the matching LMFDB change inthe same window, so it comes with the API PR and its LMFDB companion.)
H — packaging and release hardening
MANIFEST.inmakes the sdist a complete, testable checkout: the whole testsuite including
conftest.py(which the rc2 sdist omitted, leaving theshipped tests unrunnable), the scripts, the guides, the metadata files and
config.ini.example.test would miss are present, and runs its database-free tests, so a dropped
file fails the build. Verified locally: 340 passed from the unpacked source.
checkout,setup-python,upload-artifactanddownload-artifactare pinned to commit SHAs (with version comments); thePyPI publish action was already pinned.
Full suite: 1388 passed, 36 skipped, 1 xfailed. Ruff and the
-Wdocs buildclean.
python -m build+twine check --strictpass; both artifacts installand import.
🤖 Generated with Claude Code