diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f0d6ae3..21a2bb3 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,57 @@ 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 - <` with `ORDER BY id`. *Migration:* none; run `resort()` on + tables the audit below reports as `MISMATCH`. +- **Every write that does not rebuild the caches clears `stats_valid`.** + `update_from_file`, `rewrite` and `reload` with `restat=False` (and any + replacement that installs cloned, partial or untouched counts and stats + beside changed data) now mark the table invalid, in the same transaction as + the swap. A `metafile` no longer installs a `stats_valid = true` describing + the database it was exported from. Without this a count cached before such a + write was still served afterwards, defeating the enforcement above. The rule + is deliberately conservative in one place: `resort()` leaves the rows + unchanged, so the caches would still be accurate, but on a table without + `saving` nothing rebuilds them and it ends invalid. *Migration:* run + `refresh_stats()` after a `resort()` on such a table. +- **`scripts/audit_id_order.py`**, a read-only check that streams each + `id_ordered` table in sort order and reports whether its ids actually + increase, since the flag can drift. Each table is audited in a read + transaction of its own, so one table's error does not poison the rest of the + run, and `id_ordered` with no configured sort is reported as an error rather + than trivially passing. + +- **psycopg is a plain dependency.** `pip install psycodict` now installs a + working package (pure-Python psycopg using the system `libpq`); `pgbinary` + adds the bundled binary build and `pgc` a locally compiled one. The + `pgsource` extra is removed -- plain install replaces it -- and a `<4` ceiling + keeps an unreviewed future driver major out of a 1.x environment. *Migration:* + replace `psycodict[pgsource]` with plain `psycodict`; `psycodict[pgbinary]` is + unchanged. +- **Search-data export files carry an optional format marker.** A file written + by `copy_to`/`rewrite` now begins with `# psycodict-export-format: 1`; a file + without one is format 0, so every older export still loads, and a reader + refuses a version it does not understand before loading any data. This is the + data-file format, distinct from the `meta_*` metadata format. Versioning.md + states the realistic compatibility promise (newer readers read older files, + not necessarily the reverse) and decouples both format numbers from the + package major version. +- **Generic defaults instead of LMFDB ones.** The default database name is + `postgres` rather than `lmfdb` -- the maintenance database conventionally + created with a PostgreSQL cluster, so the one most likely to exist. (Note + that it is not libpq's own default, which is the connection user name.) + Deployments name their own database in the config or constructor, as LMFDB + already does. The unused `secretsfile` argument to `PostgresDatabase` is + removed (secrets-file resolution lives in `Configuration`). +- **Packaging.** The source distribution is complete -- tests including + `conftest.py`, the maintenance scripts, the guides and metadata files -- and + CI unpacks it and runs its database-free tests, so a missing file fails the + build. The release workflow's actions are pinned to commit SHAs, and the + install smoke tests assert `importlib.metadata.version == __version__` and run + `pip check` for both a plain and a binary-extra install. + ### Release candidates 1.0.0 is published as a sequence of release candidates first. `pip` ignores diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4aa5f8d..da90b64 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -59,8 +59,10 @@ python -m pip install -e ".[pgbinary,test]" python -m pip install ruff ``` -The `pgsource` extra may be used instead of `pgbinary` when testing against a -system installation of libpq. +To test against a system installation of libpq, leave `pgbinary` out and run +`pip install -e ".[test]"` instead: psycopg is a plain dependency, so the base +install already brings the pure-Python driver, which uses whatever libpq the +system provides. (`pgc` compiles against it instead, and needs build tools.) ## PostgreSQL test database diff --git a/DataManagement.md b/DataManagement.md index 0554b22..edfba84 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,10 +64,18 @@ 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. - * **`meta_format`** — a single-row `(version, min_compat)` stamp of the metadata *format*: the layout of the `meta_*` tables, versioned by an integer aligned with psycodict's major version (`META_FORMAT`). Every connection checks it. The same format connects silently; an older but compatible format connects with a warning and operates at that older format (newer features unavailable) so a not-yet-migrated or read-only database — the LMFDB devmirror, say — keeps working; a newer format connects when its stamped `min_compat` admits this psycodict, and is otherwise refused. A database that has meta tables but no `meta_format` is the unstamped 0.x baseline (format 0); one with no meta tables at all is fresh and must be connected to with `PostgresDatabase(create=True)`, which bootstraps all of the above. Migrating to a newer format is deliberate — `db.upgrade_metadata()` or `PostgresDatabase(upgrade=True)`, never a side effect of connecting. See [MetadataFormats.md](MetadataFormats.md) for the full policy (including why a pre-1.0 psycodict must not be pointed at a migrated database). + * **`meta_format`** — a single-row `(version, min_compat)` stamp of the metadata *format*: the layout of the `meta_*` tables, versioned by an integer of its own (`META_FORMAT`), independent of psycodict's major version. Every connection checks it. The same format connects silently; an older but compatible format connects with a warning and operates at that older format (newer features unavailable) so a not-yet-migrated or read-only database — the LMFDB devmirror, say — keeps working; a newer format connects when its stamped `min_compat` admits this psycodict, and is otherwise refused. A database that has meta tables but no `meta_format` is the unstamped 0.x baseline (format 0); one with no meta tables at all is fresh and must be connected to with `PostgresDatabase(create=True)`, which bootstraps all of the above. Migrating to a newer format is deliberate — `db.upgrade_metadata()` or `PostgresDatabase(upgrade=True)`, never a side effect of connecting. See [MetadataFormats.md](MetadataFormats.md) for the full policy (including why a pre-1.0 psycodict must not be pointed at a migrated database). Rows of `meta_indexes` and `meta_constraints` become DDL when an index or constraint is rebuilt, so psycodict validates them both when they are imported @@ -108,11 +116,97 @@ 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. + +The replacement paths — `reload`, `rewrite`, a non-inplace `update_from_file`, +and the staged swap — decide the flag in one place, in the same transaction as +the swap. The table ends up valid only when the counts and stats that go live +were built from the data that goes live with them: either recomputed here +(`restat`, on a table with `saving` on) or supplied whole by the export +(`reload` given **both** `countsfile` and `statsfile`). Anything else — a +`restat=False` write, a partial cache set, companions that were merely cloned +or left in place beside replaced rows — is marked invalid. A `metafile` +carries a `stats_valid` of its own describing the database it was exported +from; it does not vouch for caches the reload did not install, and is +overridden to false with a message when it disagrees. + +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. +`resort=False` says *do not rebuild*; it does not say the sort was left alone. +Whether an update can move the ids is decided from the file — does it carry a +sort column? — independently of the option, so a write that touches one always +records `out_of_order = true`, on the in-place path and on the replacement path +alike. A `rewrite` runs an arbitrary function and dumps every search column, so +one that is not rebuilt always marks the table out of order; there is no way to +tell that it did not move a sort key. A staged write is not allowed to rebuild +inside the staged context, but it carries that fact through the commit, so the +swapped-in table is marked out of order and `resort()` afterwards puts it right. + +`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. Each table is audited in a read transaction of its +own, so one that errors does not affect the tables audited after it. Use it +before trusting `out_of_order = false` on a table whose history includes +non-inplace updates, which could have left the flag stale. + +```bash +python scripts/audit_id_order.py --all +``` ### Locking @@ -131,8 +225,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. @@ -147,6 +242,7 @@ The reader (`_read_header_lines`/`_check_header_lines`) checks that these names Here is a genuine two-row export (`db.demo_circles.copy_to("demo_circles.txt")`) of the table created above: ``` +# psycodict-export-format: 1 id|center|label|props|radius bigint|integer[]|text|jsonb|integer @@ -154,11 +250,11 @@ bigint|integer[]|text|jsonb|integer 1|{3,4}|big|{"note": "3-4-5", "unit": false}|5 ``` -Note the id column first, the `integer[]` written as `{0,0}`, the `jsonb` keys reordered by Postgres, and (had a value contained a `|`) it would be escaped as `\|`. +Note the marker line, the id column first, the `integer[]` written as `{0,0}`, the `jsonb` keys reordered by Postgres, and (had a value contained a `|`) it would be escaped as `\|`. ### `copy_to` and `copy_from` -**`copy_to(searchfile, countsfile=None, statsfile=None, indexesfile=None, constraintsfile=None, metafile=None, columns=None, query=None, include_id=True)`** exports via `COPY ... TO STDOUT`. Only the *search* file gets the three header lines; the counts, stats, meta, index and constraint files are headerless raw rows (their column layout is fixed and known). `query` restricts which rows are exported; `columns` restricts which columns. The counts and stats files are written only if the table has `saving` on. +**`copy_to(searchfile, countsfile=None, statsfile=None, indexesfile=None, constraintsfile=None, metafile=None, columns=None, query=None, include_id=True)`** exports via `COPY ... TO STDOUT`. Only the *search* file gets the marker and the three header lines; the counts, stats, meta, index and constraint files are headerless raw rows (their column layout is fixed and known). `query` restricts which rows are exported; `columns` restricts which columns. The counts and stats files are written only if the table has `saving` on. **`copy_from(searchfile, resort=False, reindex=None, restat=True)`** streams a search file into the **existing** table via `COPY ... FROM STDIN`, *appending* rows. It validates the header, and if the file has no `id` column it assigns ids contiguously starting just after the current maximum (using a temporary sequence). `reindex` defaults to `True` for files with more than 1000 data rows. `total` is incremented and statistics invalidated. diff --git a/MANIFEST.in b/MANIFEST.in new file mode 100644 index 0000000..5a8d1ca --- /dev/null +++ b/MANIFEST.in @@ -0,0 +1,37 @@ +# 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 configuration example the guides tell users to copy to config.ini. It is +# named here rather than left to setuptools' implicit list, which does not +# cover it. (The remaining tracked files at the top level are either included +# by setuptools itself -- LICENSE, README.md, pyproject.toml, MANIFEST.in -- or +# development-only: .gitignore and .readthedocs.yaml.) +include config.ini.example + +# 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/MetadataFormats.md b/MetadataFormats.md index 55222d3..3e7ab0c 100644 --- a/MetadataFormats.md +++ b/MetadataFormats.md @@ -20,9 +20,19 @@ deliberate step rather than a side effect of connecting. | 0 | 0.x | Baseline: `meta_tables`, `meta_indexes`, `meta_constraints` and their `_hist` counterparts, with no format stamp. | | 1 | 1.0 | `meta_indexes` / `meta_indexes_hist` gain a nullable `whereclause` column, holding the predicate of a partial index (NULL for an ordinary index). Compatible with format 0: everything keeps working against a format-0 database except creating partial indexes. | -The format number is aligned with psycodict's major version: format *N* is -introduced by psycodict *N*.0, and plans to change the format should be -scheduled for a major release. +The format number is a protocol revision in its own right, not psycodict's +major version; format 1 arriving with psycodict 1.0 is where the two happened +to start, not a rule that ties them together. Which release a revision may +ship in follows from what it does to `min_compat` (see +[Versioning.md](Versioning.md)): + + * a **compatible, additive** revision — new columns, `min_compat` left low + enough that every supported client still connects — may ship in a minor + release; + * a **breaking** revision — one that raises `min_compat` past a client of the + current major series, or otherwise makes that client unsafe — requires a new + major version, since under semantic versioning that is what a break in the + compatibility promise means. ## The stamp @@ -102,7 +112,7 @@ it never reads them. > mixed-version rollout; once every process has been upgraded to 1.0+ there is > nothing to watch for. (The alternative — leaving a tombstone in the old > `meta_version` table so pre-1.0 clients refuse — was declined in favor of the -> single-stamp design; the numbering aligns with the major version instead.) +> single-stamp design, which pre-1.0 clients simply cannot see.) ## Migrating @@ -206,22 +216,26 @@ Suppose the layout of the meta tables must change. Then: reordering or repurposing an existing column is a *breaking* change; avoid it if at all possible, and see step 4 if not. 2. **Bump `META_FORMAT`** in `psycodict/base.py`, add a line to its History - comment and to the table at the top of this file. Schedule the bump for - the next major release, so the format number and the major version stay - aligned. + comment and to the table at the top of this file. The bump is a protocol + revision, so which release it may ship in is decided in step 4, by what it + does to `min_compat` — not by matching the format number to the major + version. 3. **Register the migration** in `META_MIGRATIONS` in `psycodict/database.py`: a one-line description, the `compatible` flag, the `min_compat` to stamp, and the name of a `PostgresDatabase` method performing the DDL. The method does only the DDL, idempotently (`IF NOT EXISTS` / `IF EXISTS`); stamping is handled by `upgrade_metadata`. -4. **Choose the compatibility flags honestly.** +4. **Choose the compatibility flags honestly** — they decide which release + the revision may ship in. - A purely additive change: `compatible=True`, and `min_compat` stays - what it was (0 today). + what it was (0 today). No supported client is shut out, so this may go + out in a minor release. - A breaking change: `compatible=False` (older-format databases are refused rather than warned) and `min_compat` raised to the new format - (older psycodicts are refused by newer databases). Say so loudly in - the CHANGELOG. + (older psycodicts are refused by newer databases). That shuts out + clients of the current major series, so it requires a new major version. + Say so loudly in the CHANGELOG. 5. **Gate the feature.** Code that reads or writes the new columns must consult `self._db._meta_format` and either degrade (reads: the column is absent, so treat it as its default) or raise with instructions (writes diff --git a/README.md b/README.md index 664eaf9..85126e1 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.x, version 3.2.4 or later (`psycopg>=3.2.4,<4`, 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/Versioning.md b/Versioning.md index 9283cd8..faebbdb 100644 --- a/Versioning.md +++ b/Versioning.md @@ -23,9 +23,14 @@ 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. @@ -44,9 +49,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/psycodict/base.py b/psycodict/base.py index ae61f04..c621210 100644 --- a/psycodict/base.py +++ b/psycodict/base.py @@ -71,9 +71,11 @@ def jsonb_idx(cols, cols_type): # The version of the metadata format described by the constants below: the -# layout of the meta_* tables, versioned by a single integer aligned with -# psycodict's major version (format N is introduced by psycodict N.0). The -# format of a database is stamped into the single-row meta_format table as +# layout of the meta_* tables, versioned by a single integer. That integer is +# a protocol revision of its own, not psycodict's major version -- a compatible +# additive revision may ship in a minor release, while one that raises +# min_compat past a supported client needs a major one. The format of a +# database is stamped into the single-row meta_format table as # (version, min_compat), and every connection checks it: an older but # compatible format connects with a warning and reduced functionality, while # a layout this psycodict cannot safely use is refused. The policy, and the @@ -91,6 +93,56 @@ 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:" + +# The whole of the marker grammar: the prefix, then a nonnegative decimal +# version and nothing else. Only a line matching this is a version; a line +# that merely begins with the prefix may well be a format-0 row of column +# names, since a column may be called anything that is not empty and holds no +# control characters (see validate_column_name). +_EXPORT_FORMAT_MARKER_RE = re.compile( + r"%s[ \t]*([0-9]+)" % re.escape(EXPORT_FORMAT_MARKER) +) + + +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) + + +def export_format_version(line): + """ + The export-format version a marker line declares. + + INPUT: + + - ``line`` -- a line known to be a format marker rather than a row of + column names. + + OUTPUT: + + The nonnegative integer version, or a ``ValueError`` if ``line`` is not + exactly ``# psycodict-export-format: N``. A signed or non-decimal version + is malformed: psycodict has never written one, so a file carrying one was + not written by any psycodict. + """ + marker = line.strip() + match = _EXPORT_FORMAT_MARKER_RE.fullmatch(marker) + if match is None: + raise ValueError("Malformed export-format marker: %r" % (marker,)) + return int(match.group(1)) + _meta_tables_cols = ( "name", @@ -423,14 +475,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 +598,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 +626,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 +654,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 +664,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 +680,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 +701,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 +881,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 +910,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, ) @@ -880,9 +955,10 @@ def _check_header_lines( self, F, table_name, columns_set, sep="|", prohibit_missing=True ): """ - Reads the header lines from a file (row of column names, row of column - types, blank line), checking if these names match the columns set and - the types match the expected types in the table. + Reads the header lines from a file (an optional format marker, the row + of column names, the row of column types and the blank line), checking + if these names match the columns set and the types match the expected + types in the table. Returns a list of column names present in the header. INPUT: @@ -1002,7 +1078,7 @@ def _copy_from(self, filename, table, columns, header, kwds): with DelayCommit(self, silence=True): with open(filename) as F: if header: - # This consumes the first three lines + # This consumes the header, leaving F at the first data row columns = self._check_header_lines(F, table, set(columns), sep=sep) addid = "id" not in columns else: @@ -1300,9 +1376,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,12 +1389,46 @@ def _read_header_lines(self, F, sep="|"): A list of pairs where the first entry is the column and the second the corresponding type - """ - names = [x.strip() for x in F.readline().strip().split(sep)] - types = [x.strip() for x in F.readline().strip().split(sep)] - blank = F.readline() + + 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. + + A format-0 names row may itself begin with the marker prefix, since a + column may be called anything printable, so the prefix alone does not + decide which layout this is. The blank line does: a format-0 header is + ``names / types / blank`` and a marked one ``marker / names / types / + blank``, so if the third physical line is blank the first line was a + row of column names. + """ + first = F.readline() + if first.strip().startswith(EXPORT_FORMAT_MARKER): + second = F.readline() + third = F.readline() + if third.strip(): + # Not a format-0 header, so the first line really is a marker. + version = export_format_version(first) + 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, types_line = second, third + blank = F.readline() + else: + # Format 0, whose one column happens to be named like a marker. + names_line, types_line, blank = first, second, third + else: + # No marker: format 0, and this first line is the column names. + names_line = first + types_line = F.readline() + blank = F.readline() + names = [x.strip() for x in names_line.strip().split(sep)] + types = [x.strip() for x in types_line.strip().split(sep)] if blank.strip(): - raise ValueError("The third line must be blank") + raise ValueError("The header must end with a blank line") if len(names) != len(types): raise ValueError( "The first line specifies %s columns, while the second specifies %s" @@ -1326,6 +1436,27 @@ def _read_header_lines(self, F, sep="|"): ) return list(zip(names, types)) + def _count_data_rows(self, filename, sep="|"): + """ + The number of data rows in a search-data file. + + INPUT: + + - ``filename`` -- the search-data file to count. + - ``sep`` -- a string giving the column separator, since the header is + parsed to find where the data starts. + + OUTPUT: + + The number of lines after the header. Counting physical lines instead + would count the header too, and a marked file's header is one line + longer than an unmarked one's, so the two formats would not agree on + what "1000 rows" means. + """ + with open(filename) as F: + self._read_header_lines(F, sep=sep) + return sum(1 for _ in F) + ################################################################## # Exporting, importing, reloading and reverting meta_* # ################################################################## diff --git a/psycodict/config.py b/psycodict/config.py index 9b9320c..1779999 100644 --- a/psycodict/config.py +++ b/psycodict/config.py @@ -196,7 +196,13 @@ 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 + # maintenance database conventionally created with a PostgreSQL + # cluster, so it is the one most likely to exist. (It is not + # libpq's own default, which is the connection user name.) + # 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..92832c4 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, ) @@ -347,6 +348,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 +393,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 +411,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 +527,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 +590,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 +651,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 +809,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 +940,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 +1048,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 +1352,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 +2426,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..dc1cdf4 100644 --- a/psycodict/dbdiff.py +++ b/psycodict/dbdiff.py @@ -88,9 +88,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/searchtable.py b/psycodict/searchtable.py index 4e55dea..d0a6716 100644 --- a/psycodict/searchtable.py +++ b/psycodict/searchtable.py @@ -25,6 +25,10 @@ # (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 +1646,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 +1689,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 +1702,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 +1732,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 +1755,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/statstable.py b/psycodict/statstable.py index 92d575e..8540f4b 100644 --- a/psycodict/statstable.py +++ b/psycodict/statstable.py @@ -21,7 +21,7 @@ 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 @@ -269,6 +269,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 +332,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 +379,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 +603,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 +800,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 +1356,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 +1720,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 +1919,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..56a6227 100644 --- a/psycodict/table.py +++ b/psycodict/table.py @@ -12,6 +12,7 @@ which is what ``db.`` actually returns. """ import csv +import json import os import tempfile import time @@ -22,7 +23,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, @@ -328,7 +329,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 +1186,42 @@ 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 _set_stats_validity(self, fresh): + """ + Record whether the counts and statistics now live were built from the + data now live. + + The one place the replacement paths -- ``reload``, the non-inplace + ``update_from_file`` and ``rewrite``, and the staged swap -- decide the + flag, so that they cannot drift apart on it. Call it inside the + transaction that does the swap: a failure part way through then leaves + the flag as it was rather than describing companions that never + arrived. + + Unlike :meth:`_break_stats` and :meth:`_restore_stats` this writes + unconditionally rather than trusting the cached flag, because a + metafile import replaces the whole meta_tables row (``stats_valid`` + included) behind this object's back. + """ + fresh = bool(fresh) + updater = SQL("UPDATE meta_tables SET stats_valid = %s WHERE name = %s") + self._execute(updater, [fresh, self.search_table], silent=True) + self._stats_valid = fresh + def _break_order(self): """ This function should be called when the id ordering is invalidated by an insertion or update. @@ -1192,17 +1232,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 +1273,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 +1299,14 @@ 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). + ``func`` may return anything, and the dump covers every search + column, so a rewrite that is not resorted always leaves the table + marked ``out_of_order``; there is no way to tell that it did not + move a sort key. - ``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 +1328,15 @@ 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. + resort = not kwds.get("inplace", False) + elif kwds.get("inplace"): + # update_from_file would reject this too, but only after func has + # been run over every row and the result dumped to disk. + self._forbid_row_level_resort(resort, "rewrite with inplace=True") 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 +1368,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 +1415,7 @@ def update_from_file( resort=None, reindex=None, restat=True, - logging={"operation":"file_update"}, + logging=None, **kwds ): """ @@ -1370,13 +1435,21 @@ def update_from_file( - ``datafile`` -- a file with header lines (unlike ``reload``, does not need to include all columns) and rows containing data to be updated. - ``label_col`` -- a column specifying which row(s) of the table should be updated corresponding to each row of the input file. This will usually be the label for the table, in which case it can be omitted. - ``inplace`` -- whether to do the update in place. If set, the operation cannot be undone with ``reload_revert``. - - ``resort`` -- whether this table should be resorted after updating (default is to resort when the sort columns intersect the updated columns) + - ``resort`` -- whether the replacement table should be built with its ids renumbered in sort order (default is to renumber when the updated columns intersect the sort columns). Only the replacement path can do this, so ``resort=True`` with ``inplace`` raises. ``resort=False`` says not to renumber, not that the sort was left alone: an update that touches a sort column marks the table ``out_of_order`` either way, so that ``resort()`` can put it right later. - ``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 +1457,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)) @@ -1409,8 +1486,16 @@ def update_from_file( for line in F: rowcount += 1 reindex = rowcount > 1000 + # Two separate facts, which one variable used to conflate. Whether + # the input can change the id ordering is a property of the file; + # whether to rebuild the table in sort order is the caller's + # choice. Deriving the first from the second made resort=False + # mean "the sort was not touched" as well as "do not rebuild", so + # a write that reordered the table left out_of_order = false + # behind -- an assertion search code is allowed to act on. + sort_changed = bool(set(columns[1:]).intersection(self._sort_keys)) if resort is None: - resort = bool(set(columns[1:]).intersection(self._sort_keys)) + resort = sort_changed # Create a temp table to hold the data tmp_table = "tmp_update_from_file" @@ -1464,6 +1549,22 @@ 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). Whenever the rows are not renumbered and the file + # could have moved a sort key, the ordering is recorded as + # broken instead -- including on the non-inplace path, where + # the flag set here survives the swap below, which only ever + # clears it via _set_ordered. + if not inplace and self._id_ordered and resort: + self._generate_sorted_ids(suffix=suffix) + ordered = True + else: + ordered = False + if sort_changed: + self._break_order() if reindex and inplace: # also restores constraints self.restore_indexes(columns[1:]) @@ -1472,11 +1573,14 @@ 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: + # The only way this write ends with caches describing its own + # data: recompute them, in place against the live table or on + # the _tmp companions swapped in beside the replacement below. + # With restat off, or on a table that does not save + # statistics, whatever is cached was computed from the rows as + # they were before this write. + refreshed = bool(restat and self.stats.saving) + if refreshed: if not inplace: for table in [self.stats.counts, self.stats.stats]: if not self._table_exists(physical_table_name(table, lifecycle="_tmp")): @@ -1491,7 +1595,7 @@ def drop_tmp(): # and the _tmp tables are left orphaned. reload builds its # swap list the same way. tables = [self.search_table] - if restat and self.stats.saving: + if refreshed: tables += [self.stats.counts, self.stats.stats] if self.stats.counts in tables: # _clone built the _tmp counts table with a bare LIKE, @@ -1503,13 +1607,16 @@ def drop_tmp(): self._swap_in_tmp(tables) if ordered: self._set_ordered() + # In the same transaction as the update or the swap, so that + # the flag and the data it describes move together. + self._set_stats_validity(refreshed) # 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 +1660,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 +1685,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 +1810,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 +1858,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 +1868,184 @@ 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="", sort=None): """ - 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 + intended 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`. 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") + - ``suffix`` -- names the relation to rebuild, ``{search}{suffix}``. + - ``sort`` -- the sort to number the rows by, in the form ``set_sort`` + takes. Defaults to this table's configured sort. A reload that + installs a metafile passes the sort *that file* will install: ids + assigned by the sort being replaced would not describe the sort the + table ends up with, and the reload would then mark it ordered. + + The relation being rebuilt is not necessarily described by this table + object. Under ``reload(adjust_schema=True)`` the ``_tmp`` relation is + built from the data file's header, so it may have gained or lost + columns relative to ``self.search_cols``; its own columns are read + from the catalog rather than assumed, and everything is checked before + the first destructive statement. + """ + target = self.search_table + suffix + sort = self._sort_orig if sort is None else sort + if not sort: + raise ValueError( + "Cannot resort %s: it has no configured sort" % (self.search_table,) + ) + order = self._sort_str(sort) + cols, _, has_id = self._column_types(target) + if not has_id: + raise ValueError( + "Cannot resort %s: it has no id column to renumber " + "(does the relation exist?)" % (target,) + ) + if not cols: + raise ValueError( + "Cannot resort %s: it has no columns besides id, so there is " + "nothing to sort it by" % (target,) + ) + missing = sorted( + {col if isinstance(col, str) else col[0] for col in sort}.difference(cols) ) - 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") + if missing: + # Checked here, before anything is dropped, so that a data file + # and a metafile that disagree about the schema leave the + # relation as it was rather than half rebuilt. + raise ValueError( + "Cannot resort %s by %s: the relation has no column%s %s. The " + "imported schema and the sort being installed do not agree." + % ( + target, + sort, + "" if len(missing) == 1 else "s", + ", ".join(missing), + ) + ) + # 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") + target_cols = SQL(", ").join(Identifier(c) for c in 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), + target_cols, + Literal(seq), + Identifier(target), + order, + ) + ) + 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. + + The rows themselves are untouched, so the cached counts and statistics + would still be accurate; but the replacement path marks the table + invalid unless it rebuilt them, which it does only with ``saving`` on. + On a table that does not save statistics, run ``refresh_stats()`` + afterwards to serve cached counts again. + + INPUT: + + - ``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): """ @@ -1847,8 +2059,9 @@ def _set_ordered(self): def _write_header_lines(self, F, cols, sep="|", include_id=True): """ - Writes the header lines to a file - (row of column names, row of column types, blank line). + Writes the header lines to a file (the format marker, the row of column + names, the row of column types and the blank line that ends the + header). INPUT: @@ -1859,7 +2072,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 +2153,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 +2181,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)) @@ -1971,6 +2209,62 @@ def _swap_in_tmp(self, tables): "to save disc space" ).format(self.search_table, backup_number)) + def _metafile_order_state(self, metafile, sep="|"): + """ + The ordering metadata a metafile will install: ``(id_ordered, + out_of_order, sort)``. + + A reload that carries a metafile replaces this table's sort with the + file's, so the renumbering it does has to follow *that* sort; reading + it here, before anything is rebuilt, is what lets + ``_generate_sorted_ids`` number the rows for the sort the table ends + up with rather than the one it is losing. The file is validated as + strictly as ``_reload_meta`` validates it later. + + INPUT: + + - ``metafile`` -- a file holding this table's meta_tables row + - ``sep`` -- the column separator it was written with + """ + meta_name = "meta_tables" + meta_cols, _, _ = _meta_cols_types_jsonb_idx(meta_name) + # the column which will match search_table + table_name = _meta_table_name(meta_name) + table_name_idx = meta_cols.index(table_name) + with open(metafile, "r") as F: + lines = list(csv.reader(F, delimiter=str(sep))) + if len(lines) != 1: + raise RuntimeError("%s has more than one line" % (metafile,)) + line = lines[0] + if line[table_name_idx] != self.search_table: + raise RuntimeError( + f"column {table_name_idx} (= {line[table_name_idx]}) " + f"in the file {metafile} doesn't match " + f"the search table name {self.search_table}" + ) + flags = {} + for col in ["id_ordered", "out_of_order"]: + idx = meta_cols.index(col) + if line[idx] not in ['t', 'f']: + raise RuntimeError( + f"column {idx} (= {line[idx]}) " + f"in the file {metafile} is different from 't' or 'f'" + ) + flags[col] = line[idx] == 't' + sort_idx = meta_cols.index("sort") + raw = line[sort_idx] + if raw == r"\N": # COPY's null marker: a table with no sort at all + sort = None + else: + try: + sort = json.loads(raw) + except ValueError as err: + raise RuntimeError( + f"column {sort_idx} (= {raw}) in the file {metafile} " + f"is not valid JSON: {err}" + ) + return flags["id_ordered"], flags["out_of_order"], sort + def _check_file_input(self, searchfile, kwds): """ Utility function for validating the inputs to ``rewrite``, ``reload`` and ``copy_from``. @@ -2021,9 +2315,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 +2332,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,6 +2501,28 @@ def reload( % (table, time.time() - now, filename) ) + # 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. + resort_sort = None + if resort: + if metafile: + id_ordered, out_of_order, resort_sort = self._metafile_order_state( + metafile, sep=sep + ) + resort = id_ordered and not out_of_order + # resort_sort, not the sort currently configured: the + # metafile replaces that one, and numbering the rows by + # a sort the table is about to stop having would install + # ids that satisfy nothing while claiming to be ordered. + elif not self._id_ordered: # this table doesn't need to be sorted + resort = False + if resort: + self._generate_sorted_ids(suffix=suffix, sort=resort_sort) + ordered = True + else: + ordered = False + self.restore_pkeys(suffix=suffix) # update the indexes @@ -2217,47 +2535,6 @@ def reload( # Also restores constraints self.restore_indexes(suffix=suffix) - if resort: - if metafile: - # read the metafile - # using code from _reload_meta - meta_name = 'meta_tables' - meta_cols, _, _ = _meta_cols_types_jsonb_idx(meta_name) - # the column which will match search_table - table_name = _meta_table_name(meta_name) - table_name_idx = meta_cols.index(table_name) - with open(metafile, "r") as F: - lines = list(csv.reader(F, delimiter=str(sep))) - if len(lines) != 1: - raise RuntimeError( - "%s has more than one line" % (metafile,) - ) - line = lines[0] - if line[table_name_idx] != self.search_table: - raise RuntimeError( - f"column {table_name_idx} (= {line[table_name_idx]}) " - f"in the file {metafile} doesn't match " - f"the search table name {self.search_table}" - ) - for col in ["id_ordered", "out_of_order"]: - idx = meta_cols.index(col) - if line[idx] not in ['t', 'f']: - raise RuntimeError( - f"column {idx} (= {line[idx]}) " - f"in the file {metafile} is different from 't' or 'f'" - ) - resort = ( - line[meta_cols.index("id_ordered")] == 't' - and line[meta_cols.index("out_of_order")] == 'f' - ) - 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) - else: - ordered = False - # 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]: @@ -2276,10 +2553,21 @@ def reload( # create index on counts table self._create_counts_indexes(suffix=suffix) + # The replacement's caches agree with its data in exactly two + # cases: the export supplied both of them alongside the search + # file, or they were recomputed here from what was loaded. + # Otherwise the counts and stats that go live are the ones + # already there (untouched, since they are not in ``tables``) + # or an empty or reused ``_tmp`` clone -- in neither case a + # description of the rows now in the table. + stats_fresh = (countsfile is not None and statsfile is not None) or ( + self.stats.saving and restat + ) if final_swap: self.reload_final_swap(tables=tables, metafile=metafile, - ordered=ordered) + ordered=ordered, + stats_fresh=stats_fresh) elif metafile is not None and not silence_meta: print("Warning: since the final swap was not requested, we have not updated meta_tables") print("when performing the final swap with reload_final_swap, pass the metafile as an argument to update the meta_tables") @@ -2294,7 +2582,7 @@ def reload( stats=(statsfile is not None), ) - def reload_final_swap(self, tables=None, metafile=None, ordered=False, sep="|"): + def reload_final_swap(self, tables=None, metafile=None, ordered=False, sep="|", stats_fresh=False): """ Renames the ``_tmp`` versions of ``tables`` to the live versions, and updates the corresponding meta_tables row if ``metafile`` is provided. @@ -2304,6 +2592,13 @@ def reload_final_swap(self, tables=None, metafile=None, ordered=False, sep="|"): - ``tables`` -- list of strings (optional), of the tables to be renamed. If None is provided, renames all the tables ending in ``_tmp`` - ``metafile`` -- a string (optional), giving a file containing the meta information for the table. - ``sep`` -- a character (default ``|``) to separate columns + - ``ordered`` -- whether ascending id follows the sort in the tables + being swapped in, so that the live table may be marked ordered. + - ``stats_fresh`` -- whether the counts and statistics being swapped in + were built from the data being swapped in. Defaults to False, the + safe answer for a swap finished by hand: a cached count that + describes the previous data is a wrong answer, whereas an + unnecessary ``false`` only costs a ``refresh_stats()``. """ with DelayCommit(self, silence=True): if tables is None: @@ -2319,8 +2614,23 @@ def reload_final_swap(self, tables=None, metafile=None, ordered=False, sep="|"): self._swap_in_tmp(tables) if metafile is not None: self.reload_meta(metafile, sep=sep) + if not stats_fresh: + # A metafile carries the stats_valid of the database it was + # exported from, which says nothing about whether this + # reload installed matching caches. Importing it as-is + # would let a true written elsewhere vouch for counts that + # were never rebuilt here. + print( + "Marking %s's cached statistics invalid: this reload " + "did not install counts and statistics built from the " + "data it loaded, whatever %s says. Recompute them " + "with db.%s.stats.refresh_stats()." + % (self.search_table, metafile, self.search_table) + ) if ordered: self._set_ordered() + # After reload_meta, which replaces the whole meta_tables row + self._set_stats_validity(stats_fresh) # The swapped-in table's row count bears no relation to the old # total, so recount and store it before the reinitialization # below reads meta_tables. @@ -2676,7 +2986,19 @@ 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(). Dropping the request + # does not hide the fact that the ordering moved: whether the + # file touches a sort column is decided from the file, so the + # write still records it in staged._out_of_order, which the + # commit transfers to the live table. + 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 +3170,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 +3188,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 +3228,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 @@ -2901,11 +3236,12 @@ def copy_from( try: with DelayCommit(self, silence=True): if reindex is None: - rowcount = 0 - with open(searchfile) as F: - for line in F: - rowcount += 1 - reindex = rowcount > 1003 # 3 header lines + # Data rows, not physical lines: the header is three lines + # in a format-0 file and four in a marked one, so counting + # lines would put the documented boundary in a different + # place for each format. + rowcount = self._count_data_rows(searchfile, sep=kwds.get("sep", "|")) + reindex = rowcount > 1000 if reindex: self.drop_indexes(permanent=False) now = time.time() @@ -2914,11 +3250,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/validation.py b/psycodict/validation.py index edbede7..4002de8 100644 --- a/psycodict/validation.py +++ b/psycodict/validation.py @@ -574,6 +574,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..075ab2b --- /dev/null +++ b/scripts/audit_id_order.py @@ -0,0 +1,181 @@ +#!/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. Each table is audited in a read transaction of its own, so one +snapshot is not held open across the whole run and a table that errors does not +carry that error into the tables audited after it. +""" +import argparse +import contextlib +import sys + +import psycopg +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. + + ``table._sort`` must be set; :func:`audit_table` has already checked. The + caller owns the read transaction the named cursor runs in and must close + this generator, so that the ``finally`` below closes the cursor even when + the audit stops early on a mismatch. + """ + db = table._db + clause = SQL("ORDER BY {0}, id").format(table._sort) + 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: + # CLOSE is itself a statement, so it fails when the audit failed and + # left the transaction aborted. The audit's own error is the one + # worth reporting, and the cursor dies with the transaction that + # audit_table is about to end regardless. + with contextlib.suppress(psycopg.Error): + cur.close() + + +def _end_read_transaction(db): + """ + Finish the transaction the audit read in, leaving the connection idle. + + The audit writes nothing, so rolling back is as good as committing, and + unlike a commit it also works after a failed statement: without this, one + table's error would leave the connection in an aborted transaction and + every table audited afterwards would fail with ``InFailedSqlTransaction`` + instead of being audited. Ending the transaction also stops a long + multi-table run from holding a single MVCC snapshot open from the first + table to process exit. + + Never raises: a failure to clean up must not replace the audit result the + caller is about to return. + """ + with contextlib.suppress(psycopg.Error): + db.conn.rollback() + + +def audit_table(db, name): + """ + Return ``("OK", n)``, ``("MISMATCH", detail)`` or ``("ERROR", message)`` + for one table, reading it in sort order without modifying anything. + + Each call runs in a read transaction of its own which is finished on every + exit path, so that tables are audited independently of each other. + """ + if name not in db.tablenames: + return ("ERROR", "no such table") + table = db[name] + if not table._id_ordered: + return ("ERROR", "not id_ordered") + if table._sort is None: + # Falling back to "ORDER BY id" here would report every such table as + # OK by construction. id_ordered with no sort does not describe an + # invariant, so there is nothing this script can confirm. + return ("ERROR", "id_ordered but no sort configured") + prev = None + count = 0 + try: + # closing() so that returning early on a mismatch runs the generator's + # finally and closes the named cursor there and then, rather than + # leaving it open until the generator is garbage collected. + with contextlib.closing(_server_side_rows(table)) as rows: + for rid in rows: + 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: + return ("ERROR", "%s: %s" % (type(err).__name__, err)) + finally: + _end_read_transaction(db) + return ("OK", count) + + +def main(argv=None, db=None): + """ + Run the audit over the tables named on the command line. + + ``db`` is the database to audit; when it is None (the command-line case) + one is opened from the usual configuration. The tests pass their own + fixture database rather than opening a second connection to it. + """ + 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) + + if db is None: + # The package exports no ready-made database object, so build one the + # supported way: PostgresDatabase() picks up $PSYCODICT_CONFIG or + # config.ini exactly as an application using psycodict would. + from psycodict.database import PostgresDatabase + + db = PostgresDatabase() + + 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..a095bf9 --- /dev/null +++ b/tests/test_export_format.py @@ -0,0 +1,331 @@ +# -*- 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, + export_format_version, +) + +# A column may be called anything printable, so this is a legal column name -- +# and it is character for character what a current marker line looks like. +MARKER_NAME = export_format_line() + + +def read(path): + with open(path) as F: + return F.read() + + +def strip_marker(path, dest): + """ + The same file as a format-0 file: byte for byte, minus the marker line. + """ + body = read(str(path)).split("\n", 1)[1] + assert read(str(path)).startswith(EXPORT_FORMAT_MARKER) + dest.write_text(body) + return str(dest) + + +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) + legacy = strip_marker(marked, tmp_path / "legacy.txt") + assert not read(legacy).startswith(EXPORT_FORMAT_MARKER) + + target = table_factory() + target.copy_from(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) + rewrite_first_line(searchfile, "%s %s" % (EXPORT_FORMAT_MARKER, EXPORT_FORMAT + 5)) + + 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) + rewrite_first_line(searchfile, "%s banana" % EXPORT_FORMAT_MARKER) + target = table_factory() + with pytest.raises(ValueError, match="[Mm]arker"): + target.copy_from(searchfile) + assert target.count() == 0 + + +def test_a_negative_version_is_rejected(filled_table, table_factory, tmp_path): + """ + A version is a nonnegative decimal. ``int()`` would happily read ``-1``, + but psycodict has never written such a marker, so a file carrying one was + not written by any psycodict and must not be read as format -1 (which, + being "not newer than the current format", would otherwise be accepted). + """ + searchfile = filled_table_dump(filled_table, tmp_path) + rewrite_first_line(searchfile, "%s -1" % EXPORT_FORMAT_MARKER) + target = table_factory() + with pytest.raises(ValueError, match="[Mm]arker"): + target.copy_from(searchfile) + assert target.count() == 0 + + +def test_export_format_version_reads_only_the_exact_grammar(): + assert export_format_version(export_format_line()) == EXPORT_FORMAT + assert export_format_version("%s 0" % EXPORT_FORMAT_MARKER) == 0 + assert export_format_version(" %s 12\n" % EXPORT_FORMAT_MARKER) == 12 + for bad in [ + "%s -1" % EXPORT_FORMAT_MARKER, + "%s +1" % EXPORT_FORMAT_MARKER, + "%s 1.0" % EXPORT_FORMAT_MARKER, + "%s banana" % EXPORT_FORMAT_MARKER, + "%s" % EXPORT_FORMAT_MARKER, + "%s 1 and more" % EXPORT_FORMAT_MARKER, + "%s 1|another_column" % EXPORT_FORMAT_MARKER, + ]: + with pytest.raises(ValueError, match="[Mm]arker"): + export_format_version(bad) + + +# --- legacy headers that look like a marker -------------------------------- +# +# validate_column_name rules out only an empty name and control characters, so +# a format-0 names row may begin with the marker prefix, or be exactly a valid +# marker. Deciding on the prefix alone would refuse to load those older files. + +MARKER_LIKE_COLUMNS = [(MARKER_NAME, "integer"), ("another_column", "text")] +MARKER_ONLY_COLUMNS = [(MARKER_NAME, "integer")] + + +def marker_like_table(table_factory, columns): + table = table_factory(columns=columns, label_col=None, sort=None) + table.insert_many([{col: i if typ == "integer" else "v%d" % i + for col, typ in columns} for i in range(5)]) + return table + + +def test_a_legacy_names_row_beginning_with_the_prefix_loads(table_factory, tmp_path): + """ + ``# psycodict-export-format: 1|another_column`` is a valid format-0 names + row for a two-column table, not a malformed marker. + """ + source = marker_like_table(table_factory, MARKER_LIKE_COLUMNS) + marked = str(tmp_path / "marked.txt") + source.copy_to(marked, include_id=False) + legacy = strip_marker(marked, tmp_path / "legacy.txt") + assert read(legacy).split("\n")[0] == "%s|another_column" % MARKER_NAME + + target = table_factory(columns=MARKER_LIKE_COLUMNS, label_col=None, sort=None) + target.copy_from(legacy) + assert target.count() == 5 + assert {r[MARKER_NAME] for r in target.search({}, [MARKER_NAME])} == set(range(5)) + + +def test_a_one_column_legacy_names_row_equal_to_a_marker_loads(table_factory, tmp_path): + """ + The exactly ambiguous case: a one-column format-0 file whose names row *is* + a syntactically valid marker. The blank line tells the two apart -- a + format-0 header is names/types/blank, a marked one marker/names/types/blank. + """ + source = marker_like_table(table_factory, MARKER_ONLY_COLUMNS) + marked = str(tmp_path / "marked.txt") + source.copy_to(marked, include_id=False) + legacy = strip_marker(marked, tmp_path / "legacy.txt") + assert read(legacy).split("\n")[0] == MARKER_NAME + + target = table_factory(columns=MARKER_ONLY_COLUMNS, label_col=None, sort=None) + target.copy_from(legacy) + assert target.count() == 5 + assert {r[MARKER_NAME] for r in target.search({}, [MARKER_NAME])} == set(range(5)) + + +def test_a_marker_like_legacy_names_row_loads_with_a_custom_separator( + table_factory, tmp_path +): + """ + The parser takes ``sep=``, and the names row is split with it, so the + disambiguation must hold for a file written with a separator other than + ``|`` too. + """ + source = marker_like_table(table_factory, MARKER_LIKE_COLUMNS) + marked = str(tmp_path / "marked.txt") + source.copy_to(marked, include_id=False, sep=";") + legacy = strip_marker(marked, tmp_path / "legacy.txt") + assert read(legacy).split("\n")[0] == "%s;another_column" % MARKER_NAME + + target = table_factory(columns=MARKER_LIKE_COLUMNS, label_col=None, sort=None) + target.copy_from(legacy, sep=";") + assert target.count() == 5 + + +def test_a_marked_file_round_trips_with_a_custom_separator(table_factory, tmp_path): + source = marker_like_table(table_factory, MARKER_LIKE_COLUMNS) + marked = str(tmp_path / "marked.txt") + source.copy_to(marked, include_id=False, sep=";") + assert read(marked).split("\n")[0] == MARKER_NAME + + target = table_factory(columns=MARKER_LIKE_COLUMNS, label_col=None, sort=None) + target.copy_from(marked, sep=";") + assert target.count() == 5 + + +def test_a_header_without_its_blank_line_says_so(filled_table, table_factory, tmp_path): + """ + The error names what is wrong, not which physical line it was on: the blank + line is the third line of a format-0 header and the fourth of a marked one. + """ + searchfile = filled_table_dump(filled_table, tmp_path) + lines = read(searchfile).split("\n") + del lines[3] # the blank line of a marked header + with open(searchfile, "w") as F: + F.write("\n".join(lines)) + target = table_factory() + with pytest.raises(ValueError, match="blank line"): + 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_adjust_schema_builds_the_table_from_a_marked_header(db, filled_table, tmp_path): + """ + ``adjust_schema=True`` builds the replacement table from the file's header + instead of cloning the current one, and it reads that header through the + same marker-aware reader. Exporting a strict subset of the columns proves + the header really drove the creation: the reloaded table has exactly the + columns the file named. + """ + searchfile = str(tmp_path / "s.txt") + name = filled_table.search_table + filled_table.copy_to(searchfile, columns=["n", "label"], include_id=False) + assert read(searchfile).startswith(EXPORT_FORMAT_MARKER) + + filled_table.reload(searchfile, adjust_schema=True) + + # reload swaps in a new table, so the assertions must be made against the + # object the database hands out now, not the pre-reload one. + reloaded = db[name] + assert set(reloaded.search_cols) == {"n", "label"} + assert reloaded.count() == 200 + assert {r["n"] for r in reloaded.search({}, ["n"])} == set(range(200)) + assert reloaded.lucky({"n": 7}, "label") == "l7" + # The file carries no ids, so the reload renumbers in sort order -- over + # the relation the header built, whose columns are not this table's. + assert [r["id"] for r in reloaded.search({"n": {"$lt": 3}}, ["id"], sort=["n"])] == [1, 2, 3] + + +# --- the automatic reindex decision ---------------------------------------- +# +# copy_from reindexes when more than 1000 *rows* are added. A marked file's +# header is four lines and a format-0 file's is three, so a count of physical +# lines would put that boundary in a different place for each format. + +SIMPLE_COLUMNS = [("n", "integer"), ("label", "text")] + + +def write_search_file(path, nrows, marked=True, sep="|"): + with open(path, "w") as F: + if marked: + F.write(export_format_line() + "\n") + F.write(sep.join(col for col, _ in SIMPLE_COLUMNS) + "\n") + F.write(sep.join(typ for _, typ in SIMPLE_COLUMNS) + "\n\n") + for i in range(nrows): + F.write("%d%sl%d\n" % (i, sep, i)) + return str(path) + + +@pytest.mark.parametrize("marked", [True, False], ids=["marked", "legacy"]) +@pytest.mark.parametrize("nrows", [0, 1, 1000, 1001]) +def test_count_data_rows_ignores_the_header(empty_table, tmp_path, marked, nrows): + searchfile = write_search_file(tmp_path / "s.txt", nrows, marked=marked) + assert empty_table._count_data_rows(searchfile) == nrows + + +def test_count_data_rows_honors_a_custom_separator(empty_table, tmp_path): + searchfile = write_search_file(tmp_path / "s.txt", 3, sep=";") + assert empty_table._count_data_rows(searchfile, sep=";") == 3 + + +@pytest.mark.parametrize("marked", [True, False], ids=["marked", "legacy"]) +@pytest.mark.parametrize( + "nrows,expected", [(1000, False), (1001, True)], ids=["at-the-limit", "over-it"] +) +def test_copy_from_reindexes_above_1000_rows( + table_factory, tmp_path, monkeypatch, marked, nrows, expected +): + """ + Whether the indexes were dropped is asked of ``drop_indexes`` itself: both + branches leave the same indexes in place afterward, so the final state of + the database cannot tell them apart. + """ + target = table_factory(columns=SIMPLE_COLUMNS) + searchfile = write_search_file(tmp_path / "s.txt", nrows, marked=marked) + + dropped = [] + original = target.drop_indexes + + def spy(*args, **kwds): + dropped.append((args, kwds)) + return original(*args, **kwds) + + monkeypatch.setattr(target, "drop_indexes", spy) + target.copy_from(searchfile) + + assert bool(dropped) is expected + assert target.count() == nrows + + +# --- helpers --------------------------------------------------------------- + +def filled_table_dump(table, tmp_path): + searchfile = str(tmp_path / "s.txt") + table.copy_to(searchfile, include_id=False) + return searchfile + + +def rewrite_first_line(path, line): + lines = read(path).split("\n") + lines[0] = line + with open(path, "w") as F: + F.write("\n".join(lines)) diff --git a/tests/test_id_order_audit.py b/tests/test_id_order_audit.py new file mode 100644 index 0000000..ea65d30 --- /dev/null +++ b/tests/test_id_order_audit.py @@ -0,0 +1,254 @@ +# -*- 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 + +import psycopg +from psycopg.sql import SQL, Identifier + +from conftest import sample_row + + +def _idle(db): + """ + Whether the connection is between transactions rather than inside one. + """ + return db.conn.info.transaction_status == psycopg.pq.TransactionStatus.IDLE + + +# 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 + + +def test_audit_errors_when_id_ordered_but_no_sort(db, table_factory): + """ + ``id_ordered`` with no sort configured cannot be audited: ordering by id + itself would call every such table OK by construction, which asserts + nothing. It is a broken metadata state, so say so. + """ + table = table_factory(sort=None, id_ordered=True) + table.insert_many([sample_row(i) for i in range(5)]) + assert table._id_ordered and table._sort is None + + status, detail = audit.audit_table(db, table.search_table) + assert status == "ERROR" + assert "sort" in detail + + +def _break_the_sort_under_the_metadata(table): + """ + Drop the column a table is sorted by, behind psycodict's back, leaving + meta_tables still naming it in the sort. + + That is the shape of failure the audit meets in the wild -- metadata that + no longer describes the database -- and it makes the generated ``SELECT + ... ORDER BY`` fail on the server, aborting the audit's transaction. + """ + table._execute( + SQL("ALTER TABLE {0} DROP COLUMN {1}").format( + Identifier(table.search_table), Identifier("n") + ) + ) + table._db.conn.commit() + + +def test_an_audit_error_does_not_poison_the_tables_audited_after_it(db, table_factory): + """ + A failed audit must end its own transaction. Otherwise the connection + stays in an aborted transaction and every later table reports + InFailedSqlTransaction instead of being audited -- so a single broken + table would turn a whole ``--all`` run into a wall of false errors. + """ + broken = table_factory(sort=["n"]) + broken.insert_many([sample_row(i) for i in range(5)]) + healthy = table_factory(sort=["n"]) + healthy.insert_many([sample_row(i) for i in range(5)]) + healthy.resort() + _break_the_sort_under_the_metadata(broken) + + status, detail = audit.audit_table(db, broken.search_table) + assert status == "ERROR" + assert "InFailedSqlTransaction" not in detail + + status, detail = audit.audit_table(db, healthy.search_table) + assert (status, detail) == ("OK", 5) + + +def test_the_connection_is_idle_after_every_kind_of_result(db, table_factory, ordered_table): + """ + Each table is audited in a bounded read transaction, whatever the result, + so the next table starts from a fresh snapshot rather than inheriting a + transaction opened by the previous one. + """ + assert audit.audit_table(db, ordered_table.search_table)[0] == "OK" + assert _idle(db) + + mismatched = table_factory(sort=["n"]) + mismatched.insert_many([sample_row(i) for i in range(10)]) + mismatched._execute( + SQL("UPDATE {0} SET id = 100 - n").format(Identifier(mismatched.search_table)) + ) + mismatched._db.conn.commit() + assert audit.audit_table(db, mismatched.search_table)[0] == "MISMATCH" + assert _idle(db) + + broken = table_factory(sort=["n"]) + broken.insert_many([sample_row(i) for i in range(3)]) + _break_the_sort_under_the_metadata(broken) + assert audit.audit_table(db, broken.search_table)[0] == "ERROR" + assert _idle(db) + + +def test_audit_streams_through_a_server_side_cursor_and_closes_it(db, ordered_table, monkeypatch): + """ + The audit reads whole tables, so it must keep the sort and the result set + on the server: a client-side cursor would pull every row of a production + table into memory. The cursor must also be closed once the audit is done + with it, including when it stops early. + """ + handed_out = [] + real_cursor = db._cursor + + def spy(buffered=False): + cur = real_cursor(buffered=buffered) + handed_out.append((buffered, cur)) + return cur + + monkeypatch.setattr(db, "_cursor", spy) + + assert audit.audit_table(db, ordered_table.search_table)[0] == "OK" + assert handed_out, "the audit did not open a cursor" + assert all(buffered for buffered, _ in handed_out) + # A named cursor is what makes it server-side; see _cursor(buffered=True). + assert all(cur.name for _, cur in handed_out) + assert all(cur.closed for _, cur in handed_out) + + +def test_mismatch_closes_the_cursor_before_returning(db, table_factory, monkeypatch): + """ + The mismatch return leaves the streaming generator part way through, so + the cursor is only closed if the audit closes it deliberately. + """ + table = table_factory(sort=["n"]) + table.insert_many([sample_row(i) for i in range(10)]) + table._execute( + SQL("UPDATE {0} SET id = 100 - n").format(Identifier(table.search_table)) + ) + table._db.conn.commit() + + handed_out = [] + real_cursor = db._cursor + + def spy(buffered=False): + cur = real_cursor(buffered=buffered) + handed_out.append(cur) + return cur + + monkeypatch.setattr(db, "_cursor", spy) + + assert audit.audit_table(db, table.search_table)[0] == "MISMATCH" + assert handed_out and all(cur.closed for cur in handed_out) + + +def test_main_audits_the_database_it_is_given(db, ordered_table, capsys): + """ + The documented command line has to actually run. ``main`` takes the + database as an argument so the test can hand it the fixture connection; + without one it builds a PostgresDatabase from the usual configuration. + """ + rc = audit.main([ordered_table.search_table], db=db) + out = capsys.readouterr().out + assert rc == 0 + assert "OK" in out + assert ordered_table.search_table in out + + +def test_main_reports_a_mismatch_with_a_nonzero_status(db, table_factory, capsys): + table = table_factory(sort=["n"]) + table.insert_many([sample_row(i) for i in range(10)]) + table._execute( + SQL("UPDATE {0} SET id = 100 - n").format(Identifier(table.search_table)) + ) + table._db.conn.commit() + + rc = audit.main([table.search_table], db=db) + assert rc == 1 + assert "MISMATCH" in capsys.readouterr().out diff --git a/tests/test_resort.py b/tests/test_resort.py new file mode 100644 index 0000000..7e59e8c --- /dev/null +++ b/tests/test_resort.py @@ -0,0 +1,381 @@ +# -*- 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 + + +# --------------------------------------------------------------------------- +# the rebuild follows the relation it is actually rebuilding +# +# Under reload(adjust_schema=True) the _tmp relation is built from the data +# file's header, so it need not have the columns of the live table object. +# --------------------------------------------------------------------------- + +def _reload_file(path, header, types, rows, sep="|"): + """ + A reload search file: names, types, a blank line, then the rows. + """ + with open(path, "w") as F: + F.write(sep.join(header) + "\n") + F.write(sep.join(types) + "\n\n") + for row in rows: + F.write(sep.join(row) + "\n") + return str(path) + + +ROW_ORDER = [3, 1, 4, 0, 2] + + +@pytest.fixture +def two_col_table(table_factory): + """ + A small id_ordered table sorted by ``n``, with rows loaded out of order. + """ + table = table_factory(columns=[("n", "integer"), ("label", "text")], sort=["n"]) + table.insert_many([{"n": i, "label": "l%d" % i} for i in ROW_ORDER]) + return table + + +@pytest.fixture +def three_col_table(table_factory): + """ + The same, plus an ``extra`` column running against ``n``. + """ + table = table_factory( + columns=[("n", "integer"), ("label", "text"), ("extra", "text")], sort=["n"] + ) + table.insert_many( + [{"n": i, "label": "l%d" % i, "extra": "e%d" % (4 - i)} for i in ROW_ORDER] + ) + return table + + +def test_adjusted_schema_reload_preserves_an_added_column(db, two_col_table, tmp_path): + """ + A column the file adds exists in the _tmp relation but not in the live + table object. Copying the rows by the object's columns dropped it from + the INSERT, so every value loaded from the file came back NULL. + """ + name = two_col_table.search_table + path = _reload_file( + tmp_path / "d.txt", + ["n", "label", "extra"], + ["integer", "text", "text"], + [[str(i), "l%d" % i, "e%d" % i] for i in ROW_ORDER], + ) + two_col_table.reload(path, adjust_schema=True, resort=True) + + table = db[name] + rows = list(table.search({}, projection=["n", "label", "extra"], sort=[["id", 1]])) + assert [rec["n"] for rec in rows] == [0, 1, 2, 3, 4] + assert [rec["extra"] for rec in rows] == ["e0", "e1", "e2", "e3", "e4"] + + +def test_adjusted_schema_reload_can_drop_a_column(db, three_col_table, tmp_path): + """ + And the other direction: a column the file leaves out is gone from the + _tmp relation, so naming it in the copy failed with an undefined column. + """ + name = three_col_table.search_table + path = _reload_file( + tmp_path / "d.txt", + ["n", "label"], + ["integer", "text"], + [[str(i), "l%d" % i] for i in ROW_ORDER], + ) + three_col_table.reload(path, adjust_schema=True, resort=True) + + table = db[name] + assert "extra" not in table.search_cols + rows = list(table.search({}, projection=["n", "label"], sort=[["id", 1]])) + assert [rec["n"] for rec in rows] == [0, 1, 2, 3, 4] + assert [rec["label"] for rec in rows] == ["l0", "l1", "l2", "l3", "l4"] + + +def _metafile_with_sort(table, tmp_path, sort_json, name="meta.txt"): + """ + This table's metafile with its ``sort`` column replaced, still claiming + to be ordered. + """ + from psycodict.base import _meta_tables_cols + + path = tmp_path / name + table.copy_to_meta(str(path)) + with open(path) as F: + fields = F.read().rstrip("\n").split("|") + fields[_meta_tables_cols.index("sort")] = sort_json + fields[_meta_tables_cols.index("id_ordered")] = "t" + fields[_meta_tables_cols.index("out_of_order")] = "f" + with open(path, "w") as F: + F.write("|".join(fields) + "\n") + return str(path) + + +def test_a_metafile_reload_numbers_by_the_sort_it_installs(db, two_col_table, tmp_path): + """ + A reload carrying a metafile replaces the sort. Numbering the rows by the + sort being replaced and then installing the new one asserts an ordering the + ids do not have -- the metadata would claim ORDER BY id is ORDER BY extra + while the ids in fact followed n. + """ + name = two_col_table.search_table + path = _reload_file( + tmp_path / "d.txt", + ["n", "label", "extra"], + ["integer", "text", "text"], + # extra runs against n, so ordering by one is visibly not the other + [[str(i), "l%d" % i, "e%d" % (4 - i)] for i in ROW_ORDER], + ) + metafile = _metafile_with_sort(two_col_table, tmp_path, '["extra"]') + two_col_table.reload(path, metafile=metafile, adjust_schema=True, resort=True) + + table = db[name] + assert table._sort_orig == ["extra"] + rows = list(table.search({}, projection=["n", "extra"], sort=[["id", 1]])) + # the added column survived the rebuild ... + assert [rec["extra"] for rec in rows] == ["e0", "e1", "e2", "e3", "e4"] + # ... and the ids follow the new sort, not the old one + assert [rec["n"] for rec in rows] == [4, 3, 2, 1, 0] + assert table._out_of_order is False + + from test_id_order_audit import audit + + assert audit.audit_table(db, name)[0] == "OK" + + +def test_a_metafile_reload_refuses_a_sort_the_imported_schema_lacks(db, three_col_table, tmp_path): + """ + If the file and the metafile disagree -- the sort names a column the + imported schema does not have -- say so before anything is dropped, rather + than failing part way through the rebuild. + """ + name = three_col_table.search_table + before = list(three_col_table.search({}, projection=["n", "extra"], sort=[["n", 1]])) + path = _reload_file( + tmp_path / "d.txt", + ["n", "label"], + ["integer", "text"], + [[str(i), "l%d" % i] for i in ROW_ORDER], + ) + metafile = _metafile_with_sort(three_col_table, tmp_path, '["extra"]') + with pytest.raises(ValueError, match="extra"): + three_col_table.reload(path, metafile=metafile, adjust_schema=True, resort=True) + + # The live table is untouched, and no half-built relation is left behind. + table = db[name] + assert list(table.search({}, projection=["n", "extra"], sort=[["n", 1]])) == before + assert not table._table_exists(name + "_tmp") + + +def test_resort_refuses_a_relation_with_no_columns_besides_id(db, table_factory): + """ + An id-only table has nothing to sort by. Building the copy from an empty + column list would emit ``INSERT INTO t (id, ) SELECT ...``; refuse it with + an explanation instead. + """ + table = table_factory(columns=[("n", "integer")], label_col=None, sort=None) + table.drop_column("n", force=True) + assert table.search_cols == [] + + with pytest.raises(ValueError, match="no columns besides id"): + table._generate_sorted_ids(sort=["n"]) + # and through the public entry point it never gets that far + with pytest.raises(ValueError, match="id_ordered"): + table.resort() + + +def test_a_metafile_sort_may_be_a_direction_pair(db, two_col_table, tmp_path): + """ + The sort arrives from the metafile as JSON, so a ``(column, -1)`` pair + comes back as a two-element list rather than a tuple. + """ + name = two_col_table.search_table + path = _reload_file( + tmp_path / "d.txt", + ["n", "label"], + ["integer", "text"], + [[str(i), "l%d" % i] for i in ROW_ORDER], + ) + metafile = _metafile_with_sort(two_col_table, tmp_path, '[["n", -1]]') + two_col_table.reload(path, metafile=metafile, resort=True) + + table = db[name] + assert table._sort_orig == [["n", -1]] + assert [rec["n"] for rec in table.search({}, projection=["n"], sort=[["id", 1]])] == [ + 4, 3, 2, 1, 0 + ] + + from test_id_order_audit import audit + + assert audit.audit_table(db, name)[0] == "OK" 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_staged_writes.py b/tests/test_staged_writes.py index 8cd6d22..bc4c325 100644 --- a/tests/test_staged_writes.py +++ b/tests/test_staged_writes.py @@ -582,3 +582,113 @@ def att(prop): if version >= 140000: assert att("attcompression") == "p" assert db[name].lucky({"n": 0}, projection="label") == "stored" + + +################################################################## +# order bookkeeping across the swap # +################################################################## + + +def _flags(table): + """ + ``out_of_order`` as meta_tables records it for the live table. + """ + cur = table._execute( + SQL("SELECT out_of_order FROM meta_tables WHERE name = %s"), [table.search_table] + ) + return cur.fetchone()[0] + + +@pytest.fixture +def staged_ordered(table_factory): + """ + An ordered table of 20 rows sorted by n, ready to be staged. + """ + table = table_factory(sort=["n"]) + table.insert_many([sample_row(i) for i in range(20)]) + table.resort() + assert _flags(table) is False + return table + + +def test_staged_rewrite_of_a_sort_key_lands_out_of_order(db, staged_ordered): + """ + A staged write is deliberately not allowed to rebuild in sort order, so + whatever it did to the sort keys has to reach the live table as + out_of_order. The staged wrapper drops the rewrite's resort request; when + that request was also the only record that a sort column had moved, the + replacement went live claiming ORDER BY id still matched the sort. + """ + name = staged_ordered.search_table + + def reverse_n(rec): + rec["n"] = 1000 - rec["n"] + return rec + + with staged_ordered.staged() as staged: + staged.rewrite(reverse_n) + + live = db[name] + assert live._out_of_order is True + assert _flags(live) is True + # And the claim is true: ids no longer follow the sort. + ns = list(live.search({}, projection="n", sort=[["id", 1]])) + assert ns != sorted(ns) + + +def test_staged_rewrite_of_a_sort_key_is_seen_by_the_audit(db, staged_ordered): + """ + The audit is the ground truth the flag is checked against: it must report + the same broken ordering, and report OK again once resort() has rebuilt. + """ + from test_id_order_audit import audit + + name = staged_ordered.search_table + + def reverse_n(rec): + rec["n"] = 1000 - rec["n"] + return rec + + with staged_ordered.staged() as staged: + staged.rewrite(reverse_n) + + assert audit.audit_table(db, name)[0] == "MISMATCH" + db[name].resort() + assert audit.audit_table(db, name)[0] == "OK" + assert _flags(db[name]) is False + + +def test_staged_update_from_file_records_the_broken_order(db, staged_ordered, tmp_path): + """ + An explicit resort=True on a staged update still does not rebuild inside + the staged context, but it must not lose the fact that the file moves a + sort column either. + """ + name = staged_ordered.search_table + dump = tmp_path / "u.txt" + with open(dump, "w") as F: + F.write("label|n\ntext|integer\n\nl5|9999\n") + + with staged_ordered.staged() as staged: + staged.update_from_file(str(dump), resort=True) + # The staged copy was edited in place, not rebuilt: no renumbering. + assert staged._out_of_order is True + + assert _flags(db[name]) is True + assert db[name].lucky({"label": "l5"}, projection="n") == 9999 + + +def test_staged_write_leaves_untouched_sort_columns_alone(db, staged_ordered, tmp_path): + """ + Staging does not break the order flag by itself; only a write that could + have moved a sort key does. + """ + name = staged_ordered.search_table + dump = tmp_path / "u.txt" + with open(dump, "w") as F: + F.write("label|data\ntext|jsonb\n\nl5|{\"a\": 1}\n") + + with staged_ordered.staged() as staged: + staged.update_from_file(str(dump)) + + assert _flags(db[name]) is False 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..ae9cc42 --- /dev/null +++ b/tests/test_stats_validity.py @@ -0,0 +1,550 @@ +# -*- 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 + + +# --------------------------------------------------------------------------- +# no changed-data path keeps a cached answer by forgetting the flag +# +# The whole gate above is worth nothing if a write changes rows, does not +# refresh the caches, and leaves stats_valid true: the cached rows are then +# still served as answers. These check the flag, the cache and the truth. +# --------------------------------------------------------------------------- + +QUERY = {"flag": True} + + +@pytest.fixture +def counted(cached_table): + """ + ``cached_table`` with a nonempty-query count actually recorded and served. + """ + before = cached_table.stats.count(QUERY, record=True) + assert before == 67 + assert cached_table.stats.quick_count(QUERY) == before + assert stats_valid_in_meta(cached_table) is True + return cached_table + + +def assert_invalidated(table, expected): + """ + The flag is false, no cached answer is served, and the truth is available. + """ + assert table._stats_valid is False + assert stats_valid_in_meta(table) is False + assert table.stats.quick_count(QUERY) is None + assert table.count(QUERY) == expected + + +def _flag_flip_file(table, tmp_path, name="u.txt"): + """ + An update file clearing ``flag`` on the rows the cached query counts. + """ + labels = [ + rec["label"] + for rec in table.search(QUERY, projection=["label"]) + ] + path = tmp_path / name + with open(path, "w") as F: + F.write("label|flag\ntext|boolean\n\n") + for label in labels: + F.write("%s|f\n" % label) + return str(path) + + +def test_inplace_update_from_file_without_restat_invalidates(counted, tmp_path): + counted.update_from_file( + _flag_flip_file(counted, tmp_path), inplace=True, restat=False + ) + assert_invalidated(counted, 0) + + +def test_replacement_update_from_file_without_restat_invalidates(db, counted, tmp_path): + name = counted.search_table + counted.update_from_file(_flag_flip_file(counted, tmp_path), restat=False) + assert_invalidated(db[name], 0) + + +def test_rewrite_without_restat_invalidates(db, counted): + name = counted.search_table + + def clear_flag(rec): + rec["flag"] = False + return rec + + counted.rewrite(clear_flag, restat=False) + assert_invalidated(db[name], 0) + + +def test_inplace_rewrite_without_restat_invalidates(counted): + def clear_flag(rec): + rec["flag"] = False + return rec + + counted.rewrite(clear_flag, inplace=True, restat=False) + assert_invalidated(counted, 0) + + +def test_reload_without_matching_caches_invalidates(db, counted, tmp_path): + """ + A reload with no counts/stats files swaps changed search data in beside + companions it merely cloned; with restat off nothing rebuilds them. + """ + name = counted.search_table + searchfile = tmp_path / "data.txt" + counted.copy_to(str(searchfile), include_id=False) + # a file whose rows differ from the cached ones + text = open(searchfile).read().replace("|t|", "|f|") + with open(searchfile, "w") as F: + F.write(text) + + counted.reload(str(searchfile), restat=False) + assert_invalidated(db[name], 0) + + +def test_a_metafile_cannot_vouch_for_caches_the_reload_did_not_build(db, counted, tmp_path): + """ + meta_tables is replaced wholesale by the metafile, stats_valid included. + That flag describes the database the file came from; it must not be + installed over caches this reload never rebuilt. + """ + name = counted.search_table + searchfile = tmp_path / "data.txt" + metafile = tmp_path / "meta.txt" + counted.copy_to(str(searchfile), include_id=False, metafile=str(metafile)) + assert "|t|" in open(metafile).read() # the file asserts stats_valid + + text = open(searchfile).read().replace("|t|", "|f|") + with open(searchfile, "w") as F: + F.write(text) + counted.reload(str(searchfile), metafile=str(metafile), restat=False) + + assert_invalidated(db[name], 0) + + +def test_a_reload_carrying_its_own_caches_stays_valid(db, counted, tmp_path): + """ + The other half of the rule: an export that supplies counts and stats + alongside the search file installed caches built for exactly that data, + so the table is entitled to go on serving them. + """ + name = counted.search_table + searchfile = tmp_path / "data.txt" + countsfile = tmp_path / "counts.txt" + statsfile = tmp_path / "stats.txt" + counted.copy_to(str(searchfile), countsfile=str(countsfile), statsfile=str(statsfile)) + + counted.reload(str(searchfile), countsfile=str(countsfile), statsfile=str(statsfile)) + + table = db[name] + assert table._stats_valid is True + assert stats_valid_in_meta(table) is True + assert table.stats.quick_count(QUERY) == 67 + + +def test_a_refreshing_replacement_restores_validity(db, counted, tmp_path): + """ + And a replacement that does recompute its caches ends valid, serving the + new answer rather than the old one. + """ + name = counted.search_table + counted.update_from_file(_flag_flip_file(counted, tmp_path), restat=True) + + table = db[name] + assert table._stats_valid is True + assert stats_valid_in_meta(table) is True + assert table.count(QUERY) == 0 + + +def test_a_failed_swap_does_not_leave_validity_behind(counted, tmp_path, monkeypatch): + """ + The flag moves in the same transaction as the swap, so a swap that fails + leaves the table exactly as it was -- still valid, still holding the data + the caches describe. + """ + name = counted.search_table + searchfile = tmp_path / "data.txt" + counted.copy_to(str(searchfile), include_id=False) + + def boom(*args, **kwargs): + raise RuntimeError("swap blew up") + + monkeypatch.setattr(type(counted), "_swap_in_tmp", boom) + with pytest.raises(RuntimeError): + counted.reload(str(searchfile), restat=False) + counted._db.conn.rollback() + counted.drop_tmp() + + counted._refresh() + assert counted._stats_valid is True + assert stats_valid_in_meta(counted) is True + assert counted.stats.quick_count(QUERY) == 67 + assert counted.search_table == name + + +def test_resort_of_a_non_saving_table_ends_invalid(db, table_factory): + """ + Pins the one place the rule is deliberately pessimistic. ``resort()`` + renumbers rows without changing them, so the cached counts would still be + accurate -- but it goes through the replacement path, which rebuilds the + caches only with ``saving`` on and otherwise cannot know they still match. + Documented in ``resort``'s docstring: refresh afterwards. + """ + table = table_factory(sort=["n"]) + table.insert_many([sample_row(i) for i in [3, 1, 2, 0]]) + assert table.stats.saving is False + name = table.search_table + table._execute( + SQL("UPDATE meta_tables SET stats_valid = true WHERE name = %s"), [name] + ) + table._db.conn.commit() + table._refresh() + + table.resort() + assert stats_valid_in_meta(db[name]) is False + + +def test_resort_of_a_saving_table_stays_valid(db, counted): + """ + With saving on, the resort rebuilds the counts against the renumbered + rows, so the table is entitled to go on serving cached answers. (The + rebuild recomputes the recorded statistics families; a one-off count + recorded by hand beforehand is not among them and is not restored.) + """ + name = counted.search_table + # sample_row(1000) has flag False, so the cached query still counts 67; + # the insert is here to break the order and give resort() work to do. + counted.insert_many([sample_row(1000)], restat=False) + counted.stats.refresh_stats() + counted.resort() + + # The swap installs a fresh table object, and `saving` lives on the + # object rather than in meta_tables, so turn it back on to record. + table = db[name] + table.stats.saving = True + assert table._stats_valid is True + assert stats_valid_in_meta(table) is True + assert table.stats.count(QUERY, record=True) == 67 + assert table.stats.quick_count(QUERY) == 67 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..63d8a58 --- /dev/null +++ b/tests/test_write_invariants.py @@ -0,0 +1,273 @@ +# -*- 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 + + +def _sort_key_update(tmp_path, name="u.txt"): + """ + An update file moving one row's ``n``, the configured sort column. + """ + dump = tmp_path / name + with open(dump, "w") as F: + F.write("label|n\ntext|integer\n\nl5|9999\n") + return str(dump) + + +def test_explicit_resort_false_still_records_a_broken_order_inplace(ordered, tmp_path): + """ + ``resort=False`` asks not to rebuild; it does not claim the sort was left + alone. The two used to be one variable, so passing it suppressed the + bookkeeping as well as the rebuild and left the table asserting that + ORDER BY id still matched the sort. + """ + ordered.update_from_file( + _sort_key_update(tmp_path), inplace=True, resort=False, restat=False + ) + assert flags(ordered)["out_of_order"] is True + assert ordered._out_of_order is True + + +def test_explicit_resort_false_survives_the_swap_on_the_replacement_path(ordered, tmp_path): + """ + Same for the non-inplace path: the replacement table was not renumbered, + so the table that goes live is out of order and must say so afterwards. + """ + ordered.update_from_file(str(_sort_key_update(tmp_path)), resort=False, restat=False) + assert flags(ordered)["out_of_order"] is True + + +def test_a_resorted_replacement_is_marked_ordered(ordered, tmp_path): + """ + The other half: a replacement that really was renumbered in sort order may + claim to be ordered. + """ + ordered.update_from_file(_sort_key_update(tmp_path), resort=True, restat=False) + assert flags(ordered)["out_of_order"] is False + + +def test_inplace_rewrite_marks_the_table_out_of_order(ordered): + """ + An in-place rewrite runs an arbitrary function over every row and dumps + every search column, so nothing can establish that the sort keys came + through unchanged. Its forced resort=False must not be read as evidence + that they did. + """ + ordered.rewrite(lambda rec: rec, inplace=True, restat=False) + assert flags(ordered)["out_of_order"] is True + + +def test_inplace_rewrite_refuses_an_explicit_resort(ordered): + """ + And it refuses to pretend it can rebuild, before running func over the + table rather than after. + """ + with pytest.raises(ValueError, match="resort"): + ordered.rewrite(lambda rec: rec, inplace=True, resort=True) + + +def test_an_update_touching_no_sort_column_keeps_the_order_flag(ordered, tmp_path): + """ + The flag is only pessimistic where it has to be: a file that cannot move + any sort key leaves a healthy table healthy. + """ + dump = tmp_path / "u.txt" + with open(dump, "w") as F: + F.write("label|data\ntext|jsonb\n\nl5|{\"a\": 1}\n") + ordered.update_from_file(str(dump), inplace=True, restat=False) + assert flags(ordered)["out_of_order"] is False + + +# --------------------------------------------------------------------------- +# 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 + + +def test_file_updates_without_restat_clear_stats_valid(db, ordered, tmp_path): + """ + Every file-based write that does not rebuild the caches has to say so in + meta_tables, on both the in-place and the replacement path. Otherwise a + count cached before the write is still served after it; the observable + half of that is pinned in test_stats_validity.py. + """ + dump = tmp_path / "u.txt" + with open(dump, "w") as F: + F.write("label|num\ntext|numeric\n\nl5|12345\n") + + ordered.update_from_file(str(dump), inplace=True, restat=False) + assert flags(ordered)["stats_valid"] is False + + ordered.stats.refresh_stats() + assert flags(ordered)["stats_valid"] is True + + name = ordered.search_table + ordered.update_from_file(str(dump), restat=False) + assert flags(db[name])["stats_valid"] is False + + +def test_rewrite_without_restat_clears_stats_valid(db, ordered): + name = ordered.search_table + ordered.rewrite(lambda rec: rec, restat=False) + assert flags(db[name])["stats_valid"] is False + + +def test_reload_without_fresh_caches_clears_stats_valid(db, ordered, tmp_path): + name = ordered.search_table + dump = tmp_path / "d.txt" + ordered.copy_to(str(dump), include_id=False) + ordered.reload(str(dump), restat=False) + assert flags(db[name])["stats_valid"] is False + # and total is still exact: it is maintained separately, on every write + assert flags(db[name])["total"] == 100 + + +def test_a_restatted_replacement_keeps_stats_valid(db, ordered, tmp_path): + name = ordered.search_table + dump = tmp_path / "d.txt" + ordered.copy_to(str(dump), include_id=False) + ordered.reload(str(dump), restat=True) + assert flags(db[name])["stats_valid"] is True