diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f0d6ae3..be22490 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -80,7 +80,7 @@ jobs: # supported pair, the newest pair, and the combination LMFDB actually # deploys. Add a row here when a new PostgreSQL major is released. # The extra oldest-pair row pins psycopg to the declared minimum - # (pyproject's pgbinary/pgsource floor), so the floor stays honest. + # (pyproject's pgbinary floor), so the floor stays honest. matrix: include: - {python: "3.9", postgres: "13"} @@ -162,7 +162,7 @@ jobs: # installed -- which made this check pass against a wheel with its # modules deleted. Asserting where the import came from is what makes # the test about the wheel rather than about the source tree. - - name: Wheel installs and imports + - name: Wheel installs and imports with the binary extra run: | python -m venv "$RUNNER_TEMP/smoke" "$RUNNER_TEMP/smoke/bin/pip" install "$(echo "$PWD"/dist/*.whl)[pgbinary]" @@ -178,32 +178,49 @@ jobs: "imported the checkout rather than the installed wheel: %s" % path ) assert PostgresDatabase is not None and Json is not None + assert importlib.metadata.version("psycodict") == psycodict.__version__ print("psycodict", importlib.metadata.version("psycodict"), "imports from", path) PY + "$RUNNER_TEMP/smoke/bin/pip" check - # psycopg is deliberately an optional dependency, so that users choose - # between the pure-Python and binary builds. Importing without it must - # fail with the guidance in psycodict/__init__.py rather than a bare - # ImportError -- this asserts that contract holds. - - name: Import without psycopg gives a helpful message + # psycopg is a plain dependency now, so a bare ``pip install psycodict`` + # already pulls the pure-Python driver: a plain install must import, not + # fail. (A binary build is still available as the pgbinary extra above.) + - name: Wheel installs and imports without any extra run: | - python -m venv "$RUNNER_TEMP/bare" - "$RUNNER_TEMP/bare/bin/pip" install "$(echo "$PWD"/dist/*.whl)" + python -m venv "$RUNNER_TEMP/plain" + "$RUNNER_TEMP/plain/bin/pip" install "$(echo "$PWD"/dist/*.whl)" # Outside the checkout, for the same reason as the step above. cd "$RUNNER_TEMP" - set +e - output=$(bare/bin/python -c "import psycodict" 2>&1) - status=$? - set -e - echo "$output" - if [ $status -eq 0 ]; then - echo "::error::importing psycodict without psycopg should fail" - exit 1 - fi - case "$output" in - *psycopg\[binary\]*) echo "helpful message present" ;; - *) echo "::error::missing the install-psycopg hint"; exit 1 ;; - esac + plain/bin/python - <` is convenience syntax a real database attribute wins over -- + so adding a method in a minor release never makes a table unreachable through + `db[name]`. + ### Release candidates 1.0.0 is published as a sequence of release candidates first. `pip` ignores diff --git a/DataManagement.md b/DataManagement.md index 0554b22..854f3b8 100644 --- a/DataManagement.md +++ b/DataManagement.md @@ -47,7 +47,7 @@ Renaming a table renames its indexes and constraints with it, by substituting th * **the `id` column** is added automatically as a `bigint` primary key unless you already list one; use `id_type=` to choose a different integer type. Columns are physically laid out ordered by type (widest alignment first) for storage efficiency, so the on-disk column order is not your declaration order — but every file operation is header-driven, so this never matters to you. * **`label_col`** names the column used by `lookup`; it must be one of the search columns, or `None`. * **`sort`** is the default sort order: a list of column names or `(column, 1|-1)` pairs. - * **`id_ordered`** defaults to `True` when `sort` is given, `False` otherwise. It records the intent that, in production, the `id` column runs in the same order as `sort` (which lets some range queries use the primary-key index). It does **not** sort anything now; see [resorting is disabled](#resorting-is-disabled). + * **`id_ordered`** defaults to `True` when `sort` is given, `False` otherwise. It records the intent that, in production, the `id` column runs in the same order as `sort` (which lets some range queries use the primary-key index). See [resorting](#resorting). The most common Postgres types are `smallint`/`integer`/`bigint`, `numeric` (exact), `real`/`double precision`, `text`, `boolean`, `jsonb`, `timestamp`, and the array forms (`integer[]`, `numeric[]`, ...). @@ -64,6 +64,14 @@ Copies the column layout, `label_col`, `sort`, `id_ordered`, `id` type and descr psycodict keeps its own bookkeeping in a handful of tables that live alongside your search tables: +psycodict operates in one PostgreSQL schema, `public` unless the constructor is +given another (`PostgresDatabase(schema="myschema")`). Every relation it +creates goes there, every relation it looks for is looked for there, and every +catalog query is filtered to it, so a relation of the same name in another +schema is neither mistaken for one of these nor merged with it. The schema is +pinned in each connection's `search_path`, including replacement connections +after a reconnect. + * **`meta_tables`** — one row per search table, holding `name`, `sort`, `count_cutoff`, `id_ordered`, `out_of_order`, `stats_valid`, `label_col`, `total`, `important` and `include_nones`. This is the source of truth psycodict reads on connection to reconstruct each table object. * **`meta_indexes`** and **`meta_constraints`** — one row per index / constraint, recording how to rebuild it. `reload` and `restore_indexes` rebuild from these rows, **not** from whatever is physically on the table (see [reload](#reload)). * **`meta_tables_hist`**, **`meta_indexes_hist`**, **`meta_constraints_hist`** — versioned history of the three tables above, so that `reload_meta`/`revert_meta` can roll a table's metadata forward and back. @@ -108,11 +116,70 @@ These mutate the live table directly. They are convenient for small edits; for * **`update(query, changes, resort=False, restat=True)`** — a plain SQL `UPDATE` of every row matching `query`; `changes` maps column names to constants. * **`delete(query, restat=True)`** — deletes every row matching `query` and decrements `total`. -**Statistics invalidation.** Any write that can change the data calls `_break_stats`, which sets `meta_tables.stats_valid = false` so that cached statistics are known to be stale. If the table has `saving` on and you left `restat=True`, statistics are refreshed at the end of the call; otherwise they are simply marked invalid. Inserting rows (and updating a sort-key column) also calls `_break_order`, setting `out_of_order = true` to record that the `id` order no longer matches `sort`; `delete` leaves the order flag alone. +**Statistics invalidation.** Any write that can change the data calls `_break_stats`, which sets `meta_tables.stats_valid = false` so that cached statistics are known to be stale. If the table has `saving` on and you left `restat=True`, statistics are refreshed at the end of the call; otherwise they are simply marked invalid. + +`stats_valid` is enforced rather than merely recorded: while it is false, a +cached nonempty-query count, distinct count, minimum, maximum or sum reports a +miss, and the method computes the answer instead of returning the stored one. +The empty-query `total` is the exception, since it is maintained on every write +and so stays exact. + +`column_counts`, `numstats` and `null_counts` are the other exception, and a +caveat worth knowing. The line is what a cache miss costs: the counts above +fall back to a single statement about the rows in question, while these fall +back to rebuilding a whole statistics family, or to one full count per search +column. Making them miss while the table is invalid would rebuild on every +call and never converge, since only `refresh_stats()` restores the flag, so +they read what is recorded. A value recorded before an unrefreshed write is +therefore still reported by them; run `refresh_stats()` after a write you did +not `restat`. The flag goes back to true only in `refresh_stats()`, inside +the 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. +Refreshing a `_tmp` copy does not validate the live table. + +Bulk paths also run PostgreSQL's own `ANALYZE`, which is a different thing from +psycodict's statistics: a freshly loaded relation has no planner statistics +until autovacuum reaches it. Replacement tables are analyzed while still named +`_tmp`, before the swap, since the catalog entry follows the relation through +the rename; `copy_from` analyzes the live table it loaded into. Inserting rows (and updating a sort-key column) also calls `_break_order`, setting `out_of_order = true` to record that the `id` order no longer matches `sort`; `delete` leaves the order flag alone. `resort()` (or a rebuild through `reload`/`rewrite`) sets it back to `false`. + +### Resorting + +`table.resort()` rebuilds the table so that ascending `id` matches the +configured sort, laying the rows out on disk in that order. It is a full-table +operation: the data is dumped, loaded into a fresh table whose ids are assigned +`1..N` in sort order, and swapped into place through the same machinery `reload` +uses, so the primary key, indexes, constraints, grants and counts/stats +companions are rebuilt, `ANALYZE` is run, and the previous table is kept as a +`name_old` backup. **The ids change** — anything outside psycodict that +stored them as durable identifiers must be updated — and the operation needs +temporary disk space for the replacement, its indexes, the dump and the backup. +By default a table already marked ordered reports that there is nothing to do; +pass `force=True` to rebuild anyway. + +The earlier in-place implementation renumbered every id with an `UPDATE`, which +stalled replication and left the rows in their old physical order (defeating the +point of id-ordering); it was disabled, and is now replaced by this rebuild. + +Because a resort is expensive, it is no longer a side effect of a small write. +`resort=True` on `insert_many`, `update`, `copy_from` or an in-place +`update_from_file` **raises**, pointing at `resort()`; `resort=False` (the +default) is unaffected. The replacement operations that rebuild the whole table +anyway — `reload`, `rewrite`, and a non-inplace `update_from_file` — still +establish id order as part of building the new table. The workflow for a +maintained table is therefore: -### Resorting is disabled +```python +table.update(...) # marks out_of_order +# later, in a maintenance window: +table.resort() # rebuild in sort order +``` -`resort()` is a **no-op on this branch**: it prints `resorting disabled` and returns `None` without touching the table. In-place resorting was found to stall replication and to not persist correctly on disk, and since the tables are effectively read-only in production the supported way to renumber ids is to dump the data in sorted order and `reload` it. Consequently the `resort=` keyword on `insert_many`, `update`, `copy_from`, `reload` and `update_from_file` currently does nothing. +`scripts/audit_id_order.py` checks the flag against reality: it streams each +`id_ordered` table in sort order and reports `OK`, `MISMATCH` or an error +without modifying anything. Use it before trusting `out_of_order = false` on a +table whose history includes non-inplace updates, which could have left the flag +stale. ### Locking @@ -131,8 +198,9 @@ This is the heart of psycodict. The workflow is: `copy_to` a table (or the curr ### The file format -A **search file** begins with three header lines followed by one line per row: +A **search file** begins with an optional format marker, then three header lines, then one line per row: + 0. optionally, `# psycodict-export-format: N` — the file format version. A file without this line is format 0, the historical layout, so every file psycodict has ever written still loads; new files psycodict writes carry the current version. A reader refuses a file whose version it does not understand before loading any data. This is the *file* format, distinct from the database *metadata* format of the `meta_*` tables (see MetadataFormats.md); 1. the column names, separated by the delimiter (default `|`), with `id` first if present; 2. the Postgres type of each column, in the same order; 3. a blank line. diff --git a/MANIFEST.in b/MANIFEST.in new file mode 100644 index 0000000..190855c --- /dev/null +++ b/MANIFEST.in @@ -0,0 +1,30 @@ +# What goes into the source distribution beyond the package itself. +# +# setuptools already ships the psycodict/ package, README.md, LICENSE and +# pyproject.toml. The sdist is also meant to be a complete, testable checkout: +# the whole test suite (conftest.py included -- without it the shipped tests do +# not run), the maintenance scripts, the narrative guides the docs are built +# from, and the project metadata files. + +# The test suite, in full. conftest.py holds the fixtures every test uses. +graft tests +include tests/conftest.py + +# Read-only maintenance scripts (e.g. the id-order audit). +graft scripts + +# The narrative guides (also the sources the Sphinx docs include) and the +# project metadata. +include *.md +include CITATION.cff +include CHANGELOG.md +include CONTRIBUTING.md +include SECURITY.md + +# The documentation sources, so the sdist can rebuild the docs. +graft docs +prune docs/_build + +# Housekeeping: no caches or compiled files in the archive. +global-exclude __pycache__ +global-exclude *.py[cod] diff --git a/README.md b/README.md index 664eaf9..2d96906 100644 --- a/README.md +++ b/README.md @@ -12,14 +12,17 @@ database in sync. ## Install -psycodict runs on [psycopg 3](https://www.psycopg.org/psycopg3/). psycopg is an -optional dependency so that you can choose between the binary and pure-Python -builds; install psycodict with one of the two extras: +psycodict runs on [psycopg 3](https://www.psycopg.org/psycopg3/), which is a +plain dependency, so a bare install works on a system that has `libpq`: -``` -pip install "psycodict[pgbinary]" # pulls in psycopg[binary]; no system libpq needed -pip install "psycodict[pgsource]" # pulls in pure-Python psycopg, which uses your system libpq -``` +| Command | psycopg it installs | +|---|---| +| `pip install psycodict` | pure-Python, using your system `libpq` | +| `pip install "psycodict[pgbinary]"` | precompiled, with a bundled `libpq` — the simplest install, no system `libpq` needed | +| `pip install "psycodict[pgc]"` | locally compiled against your system `libpq` (needs build tools) | + +`pgbinary` is the easiest; the plain install keeps psycodict usable as a library +in an environment that manages `libpq` itself. ## Quickstart @@ -128,7 +131,7 @@ See the [CHANGELOG](https://github.com/roed314/psycodict/blob/main/CHANGELOG.md) - Python 3.9 or newer. - PostgreSQL 13 through 18. -- psycopg 3.2.4 or newer (installed through the `pgbinary` or `pgsource` extra above). +- psycopg 3.2.4 or newer (a plain dependency; the `pgbinary`/`pgc` extras above pick a build). Python 3.9 and PostgreSQL 13 are already past their upstream end of life; psycodict keeps supporting them as legacy compatibility for downstream diff --git a/Searching.md b/Searching.md index 2ea0fb0..6849e78 100644 --- a/Searching.md +++ b/Searching.md @@ -22,9 +22,11 @@ change without notice. Code outside psycodict should call only the non-underscore methods described below (on the table, and on its public `table.stats` attribute). -A search table is reached as `db.` (equivalently `db[""]`) -on a `PostgresDatabase`. Every example in this document uses `db` for the -database and a table variable such as `nf = db.nf_fields`. +A search table is reached as `db[""]` on a `PostgresDatabase`, and +as `db.` when the name is not shadowed by a real attribute or method +of the database object (see [Versioning.md](Versioning.md#reaching-a-table)). +Every example in this document uses `db` for the database and a table variable +such as `nf = db.nf_fields`. ## Contents diff --git a/Versioning.md b/Versioning.md index 9283cd8..c209ecc 100644 --- a/Versioning.md +++ b/Versioning.md @@ -9,12 +9,19 @@ metadata tables living inside your database. ## What is public - * **Non-underscore names** in the `psycodict` package that are documented — in - the specification documents ([QueryLanguage.md](QueryLanguage.md), - [Searching.md](Searching.md), [DataManagement.md](DataManagement.md), - [MetadataFormats.md](MetadataFormats.md)) or in docstrings. Names with a - leading underscore are private, whatever module they live in, and may change - in any release. + * **The names each module exports in its `__all__`**, and their documented + behavior. A name is public exactly when it appears in its module's + `__all__`; that single test decides it, with no exceptions. It covers + explicitly exported special names such as `psycodict.__version__`, and any + name *absent* from `__all__` — underscore-prefixed or not, docstring or not — + is implementation: an explicit `from psycodict.utils import range_formatter` + still resolves, but the name is not part of this promise and may change in + any release. (`from psycodict.utils import *`, on the other hand, binds + `__all__` and nothing else, so a wildcard import binds fewer names than it + did before 1.0.) The `__all__` names are exactly the names in the + [API reference](https://psycodict.readthedocs.io/en/latest/api/index.html), + and the snapshot test `tests/test_public_api.py` freezes them, so the promise + and the code cannot drift apart. * **The query language** as specified in [QueryLanguage.md](QueryLanguage.md): the meaning of a query dictionary is stable within a major version: new features may be added in minor versions, but functioning queries will @@ -23,19 +30,36 @@ metadata tables living inside your database. Identifier, ...`). Downstream code should import these from `psycodict` rather than from the driver; the re-export point is the stable name. * **The export file format** written by `copy_to` and read by `copy_from` / - `reload` (three header lines, `|` delimiter, `\N` nulls — see - [DataManagement.md](DataManagement.md)): files written by one 1.x release - can be loaded by any other. + `reload` (see [DataManagement.md](DataManagement.md)). A file may begin with + a `# psycodict-export-format: N` marker, and a file without one is format 0, + the historical layout. A **newer** 1.x reader reads files written by an + earlier 1.x release; an **older** reader is not guaranteed to understand a + file a newer release wrote (it may carry a format the older reader predates), + and a reader refuses a marked file whose version it does not understand + rather than mis-loading it. An incompatible change to the file format + requires a new major version. * **The `meta_*` tables**, whose layout is governed by the metadata format protocol below. +## Reaching a table + +`db[name]` is the canonical, collision-free way to reach a search table, and the +one covered by this promise. `db.` (attribute access) is convenience +syntax for the same lookup, with one caveat: a real attribute or method of the +database object wins over a table of the same name, so a table called `config` +or `tablenames` is reachable only through `db["config"]`. Adding a method to +the database class in a minor release therefore never makes a table inaccessible +through the canonical `db[name]` lookup, even if it shadows an attribute-access +name. + ## What is not covered -Underscore-prefixed names; the exact SQL text psycodict emits (only its -semantics); performance characteristics; the contents of log files; and +Names absent from their module's `__all__`, whether or not they carry a +docstring and whatever they are named; the exact SQL text psycodict emits (only +its semantics); performance characteristics; the contents of log files; and undocumented behavior generally, even where observable. If something -undocumented matters to your project, open an issue — turning it into -documented (hence stable) behavior is usually easy. +undocumented matters to your project, open an issue — turning it into documented +(hence stable) behavior is usually easy. ## Database metadata compatibility @@ -44,9 +68,12 @@ number stored in each database (`meta_format`, with a `min_compat` column declaring the oldest client format the database still admits); the protocol — including how clients degrade gracefully against older databases and when a migration is required — is specified in -[MetadataFormats.md](MetadataFormats.md). The format number is bumped only at -major releases, so within 1.x a database migrated once is understood by every -client. +[MetadataFormats.md](MetadataFormats.md). The metadata format number and the +export file format number are protocol revisions in their own right, not the +package major version: a compatible, additive revision may ship in a minor +release while keeping `min_compat` low enough for existing clients. What +requires a new major version is a change that raises `min_compat` past an +earlier 1.x client, or otherwise makes that client unsafe. ## Deprecation policy diff --git a/config.ini.example b/config.ini.example index 9a2a3d5..238da5b 100644 --- a/config.ini.example +++ b/config.ini.example @@ -13,4 +13,4 @@ host = localhost port = 5432 user = postgres password = -dbname = lmfdb +dbname = postgres diff --git a/docs/api/index.md b/docs/api/index.md index 19b4c32..2568adf 100644 --- a/docs/api/index.md +++ b/docs/api/index.md @@ -1,9 +1,17 @@ # API reference -Generated from the docstrings. The map of the library: +Generated from the docstrings. Each page documents exactly its module's +`__all__` — the supported surface [Versioning.md](../Versioning.md) promises to +keep across 1.x. The map of the library: +- {mod}`psycodict` — the package root: `__version__` and the names re-exported + for convenience ({class}`~psycodict.utils.DelayCommit` and the SQL + composition classes `SQL`, `Identifier`, `Placeholder`, `Literal`, + `Composable`, `Composed`). - {mod}`psycodict.database` — {class}`~psycodict.database.PostgresDatabase`, - the connection object; each table in the database is an attribute of it. + the connection object; access a table canonically as `db[name]`. Attribute + access `db.` is shorthand for the same lookup when the table name is + not shadowed by a real attribute or method of the database object. - {mod}`psycodict.searchtable` — {class}`~psycodict.searchtable.PostgresSearchTable`, the read API (`search`, `lucky`, `lookup`, `count`, `random`, …) driven by the query @@ -36,6 +44,7 @@ Generated from the docstrings. The map of the library: ```{toctree} :maxdepth: 1 +package database searchtable table diff --git a/docs/api/package.md b/docs/api/package.md new file mode 100644 index 0000000..606ebfa --- /dev/null +++ b/docs/api/package.md @@ -0,0 +1,18 @@ +# psycodict + +The package root. Its `__all__` is the version marker and the names psycodict +re-exports for convenience, so that downstream code need not import them from a +submodule or from the driver; everything else lives in the modules below. + +The autodoc options here are deliberately wider than on the module pages: seven +of the eight root exports are *imported* members (`imported-members`) and +`__version__` is a special data member (`special-members`), so without them +Sphinx would silently render an empty page for a module whose entire public +surface is re-exports. The options are local to this page — the module pages +keep documenting exactly their own `__all__`. + +```{eval-rst} +.. automodule:: psycodict + :imported-members: + :special-members: __version__ +``` diff --git a/docs/conf.py b/docs/conf.py index a0d81a1..b2f8f81 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -52,9 +52,16 @@ # documentation conventions (INPUT:/OUTPUT: bullet blocks, EXAMPLES:: with # literal transcripts); those are plain reST, so autodoc renders them as-is. autodoc_member_order = "bysource" +# No "undoc-members": with __all__ defined, autodoc considers only the names a +# module exports, and this keeps it from documenting the ones it cannot -- so a +# helper that merely carries a docstring no longer lands in the reference just +# because it has one. That is an upper bound, not a proof of parity: autodoc +# also *drops* an exported name it has no docstring for, and (on a page without +# "imported-members"/"special-members") an exported name that is an import or a +# dunder. tests/test_public_api.py supplies the parity half -- every __all__ +# name has a docstring, and every module in the frozen surface has a page here. autodoc_default_options = { "members": True, - "undoc-members": True, "show-inheritance": True, } diff --git a/psycodict/__init__.py b/psycodict/__init__.py index e9c9c79..6ef7fca 100644 --- a/psycodict/__init__.py +++ b/psycodict/__init__.py @@ -33,6 +33,8 @@ # Single source of truth for the package version: pyproject.toml reads it via # ``[tool.setuptools.dynamic]``, and it works from an uninstalled checkout too. +#: The version of psycodict, as a :pep:`440` string. For an installed copy it +#: is the same value ``importlib.metadata.version("psycodict")`` reports. __version__ = "1.0.0rc2" try: @@ -52,3 +54,17 @@ from psycopg.sql import SQL, Identifier, Placeholder, Literal, Composable, Composed assert SQL and Identifier and Placeholder and Literal and Composable and Composed + +# The names psycodict re-exports at the package root. The Postgres* classes and +# the rest of the interface are imported from their submodules; see each +# module's __all__ and Versioning.md for the full public API. +__all__ = [ + "__version__", + "SQL", + "Identifier", + "Placeholder", + "Literal", + "Composable", + "Composed", + "DelayCommit", +] diff --git a/psycodict/base.py b/psycodict/base.py index ae61f04..cee283b 100644 --- a/psycodict/base.py +++ b/psycodict/base.py @@ -50,6 +50,13 @@ validate_column_type, ) +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "PostgresBase", +] + ################################################################## # meta_* infrastructure # @@ -91,6 +98,24 @@ def jsonb_idx(cols, cols_type): # upgrade=True). META_FORMAT = 1 +# The format of a search-data export file (the searchfile of copy_to/copy_from/ +# reload), which is a *different* thing from the metadata format above: this +# describes the layout of a data file, not of the meta_* tables. A file may +# begin with a line ``# psycodict-export-format: N``; a file without one is +# format 0, the historical layout of just names, types and a blank line, so +# every file psycodict has ever written still reads. New files are written at +# EXPORT_FORMAT; a reader refuses a version it does not understand before +# loading any data. See Versioning.md for the compatibility promise. +EXPORT_FORMAT = 1 +EXPORT_FORMAT_MARKER = "# psycodict-export-format:" + + +def export_format_line(): + """ + The marker line a new search-data export begins with, without a newline. + """ + return "%s %s" % (EXPORT_FORMAT_MARKER, EXPORT_FORMAT) + _meta_tables_cols = ( "name", @@ -423,14 +448,25 @@ def _table_exists(self, tablename): - ``tablename`` -- a string, the name of the table """ - cur = self._execute(SQL("SELECT 1 FROM pg_tables where tablename=%s"), [tablename], silent=True) + cur = self._execute( + SQL("SELECT 1 FROM pg_tables WHERE schemaname = %s AND tablename = %s"), + [self._db.schema, tablename], + silent=True, + ) return cur.fetchone() is not None def _all_tablenames(self): """ Return all (postgres) table names in the database """ - return [rec[0] for rec in self._execute(SQL("SELECT tablename FROM pg_tables ORDER BY tablename"), silent=True)] + return [ + rec[0] + for rec in self._execute( + SQL("SELECT tablename FROM pg_tables WHERE schemaname = %s ORDER BY tablename"), + [self._db.schema], + silent=True, + ) + ] def _get_locks(self): return self._execute(SQL( @@ -535,15 +571,18 @@ def _index_exists(self, indexname, tablename=None): """ if tablename: cur = self._execute( - SQL("SELECT 1 FROM pg_indexes WHERE indexname = %s AND tablename = %s"), - [indexname, tablename], + SQL( + "SELECT 1 FROM pg_indexes " + "WHERE schemaname = %s AND indexname = %s AND tablename = %s" + ), + [self._db.schema, indexname, tablename], silent=True, ) return cur.fetchone() is not None else: cur = self._execute( - SQL("SELECT tablename FROM pg_indexes WHERE indexname=%s"), - [indexname], + SQL("SELECT tablename FROM pg_indexes WHERE schemaname = %s AND indexname = %s"), + [self._db.schema, indexname], silent=True, ) table = cur.fetchone() @@ -560,7 +599,13 @@ def _relation_exists(self, name): - ``name`` -- a string, the name of the relation """ - cur = self._execute(SQL("SELECT 1 FROM pg_class where relname = %s"), [name]) + cur = self._execute( + SQL( + "SELECT 1 FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace " + "WHERE n.nspname = %s AND c.relname = %s" + ), + [self._db.schema, name], + ) return cur.fetchone() is not None def _constraint_exists(self, constraintname, tablename=None): @@ -582,9 +627,9 @@ def _constraint_exists(self, constraintname, tablename=None): cur = self._execute( SQL( "SELECT 1 from information_schema.table_constraints " - "WHERE table_name=%s and constraint_name=%s" + "WHERE table_schema = %s AND table_name = %s AND constraint_name = %s" ), - [tablename, constraintname], + [self._db.schema, tablename, constraintname], silent=True, ) return cur.fetchone() is not None @@ -592,9 +637,9 @@ def _constraint_exists(self, constraintname, tablename=None): cur = self._execute( SQL( "SELECT table_name from information_schema.table_constraints " - "WHERE constraint_name=%s" + "WHERE table_schema = %s AND constraint_name = %s" ), - [constraintname], + [self._db.schema, constraintname], silent=True, ) table = cur.fetchone() @@ -608,8 +653,8 @@ def _list_indexes(self, tablename): Lists built index names on the search table ``tablename`` """ cur = self._execute( - SQL("SELECT indexname FROM pg_indexes WHERE tablename = %s"), - [tablename], + SQL("SELECT indexname FROM pg_indexes WHERE schemaname = %s AND tablename = %s"), + [self._db.schema, tablename], silent=True, ) return [elt[0] for elt in cur] @@ -629,9 +674,9 @@ def _list_constraints(self, tablename): " ON rel.oid = con.conrelid " "INNER JOIN pg_catalog.pg_namespace nsp " " ON nsp.oid = connamespace " - "WHERE rel.relname = %s" + "WHERE nsp.nspname = %s AND rel.relname = %s" ), - [tablename], + [self._db.schema, tablename], silent=True, ) return [elt[0] for elt in cur] @@ -809,9 +854,9 @@ def _column_types(self, table_name, data_types=None): cur = self._execute( SQL( "SELECT column_name, udt_name::regtype FROM information_schema.columns " - "WHERE table_name = %s ORDER BY ordinal_position" + "WHERE table_schema = %s AND table_name = %s ORDER BY ordinal_position" ), - [tname], + [self._db.schema, tname], ) else: cur = data_types[tname] @@ -838,8 +883,11 @@ def _relation_columns(self, table): one about columns. """ cur = self._execute( - SQL("SELECT column_name FROM information_schema.columns WHERE table_name = %s"), - [table], + SQL( + "SELECT column_name FROM information_schema.columns " + "WHERE table_schema = %s AND table_name = %s" + ), + [self._db.schema, table], silent=True, commit=False, ) @@ -1300,9 +1348,9 @@ def target_name(name, tablename, kind): def _read_header_lines(self, F, sep="|"): """ - Reads the header lines from a file - (row of column names, row of column types, blank line). - Returning the dictionary of columns and their types. + Reads the header lines from a search-data file (an optional format + marker, the row of column names, the row of column types, and the blank + line), returning the columns and their types. INPUT: @@ -1313,8 +1361,33 @@ def _read_header_lines(self, F, sep="|"): A list of pairs where the first entry is the column and the second the corresponding type + + A file may begin with ``# psycodict-export-format: N``. A file without + one is format 0, the historical layout, so every file psycodict has + ever written still reads. A version newer than this psycodict + understands is refused here, before any of the data is loaded. """ - names = [x.strip() for x in F.readline().strip().split(sep)] + first = F.readline() + marker = first.strip() + if marker.startswith(EXPORT_FORMAT_MARKER): + version_text = marker[len(EXPORT_FORMAT_MARKER):].strip() + try: + version = int(version_text) + except ValueError: + raise ValueError( + "Malformed export-format marker: %r" % (marker,) + ) + if version > EXPORT_FORMAT: + raise ValueError( + "This file is psycodict export format %s, but this psycodict " + "understands only up to format %s; upgrade psycodict to read " + "it" % (version, EXPORT_FORMAT) + ) + names_line = F.readline() + else: + # No marker: format 0, and this first line is the column names. + names_line = first + names = [x.strip() for x in names_line.strip().split(sep)] types = [x.strip() for x in F.readline().strip().split(sep)] blank = F.readline() if blank.strip(): diff --git a/psycodict/config.py b/psycodict/config.py index 9b9320c..efff655 100644 --- a/psycodict/config.py +++ b/psycodict/config.py @@ -17,6 +17,13 @@ from collections import defaultdict from copy import deepcopy +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "Configuration", +] + def strbool(s): """ @@ -196,7 +203,10 @@ def __init__(self, parser=None, defaults={}, writeargstofile=False, readargs=Fal dest="postgresql_dbname", metavar="DBNAME", help="PostgreSQL database name [default: %(default)s]", - default=defaults.get("postgresql_dbname", "lmfdb"), + # A generic default, not an LMFDB one: "postgres" is the + # database libpq itself falls back to. Deployments name their + # own database in the config file or constructor. + default=defaults.get("postgresql_dbname", "postgres"), ) def sec_opt(key): diff --git a/psycodict/database.py b/psycodict/database.py index 7cd95f9..2b9b61d 100644 --- a/psycodict/database.py +++ b/psycodict/database.py @@ -47,6 +47,7 @@ check_new_table_name, derived_identifier, physical_table_name, + validate_schema_name, validate_search_table_name, validate_search_table_registry, ) @@ -54,6 +55,13 @@ from .searchtable import PostgresSearchTable from .utils import DelayCommit, safe_child_path +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "PostgresDatabase", +] + # The registry of metadata-format migrations. Entry N describes the step # from format N-1 to format N; MetadataFormats.md has the checklist a new # format must follow. @@ -347,6 +355,11 @@ def _configure_session(self, conn): # Note that it has some global effects, since register_adapter # is not limited to just one connection setup_connection(conn) + # Pin the schema first: everything below, and every statement this + # connection later runs, resolves unqualified names in it. pg_catalog + # is still searched -- PostgreSQL puts it first implicitly when it is + # not named -- so the built-in types and functions stay reachable. + conn.execute("SELECT set_config('search_path', %s, false)", [self.schema]) for name, value in self._session_settings.items(): # set_config takes both as bound values, so nothing is interpolated conn.execute("SELECT set_config(%s, %s, false)", [name, str(value)]) @@ -387,7 +400,7 @@ def query(sql, args): "SELECT count(*) FROM information_schema.role_table_grants " "WHERE grantee = %s AND table_schema = %s " "AND privilege_type IN (" + ",".join(["%s"] * len(privileges)) + ")", - [user, "public"] + privileges, + [user, self.schema] + privileges, ) read_only = rows[0][0] == 0 @@ -405,12 +418,12 @@ def query(sql, args): rows = sorted(query( "SELECT table_name, privilege_type " "FROM information_schema.role_table_grants " - "WHERE grantee = %s AND table_name IN (" + "WHERE grantee = %s AND table_schema = %s AND table_name IN (" + ",".join(["%s"] * len(knowls_tables)) + ") AND privilege_type IN (" + ",".join(["%s"] * len(privileges)) + ")", - [user] + knowls_tables + privileges, + [user, self.schema] + knowls_tables + privileges, )) read_and_write_knowls = rows == sorted( [(table, priv) for table in knowls_tables for priv in privileges] @@ -521,12 +534,20 @@ def _register_object(self, obj): obj.conn = self.conn self._objects.append(obj) - def __init__(self, config=None, secretsfile=None, create=False, upgrade=False, - session_settings=None, grant_policy=None, **kwargs): + def __init__(self, config=None, create=False, upgrade=False, + session_settings=None, grant_policy=None, schema="public", **kwargs): if config is None: from .config import Configuration config = Configuration() self.config = config + # The one schema this database operates in. Every relation psycodict + # creates goes here, every relation it looks for is looked for here, + # and every catalog query is filtered to it -- so that a table of the + # same name in another schema can neither stand in for one of these nor + # be merged with it. Checked once, here, rather than at each use. + # Deliberately not part of _connect_kwargs: it is psycodict's own + # setting, and psycopg.connect has no such parameter. + self.schema = validate_schema_name(schema) self.server_side_counter = 0 self._nocommit_stack = 0 self._silenced = False @@ -576,9 +597,9 @@ def __init__(self, config=None, secretsfile=None, create=False, upgrade=False, # Refuse to run against a database that still uses the removed # search/extras table split legacy = self._execute(SQL( - "SELECT 1 FROM information_schema.columns WHERE table_schema = 'public' " + "SELECT 1 FROM information_schema.columns WHERE table_schema = %s " "AND table_name = 'meta_tables' AND column_name = 'has_extras'" - )) + ), [self.schema]) if legacy.rowcount: cur = self._execute(SQL("SELECT name FROM meta_tables WHERE has_extras")) if cur.rowcount: @@ -637,8 +658,9 @@ def refresh_tables(self): """ cur = self._execute(SQL( "SELECT table_name, column_name, udt_name::regtype " - "FROM information_schema.columns ORDER BY table_name, ordinal_position" - )) + "FROM information_schema.columns WHERE table_schema = %s " + "ORDER BY table_name, ordinal_position" + ), [self.schema]) data_types = {} for table_name, column_name, regtype in cur: if table_name not in data_types: @@ -794,9 +816,9 @@ def _grantees(self, table_name): cur = self._execute( SQL( "SELECT DISTINCT grantee FROM information_schema.role_table_grants " - "WHERE table_name = %s AND grantee <> grantor" + "WHERE table_schema = %s AND table_name = %s AND grantee <> grantor" ), - [table_name], + [self.schema, table_name], silent=True, ) return {rec[0] for rec in cur} @@ -925,12 +947,12 @@ def _schema_relations(self, relkinds=_TABLE_RELKINDS): query = ( "SELECT c.relname FROM pg_class c " "JOIN pg_namespace n ON n.oid = c.relnamespace " - "WHERE n.nspname = current_schema()" + "WHERE n.nspname = %s" ) - values = None + values = [self._db.schema] if relkinds is not None: query += " AND c.relkind = ANY(%s)" - values = [list(relkinds)] + values.append(list(relkinds)) cur = self._execute(SQL(query), values, silent=True) return {rec[0] for rec in cur} @@ -1033,10 +1055,10 @@ def table_sizes(self): pg_total_relation_size(reltoastrelid) AS toast_bytes FROM pg_class c LEFT JOIN pg_namespace n ON n.oid = c.relnamespace - WHERE n.nspname = 'public' AND relkind = 'r' + WHERE n.nspname = %s AND relkind = 'r' ) a""" sizes = defaultdict(lambda: defaultdict(int)) - cur = self._execute(SQL(query)) + cur = self._execute(SQL(query), [self.schema]) for ( table_name, row_estimate, @@ -1337,8 +1359,8 @@ def _bootstrap_meta(self): existing = { rec[0] for rec in self._execute(SQL( "SELECT table_name FROM information_schema.tables " - "WHERE table_schema = 'public'" - )) + "WHERE table_schema = %s" + ), [self.schema]) } stored, _ = self._stored_meta_format() fmt = META_FORMAT if stored is None else min(stored, META_FORMAT) @@ -2411,7 +2433,13 @@ def tablespaces(self): """ Returns a dictionary giving giving the tablespace for all tables """ - D = {rec[0]: rec[1] for rec in self._execute(SQL("SELECT tablename, tablespace FROM pg_tables"))} + D = { + rec[0]: rec[1] + for rec in self._execute( + SQL("SELECT tablename, tablespace FROM pg_tables WHERE schemaname = %s"), + [self.schema], + ) + } return {name: space if space else "" for (name, space) in D.items()} def compare(self, other, tables=None, row_counts=True, null_counts=False, exact=False): diff --git a/psycodict/dbdiff.py b/psycodict/dbdiff.py index 56680e3..05bb816 100644 --- a/psycodict/dbdiff.py +++ b/psycodict/dbdiff.py @@ -24,6 +24,14 @@ from psycopg.sql import SQL, Identifier +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "compare_databases", + "format_differences", +] + # The columns of meta_tables that are compared for tables present in both # databases (in this order). The remaining columns are deliberately left # out: total is reported through row_counts instead, out_of_order and @@ -88,9 +96,9 @@ def _column_types(db, names): "FROM pg_attribute a " "JOIN pg_class c ON a.attrelid = c.oid " "JOIN pg_namespace n ON c.relnamespace = n.oid " - "WHERE n.nspname = 'public' AND c.relkind = 'r' " + "WHERE n.nspname = %s AND c.relkind = 'r' " "AND a.attnum > 0 AND NOT a.attisdropped" - )) + ), [db.schema]) columns = {} for table_name, column_name, typ in cur: if table_name in names: diff --git a/psycodict/encoding.py b/psycodict/encoding.py index 5747624..507531c 100644 --- a/psycodict/encoding.py +++ b/psycodict/encoding.py @@ -8,6 +8,15 @@ import datetime import math from psycopg.adapt import Dumper + +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "Json", + "Array", + "copy_dumps", +] try: try: # this fails in sage 9.3 diff --git a/psycodict/grants.py b/psycodict/grants.py index 4769c22..370640f 100644 --- a/psycodict/grants.py +++ b/psycodict/grants.py @@ -26,6 +26,14 @@ from .validation import InvalidDefinitionError, MAX_IDENTIFIER_LENGTH +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "GrantPolicy", + "LMFDBGrantPolicy", +] + # The actions a policy can grant. A policy manages exactly these: privileges # outside this set (TRUNCATE, REFERENCES, TRIGGER) are left alone. GRANT_ACTIONS = ("SELECT", "INSERT", "UPDATE", "DELETE") diff --git a/psycodict/notifications.py b/psycodict/notifications.py index 6152601..e6f2378 100644 --- a/psycodict/notifications.py +++ b/psycodict/notifications.py @@ -93,6 +93,13 @@ import psycopg from psycopg.sql import SQL, Identifier +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "NotificationListener", +] + # The single channel on which psycodict announces schema changes. The payload # is the affected table's name. See the module docstring for the contract. SCHEMA_CHANNEL = "psycodict_schema" diff --git a/psycodict/searchtable.py b/psycodict/searchtable.py index 4e55dea..0e835d9 100644 --- a/psycodict/searchtable.py +++ b/psycodict/searchtable.py @@ -21,10 +21,21 @@ from .encoding import Json from .utils import IdentifierWrapper, DelayCommit, filter_sql_injection, postgres_infix_ops +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "PostgresSearchTable", +] + # psycopg3 splits plain and server-side cursors into two classes # (psycopg2 had a single cursor class, which this name used to alias) pg_cursor = (Cursor, ServerCursor) +# The sampling strategies random_sample accepts, upper-cased because SYSTEM and +# BERNOULLI go into the TABLESAMPLE clause verbatim. +_RANDOM_SAMPLE_MODES = ("SYSTEM", "BERNOULLI", "CHOICE") + def _qualify(frag, tablename): """ @@ -1642,6 +1653,11 @@ def random(self, query={}, projection=0, pick_first=None): """ if pick_first: colvals = self.distinct(pick_first, query) + if not colvals: + # No row satisfies the query, so there is no value to pick; + # random.choice([]) would raise IndexError instead of + # returning the documented None. + return None query = dict(query) query[pick_first] = random.choice(colvals) return self.random(query, projection) @@ -1680,10 +1696,10 @@ def random(self, query={}, projection=0, pick_first=None): # a temporary hack FIXME # maxid = self.max('id') maxid = self.max_id() - # max_id returns -1 on an empty table (MAX(id) is NULL), so - # testing for 0 sent an empty table into randint(0, -1); - # anything below 1 means there are no rows. - if maxid < 1: + # max_id returns -1 on an empty table (MAX(id) is NULL). That is + # the only empty sentinel: 0 is a legitimate id, so a table whose + # single row has id 0 must not be reported as empty. + if maxid < 0: return None # a temporary hack FIXME minid = self.min_id() @@ -1693,7 +1709,11 @@ def random(self, query={}, projection=0, pick_first=None): # rid = random.randint(1, maxid) rid = random.randint(minid, maxid) res = self.lucky({"id": rid}, projection=projection) - if res: + # lucky returns None when no row has that id. Anything else is + # a hit, including a projection whose value is 0, False, "" or + # an empty list -- testing truthiness discarded those rows and + # could exhaust maxtries on a table full of them. + if res is not None: return res raise RuntimeError("Random selection failed!") @@ -1719,7 +1739,21 @@ def random_sample(self, ratio, query={}, projection=1, mode=None, repeatable=Non mode = "bernoulli" else: mode = "choice" + if not isinstance(mode, str): + raise ValueError( + "mode must be one of %s or None, not %s" + % (", ".join(map(repr, _RANDOM_SAMPLE_MODES)), type(mode).__name__) + ) mode = mode.upper() + # Checked before any work is done: an unrecognized mode used to fall + # through every branch below and return None, which reads like an empty + # result rather than a mistake. + if mode not in _RANDOM_SAMPLE_MODES: + raise ValueError( + "%r is not a valid mode; use one of %s, or None to choose " + "between 'bernoulli' and 'choice' by result count" + % (mode.lower(), ", ".join(map(repr, _RANDOM_SAMPLE_MODES))) + ) search_cols = self._parse_projection(projection) if ratio > 1 or ratio <= 0: raise ValueError("Ratio must be a positive number between 0 and 1") @@ -1728,9 +1762,11 @@ def random_sample(self, ratio, query={}, projection=1, mode=None, repeatable=Non elif mode == "CHOICE": results = list(self.search(query, projection, sort=[])) count = int(len(results) * ratio) - if repeatable is not None: - random.seed(repeatable) - return random.sample(results, count) + # A local generator, so asking for a repeatable sample does not + # reseed the process-wide random module and make every other + # caller's sequence repeat with it. + rng = random if repeatable is None else random.Random(repeatable) + return rng.sample(results, count) elif mode in ["SYSTEM", "BERNOULLI"]: cols = SQL(", ").join(self._column_composable(c) for c in search_cols) if repeatable is None: diff --git a/psycodict/slowlog.py b/psycodict/slowlog.py index 83fdce3..2c40182 100644 --- a/psycodict/slowlog.py +++ b/psycodict/slowlog.py @@ -40,6 +40,15 @@ from heapq import heapify, heappop, heappush from math import ceil, floor, log10 +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "parse_slow_log", + "slow_query_report", + "show_slow_report", +] + _TIMESTAMP_RE = re.compile(r"^(\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2},\d{3}) - ") _ANSI_RE = re.compile("\x1b\\[[0-9;]*m") _NUMBER = r"(?:[0-9]+\.?[0-9]*|\.[0-9]+)(?:[eE][-+]?[0-9]+)?" diff --git a/psycodict/statstable.py b/psycodict/statstable.py index 92d575e..52eb6c4 100644 --- a/psycodict/statstable.py +++ b/psycodict/statstable.py @@ -21,10 +21,17 @@ from psycopg.sql import SQL, Identifier, Literal from .base import PostgresBase -from .validation import physical_table_name +from .validation import column_type_sql, physical_table_name from .encoding import Json, numeric_converter from .utils import DelayCommit, KeyedDefaultDict, make_tuple +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "PostgresStatsTable", +] + # The following is used in bucketing for statistics pg_to_py = {} for typ in [ @@ -269,6 +276,54 @@ def _get_tablespace(self): # We use the same tablespace for stats and counts tables as for the main search table return self.table._get_tablespace() + def _may_use_cache(self, suffix=""): + """ + Whether a cached count or statistic may be used as an answer. + + ``stats_valid`` is an assertion that every cached nonempty-query count, + distinct count and custom statistic for the live table agrees with the + live data. A write that does not refresh them clears it, and until a + refresh restores it a cached row is a stale answer, not an answer -- so + every lookup that would serve one is routed through here and reports a + miss instead, leaving the caller to compute or recompute. + + Two things are deliberately outside this rule: + + - the empty-query ``total``, which is maintained on every write and + stays usable regardless (see :meth:`quick_count`); + - a suffixed table. A ``_tmp`` or ``_oldN`` copy carries its own + counts and stats, built or loaded together with its data, and + ``stats_valid`` says nothing about them. + + Reads that report what the cache *contains*, rather than answering a + question about the data -- ``_status``, ``status``, ``extra_counts`` -- + are also not gated: ``refresh_stats`` uses them to discover what to + recompute, so gating them would make an invalid table forget what + statistics it is supposed to have. + + The line this draws is whether a miss costs one bounded query or a + rebuild. ``quick_count``, ``quick_count_distinct`` and + ``_quick_statistic`` each fall back to a single statement about the + rows in question, so a miss is affordable and they are gated. + ``_has_stats`` and ``_has_numstats`` decide whether a whole statistics + family needs computing, and ``null_counts`` falls back to one full + count *per search column*; gating those makes ``column_counts``, + ``numstats`` and ``null_counts`` rebuild on every call with nothing to + converge on, since only ``refresh_stats`` restores the flag. Measured + on the LMFDB, that took a four-minute downstream suite past + forty-five, mostly inside ``null_counts`` over ``nf_fields`` and + friends. + + What that leaves is a real gap: a value recorded before an unrefreshed + write is still reported by ``column_counts``, ``numstats`` and + ``null_counts``. Closing it needs freshness per statistic rather than + one flag per table, which is a metadata format change; + ``refresh_stats()`` is the remedy meanwhile. + """ + if suffix: + return True + return self.table._stats_valid + def _has_stats(self, jcols, ccols, cvals, threshold, split_list=False, threshold_inequality=False, suffix=""): """ Checks whether statistics have been recorded for a given set of columns. @@ -284,6 +339,15 @@ def _has_stats(self, jcols, ccols, cvals, threshold, split_list=False, threshold rows are thrown away. - ``split_list`` -- whether entries of lists should be counted once for each entry. - ``threshold_inequality`` -- if true, then any lower threshold will still count for having stats. + + Deliberately *not* gated on ``stats_valid``: this answers "is this + statistic recorded", which is what ``add_stats`` and ``column_counts`` + use to decide whether to compute it. Reporting False while the table + is invalid makes them recompute the whole family on every call, and + since nothing but ``refresh_stats`` restores the flag, they never stop + -- measured on the LMFDB, that turned a four-minute test suite into one + still running after forty-five. See :meth:`_may_use_cache` for what + that costs in staleness. """ if split_list: values = [jcols, "split_total"] @@ -322,7 +386,11 @@ def quick_count(self, query, split_list=False, suffix="", startup=False): Either an integer giving the number of results, or None if not cached. """ if not query and not startup: + # The empty-query total is maintained on every write, so it is + # exact even when the rest of the cache is not. return self.total + if not self._may_use_cache(suffix): + return None cols, vals = self._split_dict(query) selecter = SQL( "SELECT count FROM {0} WHERE cols = %s AND values = %s AND split = %s" @@ -542,8 +610,11 @@ def quick_count_distinct(self, cols, query={}, suffix=""): OUTPUT: - Either an integer giving the number of distinct values, or None if not cached. + Either an integer giving the number of distinct values, or None if not + cached or if the cache may not be used. """ + if not self._may_use_cache(suffix): + return None ccols, cvals = self._split_dict(query) selecter = SQL("SELECT value FROM {0} WHERE stat = %s AND cols = %s AND constraint_cols = %s AND constraint_values = %s").format(Identifier(self.stats + suffix)) cur = self._execute(selecter, ["distinct", Json(cols), ccols, cvals]) @@ -736,6 +807,8 @@ def _quick_statistic(self, col, ccols, cvals, kind="max"): the constraint columns take on these values. - ``kind`` -- either "min" or "max" or "sum" """ + if not self._may_use_cache(): + return None constraint = SQL("constraint_cols = %s AND constraint_values = %s") values = [kind, Json([col]), ccols, cvals] selecter = SQL( @@ -1290,6 +1363,9 @@ def _has_numstats(self, jcol, cgcols, cvals, threshold, suffix=""): - ``threshold`` -- an integer: if the number of rows with a given tuple of values for the grouping columns is less than this threshold, those rows are thrown away. + + Not gated on ``stats_valid``, for the reason given in + :meth:`_has_stats`. """ values = [jcol, "ntotal", cgcols, cvals] if threshold is None: @@ -1651,21 +1727,27 @@ def _approx_most_common(self, col, n): """ if col not in self.table.search_cols: raise ValueError("Column %s not a search column for %s" % (col, self.search_table)) + # reltuples has to come from the table these frequencies are about. It + # used to be read from a hard-coded public.nf_fields, so on any other + # table the estimate was that table's frequencies scaled by an + # unrelated row count. selecter = SQL( """SELECT v.{0}, (c.reltuples * freq)::int as estimate_ct FROM pg_stats s CROSS JOIN LATERAL - unnest(s.most_common_vals::text::""" - + self.table.col_type[col] - + """[] + unnest(s.most_common_vals::text::{1}[] , s.most_common_freqs) WITH ORDINALITY v ({0}, freq, ord) CROSS JOIN ( - SELECT reltuples FROM pg_class - WHERE oid = regclass 'public.nf_fields') c -WHERE schemaname = 'public' AND tablename = %s AND attname = %s + SELECT c.reltuples FROM pg_class c + JOIN pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = %s AND c.relname = %s) c +WHERE schemaname = %s AND tablename = %s AND attname = %s ORDER BY v.ord LIMIT %s""" - ).format(Identifier(col)) - cur = self._execute(selecter, [self.search_table, col, n]) + ).format(Identifier(col), column_type_sql(self.table.col_type[col])) + schema = self._db.schema + cur = self._execute( + selecter, [schema, self.search_table, schema, self.search_table, col, n] + ) return [tuple(x) for x in cur] def _common_cols(self, threshold=700): @@ -1844,6 +1926,12 @@ def refresh_stats(self, total=True, reset_None_to_1=False, suffix=""): # Refresh total in meta_tables self._set_total(self._slow_count({}, suffix=suffix, extra=False), suffix=suffix) self.refresh_null_counts(suffix=suffix) + if not suffix: + # Everything above ran in this transaction, so the caches now + # agree with the data and the table can be marked valid with + # them. A suffixed refresh is rebuilding some other relation's + # caches and says nothing about the live table. + self.table._restore_stats() self._logger.info("Refreshed statistics in %.3f secs" % (time.time() - t0)) def status(self, reset_None_to_1=False): diff --git a/psycodict/table.py b/psycodict/table.py index a7a24a5..531d79f 100644 --- a/psycodict/table.py +++ b/psycodict/table.py @@ -22,7 +22,7 @@ from psycopg.sql import SQL, Identifier, Placeholder, Literal from .encoding import Json, check_copy_sep, copy_dumps -from .base import PostgresBase, _meta_table_name +from .base import PostgresBase, _meta_table_name, export_format_line from .utils import DelayCommit, IdentifierWrapper, LockError from .base import ( _meta_cols_types_jsonb_idx, @@ -49,6 +49,13 @@ ) from .statstable import PostgresStatsTable +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "PostgresTable", +] + assert _operator_classes and _valid_storage_params @@ -328,7 +335,10 @@ def _get_tablespace(self): """ Determine the tablespace hosting this table (which is then used for indexes and constraints) """ - cur = self._execute(SQL("SELECT tablespace FROM pg_tables WHERE tablename=%s"), [self.search_table]) + cur = self._execute( + SQL("SELECT tablespace FROM pg_tables WHERE schemaname = %s AND tablename = %s"), + [self._db.schema, self.search_table], + ) return cur.fetchone()[0] def _create_index_statement(self, name, table, type, columns, modifiers, storage_params, whereclause=None): @@ -1182,6 +1192,20 @@ def _break_stats(self): self._execute(updater, [self.search_table], silent=True) self._stats_valid = False + def _restore_stats(self): + """ + Record that the cached counts and statistics agree with the live data. + + The counterpart of :meth:`_break_stats`, and the only way the flag goes + back to true. Call it from inside the transaction that rebuilt or + loaded the caches, so that a failure part-way through leaves the table + marked invalid rather than claiming a cache it does not have. + """ + if not self._stats_valid: + updater = SQL("UPDATE meta_tables SET stats_valid = true WHERE name = %s") + self._execute(updater, [self.search_table], silent=True) + self._stats_valid = True + def _break_order(self): """ This function should be called when the id ordering is invalidated by an insertion or update. @@ -1192,17 +1216,24 @@ def _break_order(self): self._execute(updater, [self.search_table], silent=True) self._out_of_order = True - def finalize_changes(self): + def _forbid_row_level_resort(self, resort, method): """ - Intended to finish off a batch of data changes by updating the - cached total, refreshing statistics targets, and re-sorting by id. - Currently a placeholder that does nothing. + Reject ``resort=True`` on a row-level write. + + A small write must not silently trigger a full dump-and-rebuild of the + table: ``resort()`` rebuilds and swaps, which on a large table is an + expensive, disk-hungry, ID-renumbering operation, not a step of an + ``update`` or an ``insert_many``. ``resort=False`` (the default) is + fine; ``resort=True`` points the caller at the explicit method. """ - # TODO - # Update stats.total - # Refresh stats targets - # Sort and set self._out_of_order - pass + if resort: + raise ValueError( + "%s no longer resorts as part of the write: %s.resort() is now " + "an explicit full-table rebuild that renumbers ids and needs " + "temporary disk space, so it must be called deliberately. " + "Finish the write, then call table.resort() in a maintenance " + "window." % (method, "table") + ) def _forbid_reindex_false(self, reindex, inplace): """ @@ -1226,7 +1257,7 @@ def rewrite( # Keyword-only so that old positional calls (which had reindex fifth, # before restat) fail loudly instead of silently binding to restat. *, - resort=True, + resort=None, restat=True, tostr_func=None, datafile=None, @@ -1252,7 +1283,10 @@ def rewrite( - ``func`` -- a function that takes a record (dictionary) as input and returns the modified record - ``query`` -- a query dictionary; only rows satisfying this query will be changed - - ``resort`` -- whether to resort the table after running the rewrite + - ``resort`` -- whether to resort the table after running the rewrite. + Defaults to resorting a non-inplace (replacement) rewrite and not an + in-place one; ``resort=True`` on an in-place rewrite raises, since + the in-place path cannot resort (call ``resort()`` afterward). - ``restat`` -- whether to recompute statistics after running the rewrite - ``tostr_func`` -- a function to be used when writing data to the temp file defaults to copy_dumps from encoding @@ -1274,6 +1308,12 @@ def rewrite( """ # Fail before the expensive dump below rather than deep inside update_from_file self._forbid_reindex_false(kwds.get("reindex"), kwds.get("inplace")) + if resort is None: + # Default: resort on the non-inplace (replacement) path, as before; + # an in-place rewrite edits rows on the live table and cannot + # resort, so do not ask it to. An explicit resort=True on an + # in-place rewrite still raises, in update_from_file. + resort = not kwds.get("inplace", False) sep = kwds.get("sep", "|") # An unusable separator would otherwise be rejected only by COPY itself, # after func has already been run over every row of the table. @@ -1305,7 +1345,9 @@ def rewrite( tot = self.count(query) try: with datafile: - # write headers + # write headers, including the export-format marker so this + # file reads the same way copy_to's do + datafile.write(export_format_line() + "\n") datafile.write(sep.join(data_cols) + "\n") datafile.write( sep.join(self.col_type.get(col) for col in data_cols) @@ -1350,7 +1392,7 @@ def update_from_file( resort=None, reindex=None, restat=True, - logging={"operation":"file_update"}, + logging=None, **kwds ): """ @@ -1373,10 +1415,18 @@ def update_from_file( - ``resort`` -- whether this table should be resorted after updating (default is to resort when the sort columns intersect the updated columns) - ``reindex`` -- only meaningful when ``inplace`` is set: whether to drop the indexes touching the updated columns before the update and recreate them afterward, which is faster when many rows change (by default this is done when more than 1000 rows are updated). Without ``inplace``, all indexes are necessarily recreated on the replacement table, so ``reindex=True`` is redundant and ``reindex=False`` raises an error. - ``restat`` -- whether to recompute stats for the table - - ``logging`` -- a dictionary of keyword arguments for _log_db_change + - ``logging`` -- a dictionary of keyword arguments for _log_db_change. + A copy is taken, so the caller's dictionary is not modified and two + calls sharing one dictionary do not see each other's ``logid``. - ``kwds`` -- passed on to the ``COPY`` command. Cannot include "columns". """ self._forbid_reindex_false(reindex, inplace) + if inplace: + # The in-place path edits the live table's rows; it cannot resort + # without an expensive rebuild, so like the other row-level writes + # it points at resort(). A non-inplace update swaps in a new table + # and may establish order as part of building it. + self._forbid_row_level_resort(resort, "update_from_file with inplace=True") if not inplace: # The non-inplace update clones the search table to a _tmp copy # and rebuilds its indexes with _tmp names, just like reload, so @@ -1384,8 +1434,12 @@ def update_from_file( # The counts and stats tables are not checked: this method # deliberately reuses their _tmp versions when they exist. self._check_tmp_leftovers([self.search_table]) - logid = self._check_locks(logging["operation"], datafile=datafile) - logging["aborted"] = True + # Copied rather than used directly: this dictionary is mutated below, + # and the default used to be a shared literal, so consecutive default + # calls carried the previous call's logid and aborted flag. + log_data = {"operation": "file_update"} if logging is None else dict(logging) + logid = self._check_locks(log_data["operation"], datafile=datafile) + log_data["aborted"] = True try: sep = kwds.get("sep", "|") print("Updating %s from %s..." % (self.search_table, datafile)) @@ -1464,6 +1518,19 @@ def drop_tmp(): Identifier(tmp_table), Identifier(self.search_table), Identifier(label_col))) + # Renumber into sort order before the keys are rebuilt: on the + # non-inplace path the renumber replaces the _tmp table, so it + # must run while that table has no primary key or indexes. The + # in-place path cannot renumber (resort=True was rejected up + # front); if it changed a sort-key column it just records that + # the id order no longer matches. + if not inplace and self._id_ordered and resort: + self._generate_sorted_ids(suffix=suffix) + ordered = True + else: + ordered = False + if inplace and resort and self._id_ordered: + self._break_order() if reindex and inplace: # also restores constraints self.restore_indexes(columns[1:]) @@ -1472,10 +1539,6 @@ def drop_tmp(): self.restore_indexes(suffix=suffix) # We also need to recreate the primary key self.restore_pkeys(suffix=suffix) - if self._id_ordered and resort: - ordered = self.resort(suffix=suffix) - else: - ordered = False if restat and self.stats.saving: if not inplace: for table in [self.stats.counts, self.stats.stats]: @@ -1505,11 +1568,11 @@ def drop_tmp(): self._set_ordered() # Delete the temporary table used to load the data drop_tmp() - logging["logid"] = logid - logging["aborted"] = False + log_data["logid"] = logid + log_data["aborted"] = False print("Updated %s in %.3f secs" % (self.search_table, time.time() - now)) finally: - self._log_db_change(**logging) + self._log_db_change(**log_data) def delete(self, query, restat=True): """ @@ -1553,6 +1616,7 @@ def update(self, query, changes, resort=False, restat=True): - ``resort`` -- whether to resort the table afterward - ``restat`` -- whether to recompute statistics afterward """ + self._forbid_row_level_resort(resort, "update") logid = self._check_locks("update") aborted = True try: @@ -1577,8 +1641,6 @@ def update(self, query, changes, resort=False, restat=True): self._execute(updater, change_values + values) self._break_order() self._break_stats() - if resort: - self.resort() if restat and self.stats.saving: self.stats.refresh_stats(total=False) aborted = False @@ -1704,6 +1766,7 @@ def insert_many(self, data, resort=False, reindex=None, restat=True): If the search table has an id, the dictionaries will be updated with the ids of the inserted records, though note that those ids will change if the ids are resorted. """ + self._forbid_row_level_resort(resort, "insert_many") logid = self._check_locks("insert_many") aborted = True search_data = [] @@ -1751,8 +1814,6 @@ def insert_many(self, data, resort=False, reindex=None, restat=True): ) self._break_order() self._break_stats() - if resort: - self.resort() if reindex: self.restore_pkeys() self.restore_indexes(search_cols) @@ -1763,77 +1824,132 @@ def insert_many(self, data, resort=False, reindex=None, restat=True): finally: self._log_db_change("insert_many", aborted=aborted, logid=logid, nrows=len(search_data)) - def resort(self, suffix="", sort=None): + def _generate_sorted_ids(self, suffix=""): """ - Restores the sort order on the id column. - The id sequence might have gaps after resorting. - See: https://www.postgresql.org/docs/current/functions-sequence.html + Rebuild ``{search}{suffix}`` so that ascending ``id`` follows the + configured sort, laying the rows out on disk in that order too. + + This is a physical rebuild, not an in-place ``UPDATE`` of every id: the + rows are copied into a fresh table in sort order, with ids drawn in + that order from a new sequence, and that table replaces the original. + So the on-disk order matches the id order -- the point of id-ordering, + which an in-place renumber does not achieve -- and there is no table + bloat or replication stall from rewriting every row. + + Must run *before* the primary key and indexes are (re)built on the + ``{suffix}`` table, since it drops and recreates that table. It is the + internal step reload and non-inplace update_from_file use; the public + entry point is :meth:`resort`. ``self._sort`` must be set. + """ + target = self.search_table + suffix + if self._sort is None: + raise ValueError( + "Cannot resort %s: it has no configured sort" % (self.search_table,) + ) + # Transient, named nowhere else, so shortened to fit rather than left + # for the server to truncate. + resorting = derived_identifier(target, "_resort") + seq = derived_identifier(target, "_resort_seq") + search_cols = SQL(", ").join(Identifier(c) for c in self.search_cols) + # A fresh table with the same columns and storage but no indexes. + self._clone(target, resorting) + try: + self._execute( + SQL("CREATE TEMP SEQUENCE {0} MINVALUE 1 START 1 CACHE 10000").format( + Identifier(seq) + ) + ) + # nextval is evaluated as rows leave the sort, so the ids run + # 1..N in sort order, and the INSERT lays them down in that order. + self._execute( + SQL( + "INSERT INTO {0} (id, {1}) " + "SELECT nextval({2}), {1} FROM {3} ORDER BY {4}, id" + ).format( + Identifier(resorting), + search_cols, + Literal(seq), + Identifier(target), + self._sort, + ) + ) + self._execute(SQL("DROP SEQUENCE {0}").format(Identifier(seq))) + self._execute(SQL("DROP TABLE {0}").format(Identifier(target))) + self._execute( + SQL("ALTER TABLE {0} RENAME TO {1}").format( + Identifier(resorting), Identifier(target) + ) + ) + except Exception: + # Leave no half-built _resort table behind for the next attempt's + # _clone to trip over. + self._execute( + SQL("DROP TABLE IF EXISTS {0}").format(Identifier(resorting)) + ) + raise + + def resort(self, force=False): + """ + Rebuild the table so that ascending ``id`` matches the configured + sort, and lay the rows out on disk in that order. + + This is an explicit, potentially expensive full-table operation, not a + step of an ordinary write. The table is dumped, loaded into a fresh + table whose ids are assigned 1..N in sort order, and swapped into + place, reusing :meth:`reload`'s machinery: the primary key, indexes, + constraints, grant policy and counts/stats companions are rebuilt, + ``ANALYZE`` is run, and the previous table is kept as an ``_oldN`` + backup so the operation can be reverted with ``reload_revert``. + + **The ids change.** Anything outside psycodict that stored these ids + as durable identifiers must be updated. The operation needs temporary + disk space for the replacement table, its indexes, the dump file and + the backup. INPUT: - - ``suffix`` -- a string such as "_tmp" or "_old1" to be appended to the names in the command. - - ``sort`` -- -- a list, either of strings (which are interpreted as column names - in the ascending direction) or of pairs (column name, 1 or -1). - If None, will use ``self._sort_orig``. - """ - - print("resorting disabled") - # resorting without a reload makes replication stall - # and doesn't store data correctly on disk - # Given that our tables are readonly, we should just dump sorted and reload - return None - search_table = Identifier(self.search_table + suffix) - # Transient: created and dropped inside this transaction, and named - # nowhere else, so these are shortened rather than left to the server. - tmp_table = Identifier(derived_identifier(self.search_table + suffix, "_sorter")) - tmp_seq = Identifier( - derived_identifier(self.search_table + suffix, "_sorter_newid_seq") - ) - sort_order = self._sort if sort is None else self._sort_str(sort) - if sort_order is None: - print("resort failed, no sort order given") + - ``force`` -- rebuild even when the table is already marked ordered + (``id_ordered`` and not ``out_of_order``). By default that case + reports that there is nothing to do and returns without work. + + OUTPUT: + + ``True`` if the table was rebuilt, ``False`` if it was already ordered + and ``force`` was not set. + """ + if not self._id_ordered: + raise ValueError( + "%s is not id_ordered; call set_sort() to give it a sort before " + "resorting" % (self.search_table,) + ) + if self._sort is None: + raise ValueError( + "%s has no configured sort to order by" % (self.search_table,) + ) + if not self._out_of_order and not force: + print( + "%s is already ordered; pass force=True to rebuild anyway" + % (self.search_table,) + ) return False - logid = self._check_locks("resort", suffix=suffix) + logid = self._check_locks("resort") aborted = True + # Dumped without ids, then reloaded: reload clones a fresh table, + # loads the rows, calls _generate_sorted_ids to renumber them in sort + # order, and swaps -- so grants, indexes, the backup and ANALYZE all + # come from the one replacement path rather than being duplicated here. + datafile = tempfile.NamedTemporaryFile("w", delete=False) + datafile.close() try: - with DelayCommit(self, silence=True): - if (self._id_ordered and self._out_of_order) or suffix: - now = time.time() - # we will use a temporary table to avoid ACCESS EXCLUSIVE lock - self._execute(SQL( - "CREATE TEMP SEQUENCE {0} MINVALUE 0 START 0 CACHE 10000" - ).format(tmp_seq)) - - id_type = column_type_sql(self.col_type["id"]) - self._execute(SQL( - "CREATE TEMP TABLE {0} (oldid {2}, newid {3} NOT NULL DEFAULT nextval('{1}')) ON COMMIT DROP" - ).format(tmp_table, tmp_seq, id_type, id_type)) - - self._execute(SQL( - "ALTER SEQUENCE {0} OWNED BY {1}.newid" - ).format(tmp_seq, tmp_table)) - - self._execute(SQL( - "INSERT INTO {0} " - "SELECT id as oldid FROM {1} ORDER BY {2}" - ).format(tmp_table, search_table, sort_order)) - self.drop_pkeys(suffix=suffix) - self._execute(SQL( - "UPDATE {0} SET id = {1}.newid " - "FROM {1} WHERE {0}.id = {1}.oldid" - ).format(search_table, tmp_table)) - self.restore_pkeys(suffix=suffix) - if not suffix: - self._set_ordered() - print("Resorted %s in %.3f secs" % (self.search_table, time.time() - now)) - elif self._id_ordered and not self._out_of_order: - print(f"Table {self.search_table} already sorted") - else: # not self._id_ordered - print("Data does not have an id column to be sorted") + now = time.time() + self.copy_to(datafile.name, include_id=False) + self.reload(datafile.name, resort=True, final_swap=True) + print("Resorted %s in %.3f secs" % (self.search_table, time.time() - now)) aborted = False return True finally: - self._log_db_change("resort", logid=logid, aborted=aborted, sort_order=sort_order) + os.unlink(datafile.name) + self._log_db_change("resort", logid=logid, aborted=aborted) def _set_ordered(self): """ @@ -1859,7 +1975,10 @@ def _write_header_lines(self, F, cols, sep="|", include_id=True): if include_id and cols and cols[0] != "id": cols = ["id"] + cols types = [self.col_type[col] for col in cols] - F.write("%s\n%s\n\n" % (sep.join(cols), sep.join(types))) + F.write( + "%s\n%s\n%s\n\n" + % (export_format_line(), sep.join(cols), sep.join(types)) + ) def _staged_label_index_name(self, suffix="_tmp"): """ @@ -1937,6 +2056,24 @@ def _next_backup_number(self): ) return backup_number + def _analyze(self, tables, suffix=""): + """ + Refresh PostgreSQL's planner statistics for the given relations. + + These are the server's own statistics, not the counts and stats + psycodict maintains: a relation that has just been bulk loaded has none + until autovacuum reaches it, and until then the planner costs queries + against it as though it were tiny. + + Run on a ``_tmp`` copy before the swap rather than on the live table + after it, so that no query is served by an unanalyzed relation; the + catalog entry follows the relation through the rename. + """ + for table in tables: + self._execute( + SQL("ANALYZE {0}").format(Identifier(table + suffix)), silent=True + ) + def _swap_in_tmp(self, tables): """ Helper function for ``reload``: appends _old{n} to the names of tables/indexes/pkeys @@ -1947,6 +2084,10 @@ def _swap_in_tmp(self, tables): - ``tables`` -- a list of tables to rename (e.g. self.search_table, self.stats.counts, self.stats.stats) """ now = time.time() + # Before the swap, and outside its transaction: the _tmp relations are + # complete by now, and analyzing them here keeps the window in which + # the live names are locked as short as it was. + self._analyze(tables, "_tmp") backup_number = self._next_backup_number() with DelayCommit(self, silence=True): self._swap(tables, "", "_old" + str(backup_number)) @@ -2021,9 +2162,11 @@ def _check_tmp_leftovers(self, clone_tables=None): SQL( "SELECT rel.relname, con.conname FROM pg_constraint con " "JOIN pg_class rel ON rel.oid = con.conrelid " - "WHERE rel.relname = ANY(%s) AND con.conname ~ %s" + "JOIN pg_namespace nsp ON nsp.oid = rel.relnamespace " + "WHERE nsp.nspname = %s AND rel.relname = ANY(%s) " + "AND con.conname ~ %s" ), - [tables, pattern], + [self._db.schema, tables, pattern], silent=True, ) ] @@ -2036,9 +2179,9 @@ def _check_tmp_leftovers(self, clone_tables=None): for tbl, name in self._execute( SQL( "SELECT tablename, indexname FROM pg_indexes " - "WHERE tablename = ANY(%s) AND indexname ~ %s" + "WHERE schemaname = %s AND tablename = ANY(%s) AND indexname ~ %s" ), - [tables, pattern], + [self._db.schema, tables, pattern], silent=True, ) if (tbl, name) not in found @@ -2205,18 +2348,9 @@ def reload( % (table, time.time() - now, filename) ) - self.restore_pkeys(suffix=suffix) - - # update the indexes - # these are needed before restoring indexes - if indexesfile is not None: - # we do the swap at the end - self.reload_indexes(indexesfile, sep=sep) - if constraintsfile is not None: - self.reload_constraints(constraintsfile, sep=sep) - # Also restores constraints - self.restore_indexes(suffix=suffix) - + # Renumber ids in sort order before the keys are built: the + # renumber replaces the _tmp table, so it must run while that + # table still has no primary key or indexes to rebuild. if resort: if metafile: # read the metafile @@ -2253,11 +2387,24 @@ def reload( else: if not self._id_ordered: # this table doesn't need to be sorted resort = False - # tracks the success of resort - ordered = self.resort(suffix=suffix) + if resort: + self._generate_sorted_ids(suffix=suffix) + ordered = True else: ordered = False + self.restore_pkeys(suffix=suffix) + + # update the indexes + # these are needed before restoring indexes + if indexesfile is not None: + # we do the swap at the end + self.reload_indexes(indexesfile, sep=sep) + if constraintsfile is not None: + self.reload_constraints(constraintsfile, sep=sep) + # Also restores constraints + self.restore_indexes(suffix=suffix) + # Ensure stats/counts tables are backed up and new empty ones created if self.stats.saving: for table in [self.stats.counts, self.stats.stats]: @@ -2676,7 +2823,15 @@ def _staged_enter(self): def update_from_file(datafile, label_col=None, inplace=True, **kwds): if not inplace: raise ValueError("update_from_file on a staged table is always performed in place") - return unstaged_update_from_file(datafile, label_col=label_col, inplace=True, **kwds) + # Writes to the staged copy are in place on that copy, which is + # not the live table; resorting it inline would defeat the + # point of staging. Drop any resort request rather than letting + # the in-place guard reject it -- the caller resorts the live + # table after the commit, with resort(). + kwds.pop("resort", None) + return unstaged_update_from_file( + datafile, label_col=label_col, inplace=True, resort=False, **kwds + ) staged.update_from_file = update_from_file # With reindex set, insert_many drops the primary key around the @@ -2848,10 +3003,16 @@ def _staged_abort(self, logid): def max_id(self, table=None): """ The largest id occurring in the given table. Used in the random method. + + Returns -1 for a table with no rows, which is below every id psycodict + generates; callers distinguishing "empty" from "has rows" must test + ``< 0`` rather than ``< 1``, since 0 is a legitimate id. """ if table is None: table = self.search_table - res = self._execute(SQL("SELECT MAX(id) FROM {}".format(table))).fetchone()[0] + res = self._execute( + SQL("SELECT MAX(id) FROM {0}").format(Identifier(table)) + ).fetchone()[0] if res is None: res = -1 return res @@ -2860,10 +3021,16 @@ def max_id(self, table=None): def min_id(self, table=None): """ The smallest id occurring in the given table. Used in the random method. + + Returns 0 for a table with no rows. Unlike :meth:`max_id` that is not a + sentinel a caller can test for, since 0 is also a real id; pair it with + ``max_id() < 0`` to detect an empty table. """ if table is None: table = self.search_table - res = self._execute(SQL("SELECT MIN(id) FROM {}".format(table))).fetchone()[0] + res = self._execute( + SQL("SELECT MIN(id) FROM {0}").format(Identifier(table)) + ).fetchone()[0] if res is None: res = 0 return res @@ -2894,6 +3061,7 @@ def copy_from( If the search file contains ids, they should be contiguous, starting immediately after the current max id (or at 1 if empty). """ + self._forbid_row_level_resort(resort, "copy_from") self._check_file_input(searchfile, kwds) logid = self._check_locks("copy_from", datafile=searchfile) aborted = True @@ -2914,11 +3082,13 @@ def copy_from( ) print("Loaded data into %s in %.3f secs" % (self.search_table, time.time() - now)) self._break_order() - if self._id_ordered and resort: - self.resort() if reindex: self.restore_indexes() self._break_stats() + # A bulk COPY can change the table's size and distribution + # enough that the planner's statistics no longer describe it, + # and the stats refresh below plans against them. + self._analyze([self.search_table]) if self.stats.saving and restat: self.stats.refresh_stats(total=False) self.stats._update_total(search_count) diff --git a/psycodict/utils.py b/psycodict/utils.py index e2ed716..7493f5e 100644 --- a/psycodict/utils.py +++ b/psycodict/utils.py @@ -14,6 +14,16 @@ from psycopg.sql import SQL, Identifier, Placeholder +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "DelayCommit", + "IdentifierWrapper", + "LockError", + "SearchParsingError", +] + class SearchParsingError(ValueError): """ diff --git a/psycodict/validation.py b/psycodict/validation.py index edbede7..45e53dd 100644 --- a/psycodict/validation.py +++ b/psycodict/validation.py @@ -21,6 +21,14 @@ from psycopg.sql import SQL, Identifier +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "InvalidColumnTypeError", + "InvalidDefinitionError", +] + # This dictionary is used when creating new tables # The value associated to each type is the typlen from the pg_type table @@ -574,6 +582,19 @@ def utf8_prefix(name, max_bytes): return encoded[:max_bytes].decode("utf-8", "ignore") +def validate_schema_name(name): + """ + Check a PostgreSQL schema name psycodict will operate in. + + A schema name is an identifier like any other, and it is quoted wherever + psycodict emits it, so the rules are the identifier rules: not empty, no + control characters, and short enough that the server will not truncate it + into a different schema. It is checked once, in the constructor, rather + than at each use. + """ + return validate_relation_name(name, kind="Schema", max_length=MAX_IDENTIFIER_LENGTH) + + def catalog_identifier(name): """ The spelling the catalog holds ``name`` under. diff --git a/pyproject.toml b/pyproject.toml index 82ecad2..cbb8f96 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -13,6 +13,14 @@ description = "dictionary-based python interface to PostgreSQL databases" # has no long_description and the PyPI project page renders empty. readme = "README.md" requires-python = ">=3.9" +# psycopg is a plain dependency: ``pip install psycodict`` gets a working +# package on a system with libpq. The floor is 3.2.4 -- 3.2 introduced +# Connection.notifies(timeout=..., stop_after=...), which the notification +# listener uses, and 3.2.4 fixed notifications being dropped while no generator +# was consuming them, essential for the pull-based NotificationListener.poll(). +# The ``<4`` ceiling keeps an unreviewed future driver major out of a 1.x +# environment. +dependencies = ["psycopg>=3.2.4,<4"] authors = [{name = "David Roe", email = "roed.math@gmail.com"}, {name = "Edgar Costa", email = "edgarc@mit.edu"}] license = "GPL-2.0-or-later" keywords = ["postgres", "database", "interface"] @@ -37,12 +45,12 @@ Repository = "https://github.com/roed314/psycodict" Changelog = "https://github.com/roed314/psycodict/blob/main/CHANGELOG.md" [project.optional-dependencies] -# 3.2 introduced Connection.notifies(timeout=..., stop_after=...), which the -# notification listener uses, and 3.2.4 fixed notifications being dropped -# while no generator was consuming them -- essential for the pull-based -# NotificationListener.poll(). -pgsource = ["psycopg>=3.2.4"] -pgbinary = ["psycopg[binary]>=3.2.4"] +# In psycopg 3, plain ``psycopg`` is the pure-Python implementation using the +# system libpq; ``psycopg[binary]`` adds the bundled binary build (and +# satisfies the base dependency); ``psycopg[c]`` is a locally compiled +# extension using system libraries and build tools. +pgbinary = ["psycopg[binary]>=3.2.4,<4"] +pgc = ["psycopg[c]>=3.2.4,<4"] test = ["pytest>=7"] [tool.setuptools.dynamic] diff --git a/scripts/audit_id_order.py b/scripts/audit_id_order.py new file mode 100644 index 0000000..c6b65e5 --- /dev/null +++ b/scripts/audit_id_order.py @@ -0,0 +1,125 @@ +#!/usr/bin/env python3 +# -*- coding: utf-8 -*- +""" +Check whether the ``id_ordered`` tables really are ordered. + +A table marked ``id_ordered`` with ``out_of_order = false`` asserts that +ascending ``id`` is the configured default sort. Search code relies on that: +it may replace ``ORDER BY `` with ``ORDER BY id``. Historically the flag +could drift -- a non-inplace ``update_from_file`` left it stale -- so before +trusting it, audit it. + +This is read-only. For each selected table it streams the rows in configured +sort order (with ``id`` as the final tie-breaker) and checks that the ids come +out strictly increasing. Nothing is written. + +Usage:: + + python scripts/audit_id_order.py --all + python scripts/audit_id_order.py TABLE [TABLE ...] + +Exit status is 0 when every audited table is OK, 1 when any is out of order or +errored, 2 for a usage problem. + +The audit reads every row in sort order, which for a large table is a +substantial database sort; schedule it accordingly. Rows are streamed from the +server (a named cursor), so the client stays at constant memory however large +the table. +""" +import argparse +import sys + +from psycopg.sql import SQL, Identifier + + +def _server_side_rows(table): + """ + Stream ``(id,)`` for every row of ``table`` in configured-sort order plus + ``id`` as the final tie-breaker, using a named cursor so the whole table is + never materialized in the client. + """ + db = table._db + order = table._sort # an SQL fragment, or None + if order is None: + clause = SQL("ORDER BY id") + else: + clause = SQL("ORDER BY {0}, id").format(order) + query = SQL("SELECT id FROM {0} {1}").format(Identifier(table.search_table), clause) + # A server-side (named) cursor keeps the sort on the server and hands back + # rows in batches; see PostgresBase._cursor(buffered=True). + cur = db._cursor(buffered=True) + try: + cur.execute(query) + while True: + batch = cur.fetchmany(10000) + if not batch: + break + for row in batch: + yield row[0] + finally: + cur.close() + + +def audit_table(db, name): + """ + Return ``("OK", n)``, ``("MISMATCH", detail)`` or ``("ERROR", message)`` + for one table, reading it in sort order without modifying anything. + """ + if name not in db.tablenames: + return ("ERROR", "no such table") + table = db[name] + if not table._id_ordered: + return ("ERROR", "not id_ordered") + prev = None + count = 0 + try: + for rid in _server_side_rows(table): + if prev is not None and rid <= prev: + return ( + "MISMATCH", + "id %s follows id %s in sort order (row %s)" % (rid, prev, count + 1), + ) + prev = rid + count += 1 + except Exception as err: # pragma: no cover - surfaced to the operator + return ("ERROR", "%s: %s" % (type(err).__name__, err)) + return ("OK", count) + + +def main(argv=None): + parser = argparse.ArgumentParser(description=__doc__.splitlines()[1]) + group = parser.add_mutually_exclusive_group(required=True) + group.add_argument( + "--all", action="store_true", help="audit every id_ordered table" + ) + group.add_argument("tables", nargs="*", default=[], help="tables to audit") + args = parser.parse_args(argv) + + from psycodict import db + + if args.all: + names = sorted( + name for name in db.tablenames if db[name]._id_ordered + ) + if not names: + print("No id_ordered tables.") + return 0 + else: + names = args.tables + + worst = 0 + for name in names: + status, detail = audit_table(db, name) + if status == "OK": + print("OK %-40s %s rows" % (name, detail)) + elif status == "MISMATCH": + print("MISMATCH %-40s %s" % (name, detail)) + worst = max(worst, 1) + else: + print("ERROR %-40s %s" % (name, detail)) + worst = max(worst, 1) + return worst + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/test_correctness.py b/tests/test_correctness.py new file mode 100644 index 0000000..6f98d8c --- /dev/null +++ b/tests/test_correctness.py @@ -0,0 +1,257 @@ +# -*- coding: utf-8 -*- +""" +Regression tests for the correctness fixes in the rc3 review round. + +Each test here fails on the code as it stood at v1.0.0rc2. They are grouped by +the thing that was wrong rather than by the method, since several of them are +the same mistake made in two places: a value formatted into SQL text instead of +composed, and a result tested for truth instead of for existence. +""" +import random + +import pytest + +from psycopg.sql import SQL, Identifier + +import conftest + + +# --------------------------------------------------------------------------- +# identifiers in max_id / min_id +# --------------------------------------------------------------------------- + +# Names that are legal PostgreSQL identifiers once quoted, and that a bare +# "SELECT MAX(id) FROM %s" would either mis-parse or execute as extra SQL. +AWKWARD_NAMES = [ + "plain_name_9", + "has space", + 'has"quote', + "semi;colon", + "dash--dash", + "slash/*star", + "Ünïcødé", +] + + +@pytest.mark.parametrize("suffix", AWKWARD_NAMES) +def test_max_id_and_min_id_quote_the_table_they_are_given(db, empty_table, suffix): + """ + max_id/min_id take a table name as an argument and used to format it into + the statement as text. A name needing quotes was a syntax error, and one + containing a statement terminator was an injection. + """ + scratch = "t_%s_%s" % (suffix, empty_table.search_table[-8:]) + db._execute( + SQL("CREATE TABLE {0} (id bigint)").format(Identifier(scratch)) + ) + try: + db._execute( + SQL("INSERT INTO {0} (id) VALUES (3), (11)").format(Identifier(scratch)) + ) + assert empty_table.max_id(scratch) == 11 + assert empty_table.min_id(scratch) == 3 + finally: + db._execute(SQL("DROP TABLE {0}").format(Identifier(scratch))) + + +def test_max_id_does_not_execute_an_injected_statement(db, empty_table): + """ + The marker table must not exist afterwards: a name carrying its own + statement has to fail to resolve as a relation, not run. + """ + marker = "marker_%s" % empty_table.search_table[-8:] + injected = 'nonexistent"; CREATE TABLE %s (x int); --' % marker + with pytest.raises(Exception): + empty_table.max_id(injected) + db.conn.rollback() + assert not db._table_exists(marker) + + +def test_max_id_reports_empty_as_minus_one(empty_table): + assert empty_table.max_id() == -1 + + +# --------------------------------------------------------------------------- +# approximate statistics use the owning table +# --------------------------------------------------------------------------- + +def test_approx_most_common_scales_by_the_owning_table(db, table_factory): + """ + Frequencies came from the right table but reltuples came from a hard-coded + public.nf_fields, so on every other table the estimate was that table's + frequencies scaled by an unrelated row count. + + Two tables with the same value distribution and very different row counts + must therefore get very different estimates. + """ + small = table_factory() + big = table_factory() + small.insert_many([conftest.sample_row(i) for i in range(50)]) + big.insert_many([conftest.sample_row(i) for i in range(1000)]) + for table in (small, big): + db._execute(SQL("ANALYZE {0}").format(Identifier(table.search_table))) + + small_est = dict(small.stats._approx_most_common("flag", 2)) + big_est = dict(big.stats._approx_most_common("flag", 2)) + assert small_est and big_est + + # every row has flag set, so the estimates must bracket the real counts + assert sum(small_est.values()) == pytest.approx(50, rel=0.25) + assert sum(big_est.values()) == pytest.approx(1000, rel=0.25) + assert sum(big_est.values()) > 5 * sum(small_est.values()) + + +# --------------------------------------------------------------------------- +# update_from_file does not share log state between calls +# --------------------------------------------------------------------------- + +def _write_update(path, table, rows): + """ + A minimal update file: the label column first, as update_from_file requires, + then one column to change. + """ + cols = ["label", "num"] + with open(path, "w") as F: + F.write("|".join(cols) + "\n") + F.write("|".join(table.col_type[c] for c in cols) + "\n\n") + for label, num in rows: + F.write("%s|%s\n" % (label, num)) + + +def test_update_from_file_does_not_carry_log_state_between_calls(filled_table, tmp_path): + """ + The default was a shared dictionary literal that the method wrote logid and + aborted into, so the second default call started out holding the first + call's values -- and a caller who passed a dictionary got it modified. + """ + first = tmp_path / "first.txt" + second = tmp_path / "second.txt" + _write_update(first, filled_table, [("l0", 111)]) + _write_update(second, filled_table, [("l1", 222)]) + + filled_table.update_from_file(str(first), inplace=True, restat=False) + filled_table.update_from_file(str(second), inplace=True, restat=False) + + # Both updates landed, and the second call logged its own operation. + assert filled_table.lucky({"label": "l0"}, "num") == 111 + assert filled_table.lucky({"label": "l1"}, "num") == 222 + + # The default really is rebuilt per call. + import inspect + + from psycodict.table import PostgresTable + + default = inspect.signature(PostgresTable.update_from_file).parameters["logging"].default + assert default is None + + +def test_update_from_file_leaves_a_supplied_dictionary_alone(filled_table, tmp_path): + datafile = tmp_path / "u.txt" + _write_update(datafile, filled_table, [("l0", 333)]) + + supplied = {"operation": "caller_owned"} + filled_table.update_from_file( + str(datafile), inplace=True, restat=False, logging=supplied + ) + assert supplied == {"operation": "caller_owned"} + + +def test_update_from_file_leaves_a_supplied_dictionary_alone_on_failure( + filled_table, tmp_path +): + bad = tmp_path / "bad.txt" + bad.write_text("label|nosuchcolumn\ntext|text\n\nl0|x\n") + supplied = {"operation": "caller_owned"} + with pytest.raises(Exception): + filled_table.update_from_file( + str(bad), inplace=True, restat=False, logging=supplied + ) + filled_table._db.conn.rollback() + assert supplied == {"operation": "caller_owned"} + + +# --------------------------------------------------------------------------- +# random() edge cases +# --------------------------------------------------------------------------- + +def test_random_with_pick_first_returns_none_when_nothing_matches(filled_table): + """ + distinct() over a query nothing satisfies is empty, and random.choice([]) + raised IndexError where the documented behavior is None. + """ + assert filled_table.random({"n": -1}, pick_first="label") is None + + +def test_random_finds_the_only_row_when_its_id_is_zero(db, table_factory): + """ + -1 is the empty sentinel from max_id, so a table whose single row has id 0 + is not empty. Testing `maxid < 1` reported it as such. + """ + table = table_factory() + table.insert_many([conftest.sample_row(1)]) + db._execute( + SQL("UPDATE {0} SET id = 0").format(Identifier(table.search_table)) + ) + assert table.max_id() == 0 + assert table.random() == "l1" + + +def test_random_returns_a_false_valued_projection(db, table_factory): + """ + `if res:` discarded a row whose projected value was 0, False or "", so a + table of them exhausted maxtries and raised "Random selection failed!". + """ + table = table_factory() + table.insert_many([dict(conftest.sample_row(i), num=0) for i in range(20)]) + for _ in range(10): + assert table.random({}, "num") == 0 + + +def test_random_returns_none_for_an_empty_table(empty_table): + assert empty_table.random() is None + + +# --------------------------------------------------------------------------- +# random_sample() mode handling and RNG isolation +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize("mode", ["nonsense", "SYSTEMATIC", "", "choise"]) +def test_random_sample_rejects_an_unknown_mode(filled_table, mode): + """ + An unrecognized mode matched no branch and the method returned None, which + is indistinguishable from an empty result. + """ + with pytest.raises(ValueError, match="mode"): + filled_table.random_sample(0.5, mode=mode) + + +def test_random_sample_rejects_a_non_string_mode(filled_table): + with pytest.raises(ValueError, match="mode"): + filled_table.random_sample(0.5, mode=17) + + +@pytest.mark.parametrize("mode", ["system", "bernoulli", "choice", "CHOICE"]) +def test_random_sample_accepts_every_documented_mode(filled_table, mode): + result = filled_table.random_sample(0.5, mode=mode) + assert list(result) is not None + + +def test_repeatable_choice_sampling_leaves_the_global_rng_alone(filled_table): + """ + random.seed(repeatable) reseeded the process-wide generator, so asking for + a reproducible sample made every later random number in the program repeat. + """ + random.seed(12345) + baseline = [random.random() for _ in range(5)] + + random.seed(12345) + filled_table.random_sample(0.5, mode="choice", repeatable=99) + after = [random.random() for _ in range(5)] + + assert baseline == after + + +def test_repeatable_choice_sampling_is_still_repeatable(filled_table): + first = filled_table.random_sample(0.5, mode="choice", repeatable=7) + second = filled_table.random_sample(0.5, mode="choice", repeatable=7) + assert first == second diff --git a/tests/test_doctests.py b/tests/test_doctests.py index c50cfcd..e0850a6 100644 --- a/tests/test_doctests.py +++ b/tests/test_doctests.py @@ -171,6 +171,13 @@ def doc_tables(db): sort=["conductor_norm", "label"], ) db.test_curves.insert_many(_rows(CURVE_COLUMNS, CURVES)) + # insert_many invalidates the statistics, and a cached count may not be + # served while they are invalid. Nothing is cached yet and the totals are + # maintained by the inserts, so the caches do agree with the data; saying + # so puts both tables in the state a freshly loaded table is in, which is + # what the statistics examples assume. + db.test_fields._restore_stats() + db.test_curves._restore_stats() yield db # The namespace was verified empty above and the suite runs serially, # so everything in it now is ours: the two tables, their stats/counts diff --git a/tests/test_export_format.py b/tests/test_export_format.py new file mode 100644 index 0000000..d41e3a2 --- /dev/null +++ b/tests/test_export_format.py @@ -0,0 +1,108 @@ +# -*- coding: utf-8 -*- +""" +Search-data files carry an optional format marker. + +A new file begins with ``# psycodict-export-format: N``; a file without one is +format 0, the historical layout, so every file psycodict has ever written still +reads. A version this psycodict does not understand is refused before any data +is loaded. This is a property of the *data file*, distinct from the metadata +format of the ``meta_*`` tables. +""" +import pytest + +from psycodict.base import EXPORT_FORMAT, EXPORT_FORMAT_MARKER, export_format_line + + +def read(path): + with open(path) as F: + return F.read() + + +def test_a_new_export_starts_with_the_marker(filled_table, tmp_path): + searchfile = str(tmp_path / "s.txt") + filled_table.copy_to(searchfile) + first = read(searchfile).split("\n")[0] + assert first == "%s %s" % (EXPORT_FORMAT_MARKER, EXPORT_FORMAT) + + +def test_a_marked_file_round_trips(filled_table, table_factory, tmp_path): + searchfile = str(tmp_path / "s.txt") + filled_table.copy_to(searchfile) + assert export_format_line() in read(searchfile) + target = table_factory() + target.copy_from(searchfile) + assert target.count() == filled_table.count() + assert {r["n"] for r in target.search({}, ["n"])} == set(range(200)) + + +def test_a_legacy_unmarked_file_still_loads(filled_table, table_factory, tmp_path): + """ + A format-0 file -- no marker, just names/types/blank/data -- is what every + older export looks like, and must keep loading. + """ + marked = str(tmp_path / "marked.txt") + filled_table.copy_to(marked, include_id=False) + # strip the marker line to reproduce a format-0 file byte-for-byte + body = read(marked).split("\n", 1)[1] + assert read(marked).startswith(EXPORT_FORMAT_MARKER) + legacy = tmp_path / "legacy.txt" + legacy.write_text(body) + assert not read(str(legacy)).startswith(EXPORT_FORMAT_MARKER) + + target = table_factory() + target.copy_from(str(legacy)) + assert target.count() == 200 + + +def test_a_future_format_is_refused_before_loading(filled_table, table_factory, tmp_path): + """ + A file claiming a newer format than this psycodict understands is rejected + with a clear message, and nothing is loaded. + """ + searchfile = filled_table_dump(filled_table, tmp_path) + lines = read(searchfile).split("\n") + lines[0] = "%s %s" % (EXPORT_FORMAT_MARKER, EXPORT_FORMAT + 5) + with open(searchfile, "w") as F: + F.write("\n".join(lines)) + + target = table_factory() + with pytest.raises(ValueError, match="export format"): + target.copy_from(searchfile) + assert target.count() == 0 + + +def test_a_malformed_marker_is_rejected(filled_table, table_factory, tmp_path): + searchfile = filled_table_dump(filled_table, tmp_path) + lines = read(searchfile).split("\n") + lines[0] = "%s banana" % EXPORT_FORMAT_MARKER + with open(searchfile, "w") as F: + F.write("\n".join(lines)) + target = table_factory() + with pytest.raises(ValueError, match="[Mm]arker"): + target.copy_from(searchfile) + + +def test_reload_reads_a_marked_file(filled_table, tmp_path): + searchfile = str(tmp_path / "s.txt") + filled_table.copy_to(searchfile, include_id=False) + filled_table.reload(searchfile) + assert filled_table.count() == 200 + + +def test_create_table_from_header_reads_the_marker(db, filled_table, tmp_path): + """ + adjust_schema builds a table from a file's header; that path reads through + the same header reader, so the marker is handled there too. + """ + folder = tmp_path / "data" + db.copy_to([filled_table.search_table], str(folder)) + searchfile = folder / (filled_table.search_table + ".txt") + assert read(str(searchfile)).startswith(EXPORT_FORMAT_MARKER) + + +# --- helpers --------------------------------------------------------------- + +def filled_table_dump(table, tmp_path): + searchfile = str(tmp_path / "s.txt") + table.copy_to(searchfile, include_id=False) + return searchfile diff --git a/tests/test_id_order_audit.py b/tests/test_id_order_audit.py new file mode 100644 index 0000000..9d2dcdf --- /dev/null +++ b/tests/test_id_order_audit.py @@ -0,0 +1,90 @@ +# -*- coding: utf-8 -*- +""" +The read-only id-order audit and the write-path invariants it protects. + +``scripts/audit_id_order.py`` walks each id_ordered table in sort order and +checks the ids come out increasing. It exists because the flag can drift, so +the audit -- not the flag -- is the ground truth during the one-time +production check. +""" +import importlib.util +import pathlib + +import pytest + +from psycopg.sql import SQL, Identifier + +from conftest import sample_row + + +# Load the script as a module, since scripts/ is not a package. +_AUDIT_PATH = pathlib.Path(__file__).resolve().parent.parent / "scripts" / "audit_id_order.py" +_spec = importlib.util.spec_from_file_location("audit_id_order", _AUDIT_PATH) +audit = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(audit) + + +@pytest.fixture +def ordered_table(table_factory): + table = table_factory(sort=["n"]) + table.insert_many([sample_row(i) for i in range(20)]) + table.resort() + return table + + +def test_audit_reports_ok_for_an_ordered_table(db, ordered_table): + status, detail = audit.audit_table(db, ordered_table.search_table) + assert status == "OK" + assert detail == 20 + + +def test_audit_reads_nothing_but_reports_the_row_count(db, ordered_table): + status, detail = audit.audit_table(db, ordered_table.search_table) + assert status == "OK" and detail == ordered_table.count() + + +def test_audit_catches_a_table_whose_ids_do_not_match_the_sort(db, table_factory): + """ + A table that claims to be ordered but whose ids run against the sort must + be caught, even though its flag says out_of_order = false. + """ + table = table_factory(sort=["n"]) + table.insert_many([sample_row(i) for i in range(10)]) + # Reverse the ids against n, then lie in the metadata: ordered, not stale. + table._execute( + SQL("UPDATE {0} SET id = 100 - n").format(Identifier(table.search_table)) + ) + table._execute( + SQL("UPDATE meta_tables SET id_ordered = true, out_of_order = false WHERE name = %s"), + [table.search_table], + ) + table._db.conn.commit() + + status, detail = audit.audit_table(db, table.search_table) + assert status == "MISMATCH" + + +def test_audit_errors_on_a_non_ordered_table(db, table_factory): + table = table_factory(sort=["n"], id_ordered=False) + table.insert_many([sample_row(i) for i in range(3)]) + status, detail = audit.audit_table(db, table.search_table) + assert status == "ERROR" + assert "id_ordered" in detail + + +def test_audit_errors_on_a_missing_table(db): + status, detail = audit.audit_table(db, "no_such_table_xyz") + assert status == "ERROR" + + +def test_audit_does_not_modify_the_database(db, ordered_table): + before = [rec["id"] for rec in ordered_table.search({}, ["id"], sort=[["id", 1]])] + audit.audit_table(db, ordered_table.search_table) + after = [rec["id"] for rec in ordered_table.search({}, ["id"], sort=[["id", 1]])] + assert before == after + + +def test_audit_on_an_empty_ordered_table(db, table_factory): + table = table_factory(sort=["n"]) + status, detail = audit.audit_table(db, table.search_table) + assert status == "OK" and detail == 0 diff --git a/tests/test_public_api.py b/tests/test_public_api.py new file mode 100644 index 0000000..03fbc7a --- /dev/null +++ b/tests/test_public_api.py @@ -0,0 +1,153 @@ +# -*- coding: utf-8 -*- +""" +The public API is exactly what ``__all__`` says it is. + +Versioning.md defines the supported surface as the names each module exports in +``__all__``. This test freezes those sets, so adding or removing a public name +is a deliberate, reviewed change rather than an accident -- and so that a name a +downstream project relies on cannot quietly leave the promise. + +It also pins the two things that promise depends on: that ``from import *`` +binds exactly ``__all__`` (the one behavioral change declaring it makes), and +that the API reference can actually document the whole of ``__all__`` -- every +module has a page, and every exported callable has a docstring. +""" +import importlib +import inspect +import re +from pathlib import Path + +import pytest + + +_API_DOCS = Path(__file__).resolve().parent.parent / "docs" / "api" + + +# The frozen public surface, module by module. Changing this is changing the +# 1.x API contract: update Versioning.md and coordinate downstream in the same +# breath. +EXPECTED = { + "psycodict": { + "__version__", "SQL", "Identifier", "Placeholder", "Literal", + "Composable", "Composed", "DelayCommit", + }, + "psycodict.base": {"PostgresBase"}, + "psycodict.database": {"PostgresDatabase"}, + "psycodict.table": {"PostgresTable"}, + "psycodict.searchtable": {"PostgresSearchTable"}, + "psycodict.statstable": {"PostgresStatsTable"}, + "psycodict.config": {"Configuration"}, + "psycodict.utils": { + "DelayCommit", "IdentifierWrapper", "LockError", "SearchParsingError", + }, + "psycodict.encoding": {"Json", "Array", "copy_dumps"}, + "psycodict.grants": {"GrantPolicy", "LMFDBGrantPolicy"}, + "psycodict.notifications": {"NotificationListener"}, + "psycodict.dbdiff": {"compare_databases", "format_differences"}, + "psycodict.slowlog": {"parse_slow_log", "slow_query_report", "show_slow_report"}, + "psycodict.validation": {"InvalidColumnTypeError", "InvalidDefinitionError"}, +} + + +@pytest.mark.parametrize("modname", sorted(EXPECTED)) +def test_all_matches_the_frozen_surface(modname): + mod = importlib.import_module(modname) + assert hasattr(mod, "__all__"), "%s has no __all__" % modname + assert set(mod.__all__) == EXPECTED[modname], ( + "%s.__all__ changed; update Versioning.md and this snapshot deliberately" + % modname + ) + + +@pytest.mark.parametrize("modname", sorted(EXPECTED)) +def test_every_exported_name_resolves(modname): + mod = importlib.import_module(modname) + for name in mod.__all__: + assert hasattr(mod, name), "%s exports %s, which does not exist" % (modname, name) + + +@pytest.mark.parametrize("modname", sorted(EXPECTED)) +def test_star_import_gives_exactly_all(modname): + """ + ``from import *`` binds exactly that module's ``__all__``. + + This is the one place where declaring ``__all__`` changes what Python does + rather than only what psycodict promises: before, a wildcard import bound + every non-underscore name in the module. Pinning it per module keeps the + behavioral consequence of the contract visible, and keeps it from looking + like only the package root is governed by ``__all__``. + """ + ns = {} + exec("from %s import *" % modname, ns) + bound = set(ns) - {"__builtins__"} + assert bound == EXPECTED[modname] + + +def test_downstream_imports_still_resolve(): + """ + The names LMFDB and seminars import, including a few private ones and a few + kept-importable-but-unpromised ones, must keep resolving: ``__all__`` governs + ``import *`` and the docs, not explicit imports. + """ + from psycodict import SQL, DelayCommit # noqa: F401 + from psycodict.database import PostgresDatabase # noqa: F401 + from psycodict.searchtable import PostgresSearchTable # noqa: F401 + from psycodict.statstable import PostgresStatsTable # noqa: F401 + from psycodict.base import PostgresBase, number_types # noqa: F401 + from psycodict.config import Configuration # noqa: F401 + from psycodict.encoding import Json, Array, copy_dumps # noqa: F401 + from psycodict.grants import LMFDBGrantPolicy # noqa: F401 + from psycodict.utils import ( # noqa: F401 + IdentifierWrapper, SearchParsingError, + range_formatter, KeyedDefaultDict, + ) + # private names seminars imports for its schema bootstrap + from psycodict.table import _counts_cols, _stats_cols # noqa: F401 + from psycodict.base import ( # noqa: F401 + _meta_indexes_cols, _meta_constraints_cols, _meta_tables_cols, + ) + + +@pytest.mark.parametrize("modname", sorted(EXPECTED)) +def test_module_has_an_api_reference_page(modname): + """ + Every module in the frozen surface has a page in the API reference. + + Versioning.md promises that the ``__all__`` names are exactly the names in + the API reference; that promise is only keepable if a module cannot join + the public surface without a page to be documented on. (Sphinx's ``-W`` + build then catches a page that is not in a toctree.) + """ + if not _API_DOCS.is_dir(): + pytest.skip("built without docs/ (%s)" % _API_DOCS) + documented = set() + for page in _API_DOCS.glob("*.md"): + documented.update( + re.findall(r"^\s*\.\.\s+automodule::\s*(\S+)\s*$", page.read_text(), re.M) + ) + assert modname in documented, ( + "%s is in the public API but no docs/api/*.md documents it; add a page " + "and put it in the toctree in docs/api/index.md" % modname + ) + + +@pytest.mark.parametrize("modname", sorted(EXPECTED)) +def test_every_exported_name_is_documented(modname): + """ + Every exported class and function carries a docstring. + + The API reference runs without ``undoc-members``, so an exported name with + no docstring is silently absent from it -- the reference would quietly stop + covering the whole of ``__all__``. (Exported data such as ``__version__`` + is documented by a ``#:`` comment, which lives in the source rather than on + the object, so only callables are checked here.) + """ + mod = importlib.import_module(modname) + for name in sorted(mod.__all__): + obj = getattr(mod, name) + if not (inspect.isclass(obj) or inspect.isroutine(obj)): + continue + assert obj.__doc__, ( + "%s.%s is exported but has no docstring, so the API reference " + "would not document it" % (modname, name) + ) diff --git a/tests/test_resort.py b/tests/test_resort.py new file mode 100644 index 0000000..aa3c9c0 --- /dev/null +++ b/tests/test_resort.py @@ -0,0 +1,175 @@ +# -*- coding: utf-8 -*- +""" +``resort()`` rebuilds the table so ascending id follows the configured sort. + +Before rc3 it was a disabled no-op (``print("resorting disabled"); return None``) +because the old implementation renumbered every id in place, which stalls +replication and does not lay the rows out on disk in order. It is now a +physical rebuild through reload's machinery, and the row-level writes that used +to accept a ``resort=`` flag reject ``resort=True`` and point at it. +""" +import pytest + +from psycopg.sql import SQL, Identifier + +from conftest import sample_row + + +def ids_in_disk_order(table): + """ + The ids in physical (heap) order -- ``ctid`` follows the on-disk layout. + A resorted table must have these ascending, not just the logical ids. + """ + cur = table._execute( + SQL("SELECT id FROM {0} ORDER BY ctid").format(Identifier(table.search_table)) + ) + return [rec[0] for rec in cur] + + +@pytest.fixture +def shuffled_table(table_factory): + """ + An id_ordered table sorted by ``n`` whose rows were inserted in an order + that does *not* match ``n``, so its ids do not follow the sort. + """ + table = table_factory(sort=["n"]) + order = [3, 0, 4, 1, 2, 7, 5, 6, 9, 8] + table.insert_many([sample_row(i) for i in order]) + table.stats.saving = True + return table + + +def test_resort_puts_ids_in_sort_order(shuffled_table): + table = shuffled_table + # before: id order does not match n order + by_id = [rec["n"] for rec in table.search({}, ["n"], sort=[["id", 1]])] + assert by_id != sorted(by_id) + + assert table.resort() is True + + by_id = [rec["n"] for rec in table.search({}, ["n"], sort=[["id", 1]])] + assert by_id == sorted(by_id) + + +def test_resort_assigns_contiguous_ids_from_one(shuffled_table): + shuffled_table.resort() + ids = sorted(rec["id"] for rec in shuffled_table.search({}, ["id"])) + assert ids == list(range(1, len(ids) + 1)) + + +def test_resort_lays_rows_out_on_disk_in_order(shuffled_table): + """ + The point of id-ordering is sequential disk reads: an in-place renumber + would leave the heap order untouched. A rebuild must not. + """ + shuffled_table.resort() + assert ids_in_disk_order(shuffled_table) == sorted( + ids_in_disk_order(shuffled_table) + ) + + +def test_resort_marks_the_table_ordered(shuffled_table): + shuffled_table._break_order() + assert shuffled_table._out_of_order is True + shuffled_table.resort() + assert shuffled_table._id_ordered is True + assert shuffled_table._out_of_order is False + # and persisted + cur = shuffled_table._execute( + SQL("SELECT id_ordered, out_of_order FROM meta_tables WHERE name = %s"), + [shuffled_table.search_table], + ) + assert cur.fetchone() == (True, False) + + +def test_resort_preserves_the_data(shuffled_table): + before = {rec["n"]: rec["label"] for rec in shuffled_table.search({}, ["n", "label"])} + shuffled_table.resort() + after = {rec["n"]: rec["label"] for rec in shuffled_table.search({}, ["n", "label"])} + assert before == after + assert shuffled_table.count() == len(before) + + +def test_resort_keeps_a_backup(shuffled_table): + shuffled_table.resort() + # reload keeps the previous table as _old1 + assert shuffled_table._table_exists(shuffled_table.search_table + "_old1") + + +def test_resort_rebuilds_indexes(shuffled_table): + shuffled_table.resort() + built = set(shuffled_table._list_built_indexes()) + assert shuffled_table.search_table + "_pkey" in built + + +def test_resort_reports_nothing_to_do_when_already_ordered(shuffled_table): + shuffled_table.resort() + # a second resort with the table already ordered does nothing + assert shuffled_table.resort() is False + + +def test_resort_force_rebuilds_even_when_ordered(shuffled_table): + shuffled_table.resort() + assert shuffled_table.resort(force=True) is True + + +def test_resort_requires_id_ordered(table_factory): + table = table_factory(sort=["n"], id_ordered=False) + table.insert_many([sample_row(i) for i in range(5)]) + with pytest.raises(ValueError, match="id_ordered"): + table.resort() + + +def test_resort_requires_a_sort(table_factory): + table = table_factory(sort=["n"]) + table.set_sort(None) + table.insert_many([sample_row(i) for i in range(5)]) + with pytest.raises(ValueError, match="id_ordered|sort"): + table.resort() + + +def test_resort_handles_a_descending_sort(table_factory): + table = table_factory(sort=[("n", -1)]) + table.insert_many([sample_row(i) for i in [2, 0, 4, 1, 3]]) + table.resort() + by_id = [rec["n"] for rec in table.search({}, ["n"], sort=[["id", 1]])] + assert by_id == sorted(by_id, reverse=True) + + +def test_resort_on_an_empty_table(table_factory): + table = table_factory(sort=["n"]) + table._break_order() + assert table.resort() is True + assert table.count() == 0 + + +def test_resort_does_not_recurse_through_reload(shuffled_table, monkeypatch): + """ + resort() reuses reload, and reload must renumber via the private helper, + not by calling public resort() again. + """ + calls = {"resort": 0} + original = type(shuffled_table).resort + + def counting(self, *a, **k): + calls["resort"] += 1 + return original(self, *a, **k) + + monkeypatch.setattr(type(shuffled_table), "resort", counting) + shuffled_table.resort() + assert calls["resort"] == 1 # the outer call only; reload did not re-enter + + +# --------------------------------------------------------------------------- +# reload still establishes order through the rebuild helper +# --------------------------------------------------------------------------- + +def test_reload_orders_a_no_id_file(table_factory, tmp_path): + table = table_factory(sort=["n"]) + table.insert_many([sample_row(i) for i in [4, 1, 3, 0, 2]]) + dump = tmp_path / "d.txt" + table.copy_to(str(dump), include_id=False) + table.reload(str(dump), resort=True) + by_id = [rec["n"] for rec in table.search({}, ["n"], sort=[["id", 1]])] + assert by_id == sorted(by_id) + assert table._out_of_order is False diff --git a/tests/test_schema_contract.py b/tests/test_schema_contract.py new file mode 100644 index 0000000..15e4b1b --- /dev/null +++ b/tests/test_schema_contract.py @@ -0,0 +1,189 @@ +# -*- coding: utf-8 -*- +""" +A database operates in exactly one schema. + +Every relation psycodict creates goes there, every relation it looks for is +looked for there, and every catalog query is filtered to it -- so a relation of +the same name in another schema can neither stand in for one of these nor be +merged with it. Before this, catalog queries were a mixture of hard-coded +``'public'`` and no filter at all, so two schemas holding ``same_name`` gave +answers assembled from both. +""" +import pytest + +from psycopg.sql import SQL, Identifier + +from psycodict.database import PostgresDatabase +from psycodict.validation import InvalidDefinitionError + + +OTHER = "psycodict_other" + + +@pytest.fixture +def two_schemas(db): + """ + ``.same_name`` in the configured schema and in a second one, with + different columns, plus the metadata tables the second schema needs to be + connectable in its own right. + """ + db._execute(SQL("CREATE SCHEMA IF NOT EXISTS {0}").format(Identifier(OTHER))) + db._execute( + SQL("CREATE TABLE IF NOT EXISTS {0}.{1} (id bigint, only_here text)").format( + Identifier(OTHER), Identifier("same_name") + ) + ) + db._execute( + SQL("CREATE TABLE IF NOT EXISTS {0} (id bigint, label text, n integer)").format( + Identifier("same_name") + ) + ) + yield db + db._execute(SQL("DROP TABLE IF EXISTS {0}").format(Identifier("same_name"))) + db._execute(SQL("DROP SCHEMA IF EXISTS {0} CASCADE").format(Identifier(OTHER))) + + +def test_the_default_schema_is_public(db): + assert db.schema == "public" + + +def test_an_invalid_schema_name_is_refused(config): + for bad in ["", "a" * 64, "with\x00nul"]: + with pytest.raises((InvalidDefinitionError, ValueError)): + PostgresDatabase(config=config, schema=bad) + + +def test_schema_is_not_passed_to_the_driver(db): + """ + psycopg.connect has no ``schema`` parameter; it is psycodict's own setting + and must not end up among the connection overrides. + """ + assert "schema" not in db._connect_kwargs + assert "schema" not in db._connection_options() + + +def test_the_session_search_path_is_the_selected_schema(db): + cur = db._execute(SQL("SELECT current_schema()")) + assert cur.fetchone()[0] == db.schema + + +def test_a_reconnect_keeps_the_schema(db): + db.reset_connection() + cur = db._execute(SQL("SELECT current_schema()")) + assert cur.fetchone()[0] == db.schema + + +# --------------------------------------------------------------------------- +# catalog queries see one schema +# --------------------------------------------------------------------------- + +def test_table_exists_does_not_see_the_other_schema(two_schemas): + db = two_schemas + assert db._table_exists("same_name") + db._execute(SQL("DROP TABLE {0}").format(Identifier("same_name"))) + # still present in the other schema, but not in ours + assert not db._table_exists("same_name") + db._execute( + SQL("CREATE TABLE {0} (id bigint, label text, n integer)").format( + Identifier("same_name") + ) + ) + + +def test_all_tablenames_does_not_merge_schemas(two_schemas): + db = two_schemas + names = db._all_tablenames() + assert names.count("same_name") == 1 + + +def test_schema_relations_is_confined(two_schemas): + db = two_schemas + relations = db._schema_relations() + assert "same_name" in relations + db._execute(SQL("DROP TABLE {0}").format(Identifier("same_name"))) + assert "same_name" not in db._schema_relations() + db._execute( + SQL("CREATE TABLE {0} (id bigint, label text, n integer)").format( + Identifier("same_name") + ) + ) + + +def test_column_discovery_does_not_mix_the_two(two_schemas): + """ + The columns of ``same_name`` differ between the schemas. Reading them + unfiltered used to raise "Type mismatch" or silently union them. + """ + db = two_schemas + cols, col_type, has_id = db._column_types("same_name") + assert sorted(cols) == ["label", "n"] + assert "only_here" not in col_type + + +def test_relation_columns_is_confined(two_schemas): + db = two_schemas + assert db._relation_columns("same_name") == {"id", "label", "n"} + + +def test_index_lookups_are_confined(two_schemas): + db = two_schemas + db._execute( + SQL("CREATE INDEX {0} ON {1}.{2} (id)").format( + Identifier("same_name_idx"), Identifier(OTHER), Identifier("same_name") + ) + ) + # the index exists, but in the other schema + assert not db._index_exists("same_name_idx", "same_name") + assert db._index_exists("same_name_idx", "same_name") is False + assert "same_name_idx" not in db._list_indexes("same_name") + + +def test_constraint_lookups_are_confined(two_schemas): + db = two_schemas + db._execute( + SQL("ALTER TABLE {0}.{1} ADD CONSTRAINT {2} CHECK (id > 0)").format( + Identifier(OTHER), Identifier("same_name"), Identifier("same_name_chk") + ) + ) + assert not db._constraint_exists("same_name_chk", "same_name") + assert "same_name_chk" not in db._list_constraints("same_name") + + +def test_table_sizes_report_one_schema(two_schemas): + db = two_schemas + sizes = db.table_sizes() + assert sizes.get("same_name") is not None + + +# --------------------------------------------------------------------------- +# a database pointed at the other schema sees only that one +# --------------------------------------------------------------------------- + +def test_another_schema_does_not_inherit_the_metadata_tables(two_schemas, config): + """ + The clearest demonstration of the confinement: meta_tables exists, but not + in the other schema, so connecting there without create=True is refused + rather than quietly operating on the configured schema's metadata. + """ + with pytest.raises(ValueError, match="metadata tables"): + PostgresDatabase(config=config, schema=OTHER) + + +def test_a_database_in_another_schema_sees_only_its_own(two_schemas, config): + db = two_schemas + other = PostgresDatabase(config=config, schema=OTHER, create=True) + try: + assert other.schema == OTHER + cur = other._execute(SQL("SELECT current_schema()")) + assert cur.fetchone()[0] == OTHER + + # same relation name, and each database sees its own columns + assert other._relation_columns("same_name") == {"id", "only_here"} + assert db._relation_columns("same_name") == {"id", "label", "n"} + + # each schema has its own metadata, and the configured schema's + # search tables are not visible from the other one + assert "same_name" in other._all_tablenames() + assert set(db.tablenames) - set(other.tablenames) == set(db.tablenames) + finally: + other.conn.close() diff --git a/tests/test_security.py b/tests/test_security.py index 03c0f68..ecb3fa8 100644 --- a/tests/test_security.py +++ b/tests/test_security.py @@ -419,7 +419,12 @@ def test_reload_all_with_adjust_schema_rejects_an_injected_header_type(db, fille metafile.write_text(metafile.read_text().replace(old_name, new_name, 1)) searchfile = folder / (new_name + ".txt") lines = searchfile.read_text().split("\n") - lines[1] = lines[1].replace("text", injected("text", marker), 1) + # locate the types line rather than hard-coding an offset: a file now + # begins with the export-format marker, so it is no longer line 1 + types_idx = next( + i for i, line in enumerate(lines) if "text" in line.split("|") + ) + lines[types_idx] = lines[types_idx].replace("text", injected("text", marker), 1) searchfile.write_text("\n".join(lines)) with pytest.raises(InvalidColumnTypeError): diff --git a/tests/test_stats.py b/tests/test_stats.py index f64c8cb..f31482b 100644 --- a/tests/test_stats.py +++ b/tests/test_stats.py @@ -31,6 +31,11 @@ def saving_table(table_factory): table = table_factory() table.insert_many([sample_row(i) for i in range(200)]) table.stats.saving = True + # insert_many invalidates the statistics, and a cached count may not be + # used while they are invalid. Nothing is cached yet and the total is + # maintained by the insert, so the caches do agree with the data: say so, + # which is the state these tests are about. + table._restore_stats() return table diff --git a/tests/test_stats_duplicates.py b/tests/test_stats_duplicates.py index 26141a9..94d0acf 100644 --- a/tests/test_stats_duplicates.py +++ b/tests/test_stats_duplicates.py @@ -39,6 +39,11 @@ def saving_table(table_factory): table = table_factory() table.insert_many([sample_row(i) for i in range(200)]) table.stats.saving = True + # insert_many invalidates the statistics, and a cached count may not be + # used while they are invalid. Nothing is cached yet and the total is + # maintained by the insert, so the caches do agree with the data: say so, + # which is the state these tests are about. + table._restore_stats() return table @@ -265,6 +270,10 @@ def constrained_table(table_factory): [{"n": i, "a": i % 3, "z": i % 2, "label": "l%d" % i} for i in range(30)] ) table.stats.saving = True + # As in ``saving_table``: nothing is cached yet, so the (empty) caches do + # agree with the data, and these tests are about what add_numstats writes + # rather than about invalidation. + table._restore_stats() return table diff --git a/tests/test_stats_validity.py b/tests/test_stats_validity.py new file mode 100644 index 0000000..a19b9c4 --- /dev/null +++ b/tests/test_stats_validity.py @@ -0,0 +1,325 @@ +# -*- coding: utf-8 -*- +""" +``stats_valid`` means what it says: no cached answer survives it being false. + +Before this, write paths cleared the flag but read paths ignored it, so a +count cached before a ``restat=False`` write kept being served afterwards. +These tests pin the whole contract -- which lookups are gated, which two are +deliberately not, and how the flag is restored. +""" +import pytest + +from psycopg.sql import SQL, Identifier + +from conftest import sample_row + + +@pytest.fixture +def cached_table(table_factory): + """ + A saving table with statistics computed and recorded, and the flag true. + """ + table = table_factory() + table.insert_many([sample_row(i) for i in range(200)]) + table.stats.saving = True + table.stats.refresh_stats() + table._restore_stats() if hasattr(table, '_restore_stats') else None + return table + + +def stats_valid_in_meta(table): + """ + The flag as stored, rather than as cached on the Python object. + """ + cur = table._execute( + SQL("SELECT stats_valid FROM meta_tables WHERE name = %s"), + [table.search_table], + ) + return cur.fetchone()[0] + + +# --------------------------------------------------------------------------- +# every gated lookup reports a miss while the flag is false +# --------------------------------------------------------------------------- + +def test_a_stale_count_is_not_served_after_an_unrestatted_write(cached_table): + """ + The case from the review: cache a nonempty query, change the rows it + matches without refreshing, and the old number kept coming back. + """ + query = {"flag": True} + before = cached_table.stats.count(query, record=True) + assert cached_table.stats.quick_count(query) == before + + cached_table.update(query, {"flag": False}, restat=False) + assert not cached_table._stats_valid + + # the cached row is still physically there ... + cur = cached_table._execute( + SQL("SELECT count FROM {0} WHERE cols = %s").format( + Identifier(cached_table.stats.counts) + ), + [cached_table.stats._split_dict(query)[0]], + ) + assert cur.rowcount + + # ... but it is not an answer any more, and count() computes the truth + assert cached_table.stats.quick_count(query) is None + assert cached_table.stats.count(query) == 0 + + +def test_quick_count_distinct_is_gated(cached_table): + cols = ["flag"] + cached_table.stats._slow_count_distinct(cols, record=True) + assert cached_table.stats.quick_count_distinct(cols) is not None + cached_table._break_stats() + assert cached_table.stats.quick_count_distinct(cols) is None + + +def test_quick_statistic_is_gated(cached_table): + from psycodict.encoding import Json + + assert cached_table.stats.max("n") == 199 + ccols, cvals = Json([]), Json([]) + assert cached_table.stats._quick_statistic("n", ccols, cvals, "max") is not None + cached_table._break_stats() + assert cached_table.stats._quick_statistic("n", ccols, cvals, "max") is None + # and the public method still returns the right answer, the slow way + assert cached_table.stats.max("n") == 199 + + +def test_the_recompute_predicates_are_not_gated(cached_table): + """ + _has_stats and _has_numstats answer "is this recorded", which is what + add_stats and column_counts use to decide whether to compute. Gating them + makes those recompute the whole family on every call and never converge, + because only refresh_stats restores the flag: measured on the LMFDB, that + turned a four-minute downstream suite into one still running after + forty-five. They stay ungated, and the staleness that leaves is recorded + in the test below. + """ + from psycodict.encoding import Json + + cached_table.stats.add_stats(["flag"]) + cached_table.stats.add_numstats("num", ["flag"]) + jcols, empty = Json(["flag"]), Json([]) + assert cached_table.stats._has_stats(jcols, empty, empty, None) + assert cached_table.stats._has_numstats(Json(["num"]), Json(["flag"]), empty, None) + + cached_table._break_stats() + assert cached_table.stats._has_stats(jcols, empty, empty, None) + assert cached_table.stats._has_numstats(Json(["num"]), Json(["flag"]), empty, None) + + +@pytest.mark.xfail( + reason="column_counts can still report a value recorded before an " + "unrefreshed write; closing this needs freshness per statistic " + "rather than one flag per table", + strict=True, +) +def test_column_counts_can_still_be_stale(cached_table): + """ + The gap left by the paragraph above, pinned so that it is a known quantity + rather than a surprise, and so that a future per-statistic freshness change + turns this green. + """ + cached_table.stats.add_stats(["flag"]) + flagged = cached_table.stats.column_counts("flag")[True] + assert flagged > 0 + cached_table.update({"flag": True}, {"flag": False}, restat=False) + assert cached_table.stats.column_counts("flag").get(True, 0) == 0 + + +def test_null_counts_is_not_gated(cached_table): + """ + A miss here costs one full count per search column, not one bounded query, + so null_counts reads what is recorded like the other bulk paths. This is + the call LMFDB's results_complete makes for every query it checks, and + gating it is what took the downstream suite past forty-five minutes. + """ + cached_table.stats.refresh_null_counts() + before = cached_table.stats.null_counts() + cached_table._break_stats() + assert cached_table.stats.null_counts() == before + + +# --------------------------------------------------------------------------- +# what is deliberately not gated +# --------------------------------------------------------------------------- + +def test_the_empty_query_total_survives_invalidation(cached_table): + """ + total is maintained on every write, so it is exact regardless of the flag; + gating it would make count() do a full scan after every insert. + """ + cached_table.insert_many([sample_row(1000)], restat=False) + assert not cached_table._stats_valid + assert cached_table.stats.quick_count({}) == 201 + assert cached_table.count() == 201 + + +def test_status_still_reports_what_the_cache_holds(cached_table): + """ + refresh_stats learns which statistics to recompute from _status, so gating + it would make an invalid table forget what it is supposed to have. + """ + cached_table.stats.add_stats(["flag"]) + before = cached_table.stats._status() + cached_table._break_stats() + assert cached_table.stats._status() == before + + +def test_a_suffixed_table_is_not_gated_by_the_live_flag(cached_table): + """ + A _tmp copy carries its own caches; stats_valid describes the live table. + """ + table = cached_table + assert table.stats.count({"flag": True}, record=True) > 0 + tmp = table.search_table + "_tmp" + table._db._execute( + SQL("CREATE TABLE {0} AS TABLE {1}").format( + Identifier(tmp), Identifier(table.search_table) + ) + ) + table._db._execute( + SQL("CREATE TABLE {0} AS TABLE {1}").format( + Identifier(table.stats.counts + "_tmp"), Identifier(table.stats.counts) + ) + ) + try: + table._break_stats() + assert table.stats.quick_count({"flag": True}) is None + assert table.stats.quick_count({"flag": True}, suffix="_tmp") is not None + finally: + for name in (tmp, table.stats.counts + "_tmp"): + table._db._execute(SQL("DROP TABLE IF EXISTS {0}").format(Identifier(name))) + + +# --------------------------------------------------------------------------- +# restoring the flag +# --------------------------------------------------------------------------- + +def test_refresh_stats_restores_the_flag_and_the_cache(cached_table): + cached_table.update({"flag": True}, {"flag": False}, restat=False) + assert not cached_table._stats_valid + assert stats_valid_in_meta(cached_table) is False + + cached_table.stats.refresh_stats() + assert cached_table._stats_valid + assert stats_valid_in_meta(cached_table) is True + assert cached_table.stats.count({"flag": False}, record=True) == 200 + assert cached_table.stats.quick_count({"flag": False}) == 200 + + +def test_a_failed_refresh_leaves_the_table_invalid(cached_table, monkeypatch): + """ + The flag is set inside the refresh transaction, so a failure part-way + cannot leave a table claiming a cache it does not have. + """ + cached_table._break_stats() + + def boom(*args, **kwargs): + raise RuntimeError("refresh blew up") + + monkeypatch.setattr(cached_table.stats, "refresh_null_counts", boom) + with pytest.raises(RuntimeError): + cached_table.stats.refresh_stats() + cached_table._db.conn.rollback() + + cached_table._refresh() + assert not cached_table._stats_valid + assert stats_valid_in_meta(cached_table) is False + + +def test_refreshing_a_tmp_copy_does_not_validate_the_live_table(cached_table): + table = cached_table + for base in (table.search_table, table.stats.counts, table.stats.stats): + table._db._execute( + SQL("CREATE TABLE {0} AS TABLE {1}").format( + Identifier(base + "_tmp"), Identifier(base) + ) + ) + try: + table._break_stats() + table.stats.refresh_stats(suffix="_tmp") + assert not table._stats_valid + assert stats_valid_in_meta(table) is False + finally: + for base in (table.search_table, table.stats.counts, table.stats.stats): + table._db._execute( + SQL("DROP TABLE IF EXISTS {0}").format(Identifier(base + "_tmp")) + ) + + +# --------------------------------------------------------------------------- +# planner statistics +# --------------------------------------------------------------------------- + +def analyzed(table, name=None): + """ + Whether PostgreSQL holds planner statistics for a relation. + """ + cur = table._execute( + SQL( + "SELECT c.reltuples >= 0 AND s.last_analyze IS NOT NULL " + "OR s.last_analyze IS NOT NULL " + "FROM pg_class c " + "JOIN pg_namespace n ON n.oid = c.relnamespace " + "LEFT JOIN pg_stat_all_tables s ON s.relid = c.oid " + "WHERE n.nspname = current_schema() AND c.relname = %s" + ), + [name or table.search_table], + ) + row = cur.fetchone() + return bool(row and row[0]) + + +def test_a_reload_analyzes_before_the_swap(cached_table, tmp_path): + """ + A bulk-loaded relation has no planner statistics until autovacuum reaches + it, and a rename carries the catalog entry along, so the _tmp copy is + analyzed while it is still _tmp. + """ + searchfile = tmp_path / "data.txt" + cached_table.copy_to(str(searchfile)) + + seen = [] + original = type(cached_table)._analyze + + def record(self, tables, suffix=""): + seen.append((list(tables), suffix)) + return original(self, tables, suffix) + + type(cached_table)._analyze = record + try: + cached_table.reload(str(searchfile)) + finally: + type(cached_table)._analyze = original + + assert seen, "the reload did not analyze anything" + tables, suffix = seen[0] + assert suffix == "_tmp" + assert cached_table.search_table in tables + assert analyzed(cached_table) + + +def test_copy_from_analyzes_the_live_table(cached_table, table_factory, tmp_path): + searchfile = tmp_path / "more.txt" + cached_table.copy_to(str(searchfile)) + target = table_factory() + + seen = [] + original = type(target)._analyze + + def record(self, tables, suffix=""): + seen.append((list(tables), suffix)) + return original(self, tables, suffix) + + type(target)._analyze = record + try: + target.copy_from(str(searchfile), restat=False) + finally: + type(target)._analyze = original + + assert seen == [([target.search_table], "")] + assert target.count() == 200 diff --git a/tests/test_write.py b/tests/test_write.py index 129ffbb..77ba3d0 100644 --- a/tests/test_write.py +++ b/tests/test_write.py @@ -147,12 +147,23 @@ def test_insert_many_bulk_restores_keys_indexes_and_data(empty_table): def test_insert_many_flags_do_not_change_the_result(empty_table): - empty_table.insert_many([sample_row(0)], reindex=True, resort=True, restat=False) + empty_table.insert_many([sample_row(0)], reindex=True, resort=False, restat=False) empty_table.insert_many([sample_row(1)], reindex=False, resort=False, restat=True) assert [rec["n"] for rec in _all_rows(empty_table)] == [0, 1] assert empty_table.search_table + "_pkey" in set(empty_table._list_built_indexes()) +def test_insert_many_rejects_resort(empty_table): + """ + resort=True must not silently trigger a full rebuild from a row-level + write; it raises and points at the explicit resort(). + """ + with pytest.raises(ValueError, match="resort"): + empty_table.insert_many([sample_row(0)], resort=True) + # the write did not happen + assert _num_rows(empty_table) == 0 + + def test_insert_many_updates_ids_but_not_values(empty_table): # The documented contract, both halves: "the dictionaries will be updated # with the ids of the inserted records" -- and with nothing else. The @@ -213,11 +224,18 @@ def test_update_matching_no_rows_changes_nothing(filled_table): def test_update_flags_do_not_change_the_result(filled_table): filled_table.update({"n": 7}, {"label": "quiet"}, restat=False) - filled_table.update({"n": 8}, {"label": "sorted"}, resort=True) + filled_table.update({"n": 8}, {"label": "sorted"}) assert filled_table.lucky({"n": 7}, projection="label") == "quiet" assert filled_table.lucky({"n": 8}, projection="label") == "sorted" +def test_update_rejects_resort(filled_table): + with pytest.raises(ValueError, match="resort"): + filled_table.update({"n": 8}, {"label": "x"}, resort=True) + # the row is unchanged + assert filled_table.lucky({"n": 8}, projection="label") == "l8" + + def test_update_marks_the_table_out_of_order_and_stats_invalid(filled_table): filled_table.update({"n": 7}, {"label": "dirty"}) assert filled_table._out_of_order is True @@ -310,10 +328,11 @@ def test_copy_to_writes_name_and_type_header_lines(filled_table, tmp_path): searchfile = str(tmp_path / "search.txt") filled_table.copy_to(searchfile) lines = _read(searchfile).split("\n") - assert lines[0].split("|")[0] == "id" - assert set(lines[0].split("|")) == {"id"} | set(filled_table.search_cols) - assert lines[1].split("|")[0] == "bigint" - assert lines[2] == "" + assert lines[0] == "# psycodict-export-format: 1" + assert lines[1].split("|")[0] == "id" + assert set(lines[1].split("|")) == {"id"} | set(filled_table.search_cols) + assert lines[2].split("|")[0] == "bigint" + assert lines[3] == "" def test_copy_to_copy_from_roundtrip(filled_table, table_factory, tmp_path): @@ -346,9 +365,10 @@ def test_copy_to_copy_from_respect_a_custom_null_marker(table_factory, tmp_path) searchfile = str(tmp_path / "search.txt") source.copy_to(searchfile, null="NULL") lines = _read(searchfile).split("\n") - header = lines[0].split("|") + assert lines[0] == "# psycodict-export-format: 1" + header = lines[1].split("|") data = {} - for line in lines[3:]: + for line in lines[4:]: if line: rec = dict(zip(header, line.split("|"))) data[rec["n"]] = rec["label"] @@ -393,8 +413,9 @@ def test_copy_to_with_columns_exports_only_those_columns(filled_table, tmp_path) searchfile = str(tmp_path / "search.txt") filled_table.copy_to(searchfile, columns=["n", "label"]) lines = _read(searchfile).split("\n") - assert lines[0] == "id|label|n" - assert lines[3] == "0|l0|0" + assert lines[0] == "# psycodict-export-format: 1" + assert lines[1] == "id|label|n" + assert lines[4] == "0|l0|0" def test_copy_to_rejects_an_unknown_column(filled_table, tmp_path): @@ -405,7 +426,7 @@ def test_copy_to_rejects_an_unknown_column(filled_table, tmp_path): def test_copy_from_a_file_without_ids_assigns_them(filled_table, table_factory, tmp_path): searchfile = str(tmp_path / "search.txt") filled_table.copy_to(searchfile, include_id=False) - assert "id" not in _read(searchfile).split("\n")[0].split("|") + assert "id" not in _read(searchfile).split("\n")[1].split("|") target = table_factory() target.copy_from(searchfile) assert [rec["id"] for rec in _all_rows(target)] == list(range(200)) @@ -559,11 +580,6 @@ def test_reload_resort_rejects_a_malformed_metafile(filled_table, tmp_path): ################################################################## -def test_resort_is_a_disabled_noop(filled_table): - # resort() is deliberately short circuited in table.py: resorting without a - # reload makes replication stall. - assert filled_table.resort() is None - assert [rec["n"] for rec in _all_rows(filled_table)][:5] == [0, 1, 2, 3, 4] def test_rewrite_applies_the_function_to_every_row(filled_table): @@ -632,11 +648,6 @@ def test_drop_column_validates_its_argument(filled_table): filled_table.drop_column("nosuchcol", force=True) -def test_finalize_changes_is_a_noop(filled_table): - assert filled_table.finalize_changes() is None - assert _num_rows(filled_table) == 200 - - ################################################################## # transactions # ################################################################## diff --git a/tests/test_write_invariants.py b/tests/test_write_invariants.py new file mode 100644 index 0000000..b8719ab --- /dev/null +++ b/tests/test_write_invariants.py @@ -0,0 +1,153 @@ +# -*- coding: utf-8 -*- +""" +The meta_tables flags mean what B0 of the rc3 review says they mean, after +every supported write. + +- ``total`` is the exact live row count, independent of stat saving. +- ``out_of_order`` records whether ascending id still matches the sort. +- ``stats_valid`` records whether the cached statistics agree with the data. + +These are checked directly against meta_tables (not just the Python object), so +a write that updates one but forgets the other is caught. +""" +import pytest + +from psycopg.sql import SQL + +from conftest import sample_row + + +def flags(table): + cur = table._execute( + SQL( + "SELECT total, out_of_order, stats_valid FROM meta_tables WHERE name = %s" + ), + [table.search_table], + ) + total, out_of_order, stats_valid = cur.fetchone() + return {"total": total, "out_of_order": out_of_order, "stats_valid": stats_valid} + + +@pytest.fixture +def ordered(table_factory): + """An ordered, stats-valid table of 100 rows sorted by n.""" + table = table_factory(sort=["n"]) + table.insert_many([sample_row(i) for i in range(100)]) + table.stats.saving = True + table.resort() + table.stats.refresh_stats() + assert flags(table)["total"] == 100 + return table + + +# --------------------------------------------------------------------------- +# total is exact after every operation +# --------------------------------------------------------------------------- + +def test_insert_many_keeps_total_exact(ordered): + ordered.insert_many([sample_row(1000), sample_row(1001)], restat=False) + assert flags(ordered)["total"] == 102 + assert ordered.count() == 102 + + +def test_delete_keeps_total_exact(ordered): + ordered.delete({"n": {"$lt": 10}}, restat=False) + assert flags(ordered)["total"] == 90 + assert ordered.count() == 90 + + +def test_upsert_insert_keeps_total_exact(ordered): + ordered.upsert({"label": "brand_new"}, {"n": 5000}) + assert flags(ordered)["total"] == 101 + + +def test_upsert_update_does_not_change_total(ordered): + ordered.upsert({"label": "l5"}, {"n": 5000}) + assert flags(ordered)["total"] == 100 + + +def test_copy_from_keeps_total_exact(ordered, table_factory, tmp_path): + source = table_factory(sort=["n"]) + source.insert_many([sample_row(i) for i in range(200, 205)]) + dump = tmp_path / "d.txt" + source.copy_to(str(dump), include_id=False) + ordered.copy_from(str(dump), restat=False) + assert flags(ordered)["total"] == 105 + + +def test_total_is_exact_without_stat_saving(table_factory): + """ + total is maintained even when the stats object is not saving custom counts. + """ + table = table_factory(sort=["n"]) + assert table.stats.saving is False + table.insert_many([sample_row(i) for i in range(7)]) + assert flags(table)["total"] == 7 + table.delete({"n": 0}) + assert flags(table)["total"] == 6 + + +def test_reload_recounts_total(ordered, tmp_path): + dump = tmp_path / "d.txt" + ordered.copy_to(str(dump), include_id=False) + # dump holds 100 rows; reload replaces the table with them + ordered.reload(str(dump)) + assert flags(ordered)["total"] == 100 + + +# --------------------------------------------------------------------------- +# order-flag transitions, per B0's rules +# --------------------------------------------------------------------------- + +def test_delete_preserves_order(ordered): + ordered.delete({"n": {"$lt": 5}}) + assert flags(ordered)["out_of_order"] is False + + +def test_insert_many_breaks_order(ordered): + ordered.insert_many([sample_row(1000)], restat=False) + assert flags(ordered)["out_of_order"] is True + + +def test_update_of_a_non_sort_column_still_marks_out_of_order(ordered): + """ + update() is conservative: it marks order broken on any update. (B0 notes + this is safe if pessimistic; the important half is that it is never + falsely left ordered.) + """ + ordered.update({"n": 5}, {"label": "changed"}, restat=False) + assert flags(ordered)["out_of_order"] is True + + +def test_resort_restores_order(ordered): + ordered.insert_many([sample_row(1000)], restat=False) + assert flags(ordered)["out_of_order"] is True + ordered.resort() + assert flags(ordered)["out_of_order"] is False + + +def test_inplace_update_of_a_sort_key_breaks_order(ordered, tmp_path): + """ + B0: an in-place update_from_file that changes a sort key must set + out_of_order, since it did not rebuild. + """ + dump = tmp_path / "u.txt" + with open(dump, "w") as F: + F.write("label|n\ntext|integer\n\nl5|9999\n") + ordered.update_from_file(str(dump), inplace=True, restat=False) + assert flags(ordered)["out_of_order"] is True + + +# --------------------------------------------------------------------------- +# stats_valid transitions +# --------------------------------------------------------------------------- + +def test_writes_invalidate_stats(ordered): + ordered.insert_many([sample_row(1000)], restat=False) + assert flags(ordered)["stats_valid"] is False + + +def test_refresh_stats_revalidates(ordered): + ordered.insert_many([sample_row(1000)], restat=False) + ordered.stats.refresh_stats() + assert flags(ordered)["stats_valid"] is True