Stop applying a statement timeout based on the role name - #146
Open
roed-math wants to merge 8 commits into
Open
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.
The versioning policy said "documented non-underscore names are public", while Sphinx ran with undoc-members, so any helper that happened to have a docstring became part of the permanent 1.x surface. Each module now declares __all__ -- the curated set of names psycodict actually promises -- and the API reference documents exactly those (undoc-members is off). A non-__all__ name, docstring or not, is implementation: still importable, so nothing downstream breaks, but not promised. tests/test_public_api.py freezes every module's __all__, checks each exported name resolves, checks `from psycodict import *` binds exactly the root set, and checks the specific names LMFDB and seminars import still resolve -- including private ones (seminars' _counts_cols, _meta_*_cols) and a couple kept importable but unpromised (range_formatter, KeyedDefaultDict), since __all__ governs `import *` and the docs, not explicit imports. Versioning.md is rewritten around this: public = the __all__ names and their documented behavior; db[name] is the canonical table lookup while db.<name> is convenience a real database attribute wins over, so adding a method in a minor release never makes a table unreachable through db[name].
A connection as ``webserver`` was handed a 25 second statement_timeout by default. That is an LMFDB deployment fact, not something a generic library should infer from a role name, so the special case is removed: _resolve_session_settings applies nothing unless session_settings is given. Explicit settings are unchanged and still survive reconnects and are still held to the allow-list. LMFDB restores the timeout explicitly in its subclass; that change lands before LMFDB bumps to a psycodict carrying this one, or the web workers lose their timeout.
Documentation build overview
31 files changed ·
|
The changelog pointed at lmfdb#7128 ("Check the bound in PrimeBound"),
which has nothing to do with this change. The migration it tells
maintainers to land first is lmfdb#7131, "Set the web workers' statement
timeout explicitly".
The comment above _allowed_session_settings said the settings apply to
"a every connection", which is both ungrammatical and wrong: a
listener's notification connection deliberately does not inherit them.
Both it and the first line of _resolve_session_settings's docstring now
say what the rest of that docstring already said, the main connection
and its replacements.
No behavior change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
|
Both review items are addressed in 37239ce. No implementation change: 1. Companion-PR link. 2. Session-setting scope. The comment above Validation (disposable PG18 cluster):
🤖 Generated with Claude Code |
Owner
|
GPT signed off. |
roed-math
pushed a commit
to roed-math/lmfdb
that referenced
this pull request
Aug 5, 2026
requirements.txt still allows psycodict 1.0.0rc1, whose constructor has no
session_settings parameter: it forwards unknown keywords into psycopg.connect,
which rejects this one ("invalid connection option session_settings"), so the
web workers would not get a connection at all. rc1 also still applies its own
25 second timeout to the webserver role, so there is nothing to restore there.
Detect the parameter on PostgresDatabase.__init__ and only ask for the setting
when it exists; the previous commit's claim that the keyword works on every
psycodict version was wrong, and CI missed it because its unconstrained upgrade
installs rc2.
Also take the connecting role from a keyword by presence rather than by
truthiness, and read an explicit session_settings=None as a request for the
LMFDB default: after roed314/psycodict#146 None means no settings of the
library's own, so setdefault would quietly drop the timeout for a caller that
forwards None. An explicit mapping is left alone, including {} for a caller
that deliberately wants no timeout.
The tests capture what LMFDBDatabase hands to psycodict with the compatibility
flag patched, so they cover both psycodict APIs without opening a connection.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
2 participants
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.
Seventh of the rc3 stabilization PRs: Track G2.
Stacked on #140–#145. Pairs with LMFDB/lmfdb#7131.
A connection as
webserverwas silently given a 25 secondstatement_timeout.That is an LMFDB deployment fact, not something a generic library should infer
from a role name, so the special case is removed:
_resolve_session_settingsapplies nothing unless
session_settings=is given. Explicit settings areunchanged, still survive reconnects, and are still held to the allow-list.
Coordination: this makes LMFDB's web workers lose their timeout the moment
LMFDB runs on a psycodict that includes it, so
LMFDB/lmfdb#7131 ("Set the web
workers' statement timeout explicitly"), which restores the timeout explicitly
in
LMFDBDatabase, must land before LMFDB bumps its psycodict requirementpast this change. It is inert until then (LMFDB pins an earlier rc), the same
pattern as the grant policy.
Tests: the old test asserted the
webserverrole got the implicit timeout;replaced with one asserting no role gets a session setting implicitly, and one
asserting an explicit setting is kept.
Full suite: 1399 passed, 35 skipped, 1 xfailed.
🤖 Generated with Claude Code