Operate in exactly one schema, and filter every catalog query to it - #142
Operate in exactly one schema, and filter every catalog query to it#142roed-math wants to merge 4 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.
394a1ac to
72dde8c
Compare
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.
72dde8c to
a0ab659
Compare
…settings to it Review of roed314#142 found the "exactly one schema" contract still open in three places. search_path was set from the schema's raw text. But a search_path is not an identifier: it is a comma-separated list, in which unquoted names are case-folded and $user is a substitution. A schema named "a, b" therefore selected schema "a" while every catalog query kept binding the literal name -- so with create=True the metadata was bootstrapped into a schema the object did not think it was using. Verified on PG18: as text that name selects psycodict_path_a; composed as one Identifier it selects the schema asked for, and a name with spaces or mixed case is refused by the server outright rather than silently reinterpreted. pg_temp is now named explicitly *after* the schema, since left unnamed it is searched *first* and a temporary meta_tables shadowed the real one (also verified). pg_catalog stays unnamed and so stays ahead of both. PostgreSQL ignores a search_path entry naming a schema that is missing or that the role has no USAGE on, and with pg_temp present the session then resolves unqualified names -- including CREATE TABLE -- in the temporary schema, so current_schema() is checked afterwards and a mismatch raises. One name cannot be expressed at all: PostgreSQL unquotes each element before looking for the $user token, so "$user" is replaced by the role name and never reaches a schema of that name; validate_schema_name refuses it, with the reason. Any other spelling, $USER included, is an ordinary identifier. _get_locks() collected locks from every non-system schema while _table_locked() matched on relation name alone, so a lock on other_schema.same_name was reported as a lock on this schema's same_name -- and _check_locks() consults that before writes, reloads, create_table_like and column changes, refusing an operation nothing was blocking. Another session's temporary table of the same name did it too. Now filtered to the schema in SQL, which subsumes the old pg_toast and pg_catalog exclusions. _clone_storage_settings() was the one relation-specific catalog query still resolving a bare name through regclass. It now joins pg_class to pg_namespace and binds the schema, keeping the pre-14 branch that omits attcompression, so the helper is correct on its own rather than by way of the session's state. Tests: five in test_schema_contract.py for the path (the comma-containing schema and its two decoys, a schema needing quotes, the $user refusal, a missing schema, the temporary-relation shadow), one in test_locks.py, one in test_table_like_size.py. All seven fail on the previous head. Full suite 1332 passed, 36 skipped; ruff and the -W docs build clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Review addressed in 8cd9bd4. All three findings, plus the documentation items. 1 (P1)
|
written to search_path as |
current_schema() |
|---|---|
| text (previous head) | psycodict_path_a |
one Identifier |
psycodict_path_a, psycodict_path_b |
So create=True really did bootstrap the metadata into the first component while the object reported the literal name. A name with spaces or mixed case is a cleaner failure as text (InvalidParameterValue: List syntax is invalid), but the comma case is silent.
$user cannot be expressed at all, quoting included. PostgreSQL calls SplitIdentifierString before comparing against the $user token, so "$user" is unquoted first and substituted with the role name; a schema genuinely called $user is unreachable through search_path by any spelling. Verified: with schemas $user, $USER and postgres all present, SET search_path TO "$user" selects postgres, while "$USER" and "$User" select themselves. validate_schema_name now refuses the exact spelling $user with that reason, rather than letting it fail confusingly at connect time. Its docstring says so, and still says the name is quoted wherever psycodict emits it, which is now true of search_path too.
pg_temp is named last, and the temp-shadow test confirms both directions: 'meta_tables'::regclass resolves to the persistent OID with pg_temp named, and to the temporary table without it.
current_schema() is checked right after, raising ValueError on a mismatch. This matters more than "clear error instead of silence": with pg_temp in the path and the selected schema missing or USAGE-less, PostgreSQL falls through to pg_temp_NN, so an unqualified CREATE TABLE would have produced a temporary table.
2 (P2) Lock discovery is scoped to the schema
_get_locks() filters with t.schemaname = %s bound to self._db.schema; the pg_toast/pg_catalog exclusions go away, subsumed by the equality. show_locks()'s docstring and the DataManagement.md bullet now say the result is this database's schema.
3 (P2) _clone_storage_settings carries its own schema predicate
Replaced the %s::regclass lookup with the pg_class/pg_namespace joins and [self.schema, tname], keeping the pre-14 branch that omits attcompression.
Documentation
schema added to the class docstring's INPUT (existence, USAGE, CREATE) and to the attribute list; DataManagement.md states the effective pg_catalog / schema / pg_temp order and the existence and privilege requirements; the CHANGELOG entry for this change is extended.
Tests
Seven new, all failing on a0ab659:
test_schema_contract.py: comma-containing schema with its two decoys (asserting the metadata landed in the selected schema and the decoys stayed empty, acrossreset_connection()), a schema needing quotes, the$userrefusal, a missing schema, and the temporary-relation shadow.test_locks.py::test_locks_in_another_schema_are_not_this_schema_s:ACCESS EXCLUSIVEonother.<name>from a second connection; the lock is confirmed present in the catalog,_table_lockedreturns[], and_check_locks("insert_many")goes ahead. The existing positive cases are untouched.test_table_like_size.py::test_column_storage_settings_are_read_from_the_selected_schema: puts a same-named decoy ahead of the schema in the session'ssearch_path, so it fails on the old code even with finding 1 fixed, which is the point of the helper being independently correct.
Validation
1332 passed, 36 skipped, 1 xfailed (was 1325), twice in a row; ruff check . clean; python -m sphinx -W --keep-going clean. Local runs are against PostgreSQL 18.3 / Python 3.13 / psycopg 3.3.4; the matrix will cover 13-18 and 3.9-3.14.
One note on the local suite: tests/test_notifications.py is timing-sensitive and fails intermittently on a loaded machine, at the same rate on a0ab659 as here, so it is unrelated to this change.
|
GPT signed off. |
2 participants
Third of the rc3 stabilization PRs: Track C.
Stacked on #140 and #141; the diff contains their commits too.
The problem
Unqualified DDL and DML followed whatever
search_pathhappened to be, andcatalog inspection was a mixture of hard-coded
'public'and no filter at all.I classified every catalog query in the package: 30-odd sites, of which 5
hard-coded
public, 2 were correctly scoped, and the rest had no schemapredicate whatsoever.
With
same_namein two schemas that gives answers assembled from both:_column_typesunions their columns or raises "Type mismatch";_index_existsand
_constraint_existsreport an object that lives in the other schema;_all_tablenameslists the name twice;table_sizesreportspublicno matterwhich schema the session is actually using.
The contract
PostgresDatabase(..., schema="public"). Default unchanged, so nothing movesfor existing deployments. Validated as an identifier once in the constructor
(new
validate_schema_name), and pinned insearch_pathfrom_configure_session— which runs for the first connection and everyreplacement, so a reconnect cannot come back pointing elsewhere.
It is deliberately not part of
_connect_kwargs:psycopg.connecthas nosuch parameter, and putting it there would pass it to the driver.
The audit
Every catalog query now binds the selected schema as a value:
_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 andknowls capability probes,
_grantees, the legacy-extras check,table_sizes,the tablespace map,
_check_tmp_leftovers' two probes, the metadata bootstrap'sexisting-table set,
_approx_most_common, anddbdiff's column reader._schema_relationsand_approx_most_commonpreviously asked the server withcurrent_schema(). They bind the same value as everything else now, so there isone notion of which schema this is rather than two that happen to agree.
One query keeps its own schema on purpose: the
userdb.usersgrant probe isabout a different schema by design, not about this database's.
Tests
New
tests/test_schema_contract.py, 15 cases, 12 of which fail on rc2:public.same_nameandpsycodict_other.same_namewith different columns, theneach lookup checked for confinement; the search path after connect and after
reset_connection();schemaabsent from the driver options; an invalid schemaname refused. Two show the confinement from the other side — a database pointed
at the other schema is refused for having no metadata tables there rather than
quietly using
public's, and withcreate=Trueit sees its ownsame_namecolumns and none of
public's search tables.Full suite: 1326 passed, 36 skipped. Ruff and the
-Wdocs build clean.🤖 Generated with Claude Code