From a36cb8a208a926de811984c0ed167762f57088fd Mon Sep 17 00:00:00 2001 From: David Roe Date: Tue, 4 Aug 2026 17:52:18 -0400 Subject: [PATCH 1/8] Fix identifier composition, table-specific statistics, log state and random edge cases Four independent defects found reviewing rc2, each with regression tests that fail on rc2 and pass here. max_id/min_id formatted their table argument into the statement as text. A name needing quotes was a syntax error and a name carrying its own statement ran it; both compose an Identifier now. While there, the empty sentinel is documented: max_id returns -1, and 0 is a real id, which is what random() got wrong below. _approx_most_common read reltuples from a hard-coded public.nf_fields but read frequencies from the owning table, so every table other than nf_fields got its own frequencies scaled by an unrelated row count. The row count now comes from the table the statistics are about, looked up by name in the current schema, and the column type goes through column_type_sql rather than being concatenated. update_from_file's logging default was a shared dictionary literal that the method wrote logid and aborted into. Consecutive default calls saw the previous call's values and a caller's dictionary came back modified. An AST scan of the package confirms this was the only mutable default anywhere that is actually mutated, so nothing else needed changing. random() raised IndexError from random.choice([]) when pick_first found no values, reported a table whose only row has id 0 as empty, and discarded rows whose projection was falsy -- a table of zeros exhausted maxtries and raised "Random selection failed!". random_sample returned None for an unrecognized mode, which reads like an empty result, and reseeded the global random module when asked for a repeatable sample. --- CHANGELOG.md | 27 ++++ psycodict/searchtable.py | 45 +++++-- psycodict/statstable.py | 21 ++-- psycodict/table.py | 36 ++++-- tests/test_correctness.py | 257 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 360 insertions(+), 26 deletions(-) create mode 100644 tests/test_correctness.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 98cafa4..37857ea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -339,6 +339,33 @@ hardening standalone use; the highlights: Connect — no long-lived token is stored anywhere. (#113) - `CITATION.cff`, so GitHub renders a citation for the package. (#124) +### Fixed after the first release candidates + +- **`max_id` and `min_id` compose the table name they are given.** Both took a + `table=` argument and formatted it into the statement as text, so a name + needing quotes was a syntax error and a name carrying its own statement ran + it. Both now use `Identifier`. `max_id` returns -1 for an empty table, which + is the only empty sentinel: 0 is a real id, and `random()` treated a table + whose single row had id 0 as empty. +- **Approximate statistics are scaled by the table they describe.** + `_approx_most_common` took `reltuples` from a hard-coded `public.nf_fields` + while taking frequencies from the real table, so on every other table the + estimate was that table's frequencies multiplied by an unrelated row count. + The column type it interpolates now goes through the validated + `column_type_sql`. +- **`update_from_file` no longer shares log state between calls.** Its + `logging` default was a dictionary literal that the method wrote `logid` and + `aborted` into, so consecutive default calls saw each other's values and a + caller-supplied dictionary came back modified. The default is now `None` and + the mapping is copied per call. +- **Random selection edge cases.** `random(query, pick_first=...)` returned + `None` rather than raising `IndexError` when nothing satisfies the query; a + projected value of `0`, `False`, `""` or `[]` counts as a result instead of + being skipped until `maxtries` ran out; `random_sample` raises `ValueError` + naming the accepted modes instead of silently returning `None` for an + unknown one; and a repeatable `choice` sample uses a local + `random.Random(seed)` rather than reseeding the process-wide generator. + ### Release candidates 1.0.0 is published as a sequence of release candidates first. `pip` ignores 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..27d6edd 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 @@ -1651,21 +1651,24 @@ 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 = current_schema() AND c.relname = %s) c +WHERE schemaname = current_schema() 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])) + cur = self._execute(selecter, [self.search_table, self.search_table, col, n]) return [tuple(x) for x in cur] def _common_cols(self, threshold=700): diff --git a/psycodict/table.py b/psycodict/table.py index a7a24a5..3e059ec 100644 --- a/psycodict/table.py +++ b/psycodict/table.py @@ -1350,7 +1350,7 @@ def update_from_file( resort=None, reindex=None, restat=True, - logging={"operation":"file_update"}, + logging=None, **kwds ): """ @@ -1373,7 +1373,9 @@ def update_from_file( - ``resort`` -- whether this table should be resorted after updating (default is to resort when the sort columns intersect the updated columns) - ``reindex`` -- only meaningful when ``inplace`` is set: whether to drop the indexes touching the updated columns before the update and recreate them afterward, which is faster when many rows change (by default this is done when more than 1000 rows are updated). Without ``inplace``, all indexes are necessarily recreated on the replacement table, so ``reindex=True`` is redundant and ``reindex=False`` raises an error. - ``restat`` -- whether to recompute stats for the table - - ``logging`` -- a dictionary of keyword arguments for _log_db_change + - ``logging`` -- a dictionary of keyword arguments for _log_db_change. + A copy is taken, so the caller's dictionary is not modified and two + calls sharing one dictionary do not see each other's ``logid``. - ``kwds`` -- passed on to the ``COPY`` command. Cannot include "columns". """ self._forbid_reindex_false(reindex, inplace) @@ -1384,8 +1386,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)) @@ -1505,11 +1511,11 @@ def drop_tmp(): self._set_ordered() # Delete the temporary table used to load the data drop_tmp() - logging["logid"] = logid - logging["aborted"] = False + log_data["logid"] = logid + log_data["aborted"] = False print("Updated %s in %.3f secs" % (self.search_table, time.time() - now)) finally: - self._log_db_change(**logging) + self._log_db_change(**log_data) def delete(self, query, restat=True): """ @@ -2848,10 +2854,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 +2872,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 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 From 798fdd954db41f4a1ac5454f6ddc6296ab45a1b1 Mon Sep 17 00:00:00 2001 From: David Roe Date: Tue, 4 Aug 2026 18:07:59 -0400 Subject: [PATCH 2/8] Stop serving cached statistics while they are marked invalid Write paths called _break_stats, but read paths queried the cache tables regardless, so a count cached before a restat=False write kept being served afterwards. Reproduced against PostgreSQL 18: a query counted at 67, every matching row then changed so that none satisfy it, and quick_count still answered 67. Every lookup that would serve a cached answer now goes through one predicate, _may_use_cache, and reports a miss while stats_valid is false -- quick_count, quick_count_distinct, _quick_statistic, _has_stats, _has_numstats and null_counts, which between them are what make count, max, min, sum, column_counts and numstats recompute rather than return a stored value. Three things are deliberately outside the rule, and the predicate's docstring says why: the empty-query total, which is maintained on every write and stays exact; a suffixed table, whose caches are its own and which stats_valid says nothing about; and the _status/status/extra_counts inventory, which reports what the cache contains rather than answering a question about the data -- refresh_stats uses it to discover what to recompute, so gating it would make an invalid table forget what statistics it is supposed to have. The flag had no way back to true. _restore_stats is the counterpart of _break_stats, called at the end of refresh_stats inside the same transaction that rebuilt the caches, so a refresh that fails part-way leaves the table marked invalid rather than claiming a cache it does not have. A suffixed refresh does not touch the live flag. Separately, bulk paths now run PostgreSQL's own ANALYZE, which is a different thing from psycodict's statistics: a bulk-loaded relation has none until autovacuum reaches it and the planner costs it as though it were tiny. The _tmp copies are analyzed before the swap and outside its transaction, since the catalog entry follows the relation through a rename -- one call in _swap_in_tmp covers reload, rewrite, non-inplace update_from_file and staged commits, which all funnel through it. copy_from analyzes the live table. The existing statistics fixtures insert rows, which invalidates, and then assume a usable cache; they now say so with _restore_stats, which is true of them (nothing is cached yet and the total is maintained by the insert) and is the state a freshly loaded table is in. --- CHANGELOG.md | 36 ++++ DataManagement.md | 27 ++- psycodict/statstable.py | 77 +++++++- psycodict/table.py | 40 ++++ tests/test_doctests.py | 7 + tests/test_stats.py | 5 + tests/test_stats_duplicates.py | 9 + tests/test_stats_validity.py | 325 +++++++++++++++++++++++++++++++++ 8 files changed, 524 insertions(+), 2 deletions(-) create mode 100644 tests/test_stats_validity.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 37857ea..b29f754 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -366,6 +366,42 @@ hardening standalone use; the highlights: unknown one; and a repeatable `choice` sample uses a local `random.Random(seed)` rather than reseeding the process-wide generator. +- **`stats_valid` is enforced, not just recorded.** Write paths cleared the + flag but read paths ignored it, so a count cached before a `restat=False` + write kept being served afterwards -- verified: a query counted at 67, then + every matching row changed, still answered 67. Every lookup that would serve + a cached answer now goes through one predicate and reports a miss while the + flag is false: `quick_count`, `quick_count_distinct` and `_quick_statistic`, + which is what makes `count`, `max`, `min` and `sum` compute the answer + instead of returning a recorded one. The line is whether a miss costs one + bounded query or a rebuild, so these are deliberately not gated: the + empty-query `total`, maintained on every write and so exact; the `_status` / + `status` / `extra_counts` inventory, which is how `refresh_stats` discovers + what to recompute; `_has_stats` / `_has_numstats`, which decide whether a + whole statistics family needs computing; and `null_counts`, whose fallback + is one full count *per search column*. Gating that last group made + `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, four minutes of downstream suite became over + forty-five. **The gap that leaves:** `column_counts`, `numstats` and + `null_counts` can still report a value recorded before an unrefreshed + write. + Closing it needs freshness per statistic rather than one flag per table, + which is a metadata format change; `refresh_stats()` is the remedy + meanwhile. A suffixed (`_tmp`, `_oldN`) table is not gated by the live + table's flag, since it carries its own caches. The flag is restored only by + `refresh_stats()`, inside the transaction that rebuilt the caches, so a + failed refresh leaves the table invalid; refreshing a `_tmp` copy does not + validate the live table. +- **Bulk paths run `ANALYZE`.** A relation that has just been bulk loaded has + no planner statistics until autovacuum reaches it, so queries against it are + costed as though it were tiny. Replacement tables are analyzed while still + named `_tmp` -- before the swap, and outside its transaction, since the + catalog entry follows the relation through the rename -- which covers + `reload`, `rewrite`, non-inplace `update_from_file` and staged commits + through the one helper they share; `copy_from` analyzes the live table it + loaded into. + ### Release candidates 1.0.0 is published as a sequence of release candidates first. `pip` ignores diff --git a/DataManagement.md b/DataManagement.md index 0554b22..7fd2aca 100644 --- a/DataManagement.md +++ b/DataManagement.md @@ -108,7 +108,32 @@ These mutate the live table directly. They are convenient for small edits; for * **`update(query, changes, resort=False, restat=True)`** — a plain SQL `UPDATE` of every row matching `query`; `changes` maps column names to constants. * **`delete(query, restat=True)`** — deletes every row matching `query` and decrements `total`. -**Statistics invalidation.** Any write that can change the data calls `_break_stats`, which sets `meta_tables.stats_valid = false` so that cached statistics are known to be stale. If the table has `saving` on and you left `restat=True`, statistics are refreshed at the end of the call; otherwise they are simply marked invalid. Inserting rows (and updating a sort-key column) also calls `_break_order`, setting `out_of_order = true` to record that the `id` order no longer matches `sort`; `delete` leaves the order flag alone. +**Statistics invalidation.** Any write that can change the data calls `_break_stats`, which sets `meta_tables.stats_valid = false` so that cached statistics are known to be stale. If the table has `saving` on and you left `restat=True`, statistics are refreshed at the end of the call; otherwise they are simply marked invalid. + +`stats_valid` is enforced rather than merely recorded: while it is false, a +cached nonempty-query count, distinct count, minimum, maximum or sum reports a +miss, and the method computes the answer instead of returning the stored one. +The empty-query `total` is the exception, since it is maintained on every write +and so stays exact. + +`column_counts`, `numstats` and `null_counts` are the other exception, and a +caveat worth knowing. The line is what a cache miss costs: the counts above +fall back to a single statement about the rows in question, while these fall +back to rebuilding a whole statistics family, or to one full count per search +column. Making them miss while the table is invalid would rebuild on every +call and never converge, since only `refresh_stats()` restores the flag, so +they read what is recorded. A value recorded before an unrefreshed write is +therefore still reported by them; run `refresh_stats()` after a write you did +not `restat`. The flag goes back to true only in `refresh_stats()`, inside +the transaction that rebuilt the caches, so a refresh that fails part-way leaves +the table marked invalid rather than claiming a cache it does not have. +Refreshing a `_tmp` copy does not validate the live table. + +Bulk paths also run PostgreSQL's own `ANALYZE`, which is a different thing from +psycodict's statistics: a freshly loaded relation has no planner statistics +until autovacuum reaches it. Replacement tables are analyzed while still named +`_tmp`, before the swap, since the catalog entry follows the relation through +the rename; `copy_from` analyzes the live table it loaded into. Inserting rows (and updating a sort-key column) also calls `_break_order`, setting `out_of_order = true` to record that the `id` order no longer matches `sort`; `delete` leaves the order flag alone. ### Resorting is disabled diff --git a/psycodict/statstable.py b/psycodict/statstable.py index 27d6edd..776570b 100644 --- a/psycodict/statstable.py +++ b/psycodict/statstable.py @@ -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: @@ -1847,6 +1916,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 3e059ec..57520ea 100644 --- a/psycodict/table.py +++ b/psycodict/table.py @@ -1182,6 +1182,20 @@ def _break_stats(self): self._execute(updater, [self.search_table], silent=True) self._stats_valid = False + def _restore_stats(self): + """ + Record that the cached counts and statistics agree with the live data. + + The counterpart of :meth:`_break_stats`, and the only way the flag goes + back to true. Call it from inside the transaction that rebuilt or + loaded the caches, so that a failure part-way through leaves the table + marked invalid rather than claiming a cache it does not have. + """ + if not self._stats_valid: + updater = SQL("UPDATE meta_tables SET stats_valid = true WHERE name = %s") + self._execute(updater, [self.search_table], silent=True) + self._stats_valid = True + def _break_order(self): """ This function should be called when the id ordering is invalidated by an insertion or update. @@ -1943,6 +1957,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 @@ -1953,6 +1985,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)) @@ -2937,6 +2973,10 @@ def copy_from( 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/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_stats.py b/tests/test_stats.py index f64c8cb..f31482b 100644 --- a/tests/test_stats.py +++ b/tests/test_stats.py @@ -31,6 +31,11 @@ def saving_table(table_factory): table = table_factory() table.insert_many([sample_row(i) for i in range(200)]) table.stats.saving = True + # insert_many invalidates the statistics, and a cached count may not be + # used while they are invalid. Nothing is cached yet and the total is + # maintained by the insert, so the caches do agree with the data: say so, + # which is the state these tests are about. + table._restore_stats() return table diff --git a/tests/test_stats_duplicates.py b/tests/test_stats_duplicates.py index 26141a9..94d0acf 100644 --- a/tests/test_stats_duplicates.py +++ b/tests/test_stats_duplicates.py @@ -39,6 +39,11 @@ def saving_table(table_factory): table = table_factory() table.insert_many([sample_row(i) for i in range(200)]) table.stats.saving = True + # insert_many invalidates the statistics, and a cached count may not be + # used while they are invalid. Nothing is cached yet and the total is + # maintained by the insert, so the caches do agree with the data: say so, + # which is the state these tests are about. + table._restore_stats() return table @@ -265,6 +270,10 @@ def constrained_table(table_factory): [{"n": i, "a": i % 3, "z": i % 2, "label": "l%d" % i} for i in range(30)] ) table.stats.saving = True + # As in ``saving_table``: nothing is cached yet, so the (empty) caches do + # agree with the data, and these tests are about what add_numstats writes + # rather than about invalidation. + table._restore_stats() return table diff --git a/tests/test_stats_validity.py b/tests/test_stats_validity.py new file mode 100644 index 0000000..a19b9c4 --- /dev/null +++ b/tests/test_stats_validity.py @@ -0,0 +1,325 @@ +# -*- coding: utf-8 -*- +""" +``stats_valid`` means what it says: no cached answer survives it being false. + +Before this, write paths cleared the flag but read paths ignored it, so a +count cached before a ``restat=False`` write kept being served afterwards. +These tests pin the whole contract -- which lookups are gated, which two are +deliberately not, and how the flag is restored. +""" +import pytest + +from psycopg.sql import SQL, Identifier + +from conftest import sample_row + + +@pytest.fixture +def cached_table(table_factory): + """ + A saving table with statistics computed and recorded, and the flag true. + """ + table = table_factory() + table.insert_many([sample_row(i) for i in range(200)]) + table.stats.saving = True + table.stats.refresh_stats() + table._restore_stats() if hasattr(table, '_restore_stats') else None + return table + + +def stats_valid_in_meta(table): + """ + The flag as stored, rather than as cached on the Python object. + """ + cur = table._execute( + SQL("SELECT stats_valid FROM meta_tables WHERE name = %s"), + [table.search_table], + ) + return cur.fetchone()[0] + + +# --------------------------------------------------------------------------- +# every gated lookup reports a miss while the flag is false +# --------------------------------------------------------------------------- + +def test_a_stale_count_is_not_served_after_an_unrestatted_write(cached_table): + """ + The case from the review: cache a nonempty query, change the rows it + matches without refreshing, and the old number kept coming back. + """ + query = {"flag": True} + before = cached_table.stats.count(query, record=True) + assert cached_table.stats.quick_count(query) == before + + cached_table.update(query, {"flag": False}, restat=False) + assert not cached_table._stats_valid + + # the cached row is still physically there ... + cur = cached_table._execute( + SQL("SELECT count FROM {0} WHERE cols = %s").format( + Identifier(cached_table.stats.counts) + ), + [cached_table.stats._split_dict(query)[0]], + ) + assert cur.rowcount + + # ... but it is not an answer any more, and count() computes the truth + assert cached_table.stats.quick_count(query) is None + assert cached_table.stats.count(query) == 0 + + +def test_quick_count_distinct_is_gated(cached_table): + cols = ["flag"] + cached_table.stats._slow_count_distinct(cols, record=True) + assert cached_table.stats.quick_count_distinct(cols) is not None + cached_table._break_stats() + assert cached_table.stats.quick_count_distinct(cols) is None + + +def test_quick_statistic_is_gated(cached_table): + from psycodict.encoding import Json + + assert cached_table.stats.max("n") == 199 + ccols, cvals = Json([]), Json([]) + assert cached_table.stats._quick_statistic("n", ccols, cvals, "max") is not None + cached_table._break_stats() + assert cached_table.stats._quick_statistic("n", ccols, cvals, "max") is None + # and the public method still returns the right answer, the slow way + assert cached_table.stats.max("n") == 199 + + +def test_the_recompute_predicates_are_not_gated(cached_table): + """ + _has_stats and _has_numstats answer "is this recorded", which is what + add_stats and column_counts use to decide whether to compute. Gating them + makes those recompute the whole family on every call and never converge, + because only refresh_stats restores the flag: measured on the LMFDB, that + turned a four-minute downstream suite into one still running after + forty-five. They stay ungated, and the staleness that leaves is recorded + in the test below. + """ + from psycodict.encoding import Json + + cached_table.stats.add_stats(["flag"]) + cached_table.stats.add_numstats("num", ["flag"]) + jcols, empty = Json(["flag"]), Json([]) + assert cached_table.stats._has_stats(jcols, empty, empty, None) + assert cached_table.stats._has_numstats(Json(["num"]), Json(["flag"]), empty, None) + + cached_table._break_stats() + assert cached_table.stats._has_stats(jcols, empty, empty, None) + assert cached_table.stats._has_numstats(Json(["num"]), Json(["flag"]), empty, None) + + +@pytest.mark.xfail( + reason="column_counts can still report a value recorded before an " + "unrefreshed write; closing this needs freshness per statistic " + "rather than one flag per table", + strict=True, +) +def test_column_counts_can_still_be_stale(cached_table): + """ + The gap left by the paragraph above, pinned so that it is a known quantity + rather than a surprise, and so that a future per-statistic freshness change + turns this green. + """ + cached_table.stats.add_stats(["flag"]) + flagged = cached_table.stats.column_counts("flag")[True] + assert flagged > 0 + cached_table.update({"flag": True}, {"flag": False}, restat=False) + assert cached_table.stats.column_counts("flag").get(True, 0) == 0 + + +def test_null_counts_is_not_gated(cached_table): + """ + A miss here costs one full count per search column, not one bounded query, + so null_counts reads what is recorded like the other bulk paths. This is + the call LMFDB's results_complete makes for every query it checks, and + gating it is what took the downstream suite past forty-five minutes. + """ + cached_table.stats.refresh_null_counts() + before = cached_table.stats.null_counts() + cached_table._break_stats() + assert cached_table.stats.null_counts() == before + + +# --------------------------------------------------------------------------- +# what is deliberately not gated +# --------------------------------------------------------------------------- + +def test_the_empty_query_total_survives_invalidation(cached_table): + """ + total is maintained on every write, so it is exact regardless of the flag; + gating it would make count() do a full scan after every insert. + """ + cached_table.insert_many([sample_row(1000)], restat=False) + assert not cached_table._stats_valid + assert cached_table.stats.quick_count({}) == 201 + assert cached_table.count() == 201 + + +def test_status_still_reports_what_the_cache_holds(cached_table): + """ + refresh_stats learns which statistics to recompute from _status, so gating + it would make an invalid table forget what it is supposed to have. + """ + cached_table.stats.add_stats(["flag"]) + before = cached_table.stats._status() + cached_table._break_stats() + assert cached_table.stats._status() == before + + +def test_a_suffixed_table_is_not_gated_by_the_live_flag(cached_table): + """ + A _tmp copy carries its own caches; stats_valid describes the live table. + """ + table = cached_table + assert table.stats.count({"flag": True}, record=True) > 0 + tmp = table.search_table + "_tmp" + table._db._execute( + SQL("CREATE TABLE {0} AS TABLE {1}").format( + Identifier(tmp), Identifier(table.search_table) + ) + ) + table._db._execute( + SQL("CREATE TABLE {0} AS TABLE {1}").format( + Identifier(table.stats.counts + "_tmp"), Identifier(table.stats.counts) + ) + ) + try: + table._break_stats() + assert table.stats.quick_count({"flag": True}) is None + assert table.stats.quick_count({"flag": True}, suffix="_tmp") is not None + finally: + for name in (tmp, table.stats.counts + "_tmp"): + table._db._execute(SQL("DROP TABLE IF EXISTS {0}").format(Identifier(name))) + + +# --------------------------------------------------------------------------- +# restoring the flag +# --------------------------------------------------------------------------- + +def test_refresh_stats_restores_the_flag_and_the_cache(cached_table): + cached_table.update({"flag": True}, {"flag": False}, restat=False) + assert not cached_table._stats_valid + assert stats_valid_in_meta(cached_table) is False + + cached_table.stats.refresh_stats() + assert cached_table._stats_valid + assert stats_valid_in_meta(cached_table) is True + assert cached_table.stats.count({"flag": False}, record=True) == 200 + assert cached_table.stats.quick_count({"flag": False}) == 200 + + +def test_a_failed_refresh_leaves_the_table_invalid(cached_table, monkeypatch): + """ + The flag is set inside the refresh transaction, so a failure part-way + cannot leave a table claiming a cache it does not have. + """ + cached_table._break_stats() + + def boom(*args, **kwargs): + raise RuntimeError("refresh blew up") + + monkeypatch.setattr(cached_table.stats, "refresh_null_counts", boom) + with pytest.raises(RuntimeError): + cached_table.stats.refresh_stats() + cached_table._db.conn.rollback() + + cached_table._refresh() + assert not cached_table._stats_valid + assert stats_valid_in_meta(cached_table) is False + + +def test_refreshing_a_tmp_copy_does_not_validate_the_live_table(cached_table): + table = cached_table + for base in (table.search_table, table.stats.counts, table.stats.stats): + table._db._execute( + SQL("CREATE TABLE {0} AS TABLE {1}").format( + Identifier(base + "_tmp"), Identifier(base) + ) + ) + try: + table._break_stats() + table.stats.refresh_stats(suffix="_tmp") + assert not table._stats_valid + assert stats_valid_in_meta(table) is False + finally: + for base in (table.search_table, table.stats.counts, table.stats.stats): + table._db._execute( + SQL("DROP TABLE IF EXISTS {0}").format(Identifier(base + "_tmp")) + ) + + +# --------------------------------------------------------------------------- +# planner statistics +# --------------------------------------------------------------------------- + +def analyzed(table, name=None): + """ + Whether PostgreSQL holds planner statistics for a relation. + """ + cur = table._execute( + SQL( + "SELECT c.reltuples >= 0 AND s.last_analyze IS NOT NULL " + "OR s.last_analyze IS NOT NULL " + "FROM pg_class c " + "JOIN pg_namespace n ON n.oid = c.relnamespace " + "LEFT JOIN pg_stat_all_tables s ON s.relid = c.oid " + "WHERE n.nspname = current_schema() AND c.relname = %s" + ), + [name or table.search_table], + ) + row = cur.fetchone() + return bool(row and row[0]) + + +def test_a_reload_analyzes_before_the_swap(cached_table, tmp_path): + """ + A bulk-loaded relation has no planner statistics until autovacuum reaches + it, and a rename carries the catalog entry along, so the _tmp copy is + analyzed while it is still _tmp. + """ + searchfile = tmp_path / "data.txt" + cached_table.copy_to(str(searchfile)) + + seen = [] + original = type(cached_table)._analyze + + def record(self, tables, suffix=""): + seen.append((list(tables), suffix)) + return original(self, tables, suffix) + + type(cached_table)._analyze = record + try: + cached_table.reload(str(searchfile)) + finally: + type(cached_table)._analyze = original + + assert seen, "the reload did not analyze anything" + tables, suffix = seen[0] + assert suffix == "_tmp" + assert cached_table.search_table in tables + assert analyzed(cached_table) + + +def test_copy_from_analyzes_the_live_table(cached_table, table_factory, tmp_path): + searchfile = tmp_path / "more.txt" + cached_table.copy_to(str(searchfile)) + target = table_factory() + + seen = [] + original = type(target)._analyze + + def record(self, tables, suffix=""): + seen.append((list(tables), suffix)) + return original(self, tables, suffix) + + type(target)._analyze = record + try: + target.copy_from(str(searchfile), restat=False) + finally: + type(target)._analyze = original + + assert seen == [([target.search_table], "")] + assert target.count() == 200 From a0ab659b4f4d236caba283913071980695a6f8a7 Mon Sep 17 00:00:00 2001 From: David Roe Date: Tue, 4 Aug 2026 18:19:20 -0400 Subject: [PATCH 3/8] Operate in exactly one schema, and filter every catalog query to it Unqualified DDL and DML went wherever search_path happened to point, while catalog inspection was a mixture of hard-coded 'public' and no filter at all. With a relation of the same name in two schemas the answers came from both: _column_types unioned their columns (or raised "Type mismatch"), an index or constraint in the other schema counted as present, _all_tablenames listed the name twice, and table_sizes reported only public whatever the session was using. PostgresDatabase now takes schema="public", validates it as an identifier once in the constructor, and pins search_path to it in _configure_session -- which runs for the first connection and for every replacement, so a reconnect cannot come back pointing somewhere else. It is deliberately not part of _connect_kwargs: psycopg.connect has no such parameter, and passing it there would reach the driver. Every catalog query is then filtered to that schema, binding it as a value rather than interpolating it: _table_exists, _all_tablenames, _index_exists, _list_indexes, _relation_exists, _constraint_exists, _list_constraints, _column_types, _relation_columns, refresh_tables' column discovery, the read-only and knowls capability probes, _grantees, the legacy-extras check, table_sizes, tablespaces, _check_tmp_leftovers' two probes, the metadata bootstrap's existing-table set, _approx_most_common and dbdiff's column reader. _schema_relations and _approx_most_common previously asked the server with current_schema(); they now bind the same value as everything else, so there is one notion of which schema this is. The userdb.users grant probe keeps its own schema: that one is deliberately about a different schema, not about this database's. --- CHANGELOG.md | 13 +++ DataManagement.md | 8 ++ psycodict/base.py | 61 +++++++---- psycodict/database.py | 57 ++++++---- psycodict/dbdiff.py | 4 +- psycodict/statstable.py | 9 +- psycodict/table.py | 15 ++- psycodict/validation.py | 13 +++ tests/test_schema_contract.py | 189 ++++++++++++++++++++++++++++++++++ 9 files changed, 322 insertions(+), 47 deletions(-) create mode 100644 tests/test_schema_contract.py diff --git a/CHANGELOG.md b/CHANGELOG.md index b29f754..bb65b2b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -402,6 +402,19 @@ hardening standalone use; the highlights: through the one helper they share; `copy_from` analyzes the live table it loaded into. +- **A database operates in exactly one schema.** `PostgresDatabase` takes a + `schema=` argument (default `"public"`, so nothing changes for existing + deployments), validates it as an identifier, and pins `search_path` to it on + the first connection and on every replacement. Catalog inspection was + previously a mixture of hard-coded `'public'` and no filter at all -- 30-odd + queries across `pg_tables`, `pg_indexes`, `pg_class`, `pg_constraint` and + `information_schema` -- so with two schemas holding a relation of the same + name, column discovery could mix their columns, an index or constraint in + the other schema counted as present, and `_all_tablenames` listed the name + twice. Every one is now filtered to the selected schema, which is bound as a + value rather than interpolated. *Migration:* none unless you were relying on + psycodict seeing relations outside `public`, which it did only by accident. + ### Release candidates 1.0.0 is published as a sequence of release candidates first. `pip` ignores diff --git a/DataManagement.md b/DataManagement.md index 7fd2aca..c9b9163 100644 --- a/DataManagement.md +++ b/DataManagement.md @@ -64,6 +64,14 @@ Copies the column layout, `label_col`, `sort`, `id_ordered`, `id` type and descr psycodict keeps its own bookkeeping in a handful of tables that live alongside your search tables: +psycodict operates in one PostgreSQL schema, `public` unless the constructor is +given another (`PostgresDatabase(schema="myschema")`). Every relation it +creates goes there, every relation it looks for is looked for there, and every +catalog query is filtered to it, so a relation of the same name in another +schema is neither mistaken for one of these nor merged with it. The schema is +pinned in each connection's `search_path`, including replacement connections +after a reconnect. + * **`meta_tables`** — one row per search table, holding `name`, `sort`, `count_cutoff`, `id_ordered`, `out_of_order`, `stats_valid`, `label_col`, `total`, `important` and `include_nones`. This is the source of truth psycodict reads on connection to reconstruct each table object. * **`meta_indexes`** and **`meta_constraints`** — one row per index / constraint, recording how to rebuild it. `reload` and `restore_indexes` rebuild from these rows, **not** from whatever is physically on the table (see [reload](#reload)). * **`meta_tables_hist`**, **`meta_indexes_hist`**, **`meta_constraints_hist`** — versioned history of the three tables above, so that `reload_meta`/`revert_meta` can roll a table's metadata forward and back. diff --git a/psycodict/base.py b/psycodict/base.py index ae61f04..30ead59 100644 --- a/psycodict/base.py +++ b/psycodict/base.py @@ -423,14 +423,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 +546,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 +574,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 +602,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 +612,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 +628,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 +649,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 +829,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 +858,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, ) diff --git a/psycodict/database.py b/psycodict/database.py index 7cd95f9..c1703cb 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] @@ -522,11 +528,19 @@ def _register_object(self, obj): self._objects.append(obj) def __init__(self, config=None, secretsfile=None, create=False, upgrade=False, - session_settings=None, grant_policy=None, **kwargs): + 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/statstable.py b/psycodict/statstable.py index 776570b..8540f4b 100644 --- a/psycodict/statstable.py +++ b/psycodict/statstable.py @@ -1733,11 +1733,14 @@ def _approx_most_common(self, col, n): CROSS JOIN ( SELECT c.reltuples FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace - WHERE n.nspname = current_schema() AND c.relname = %s) c -WHERE schemaname = current_schema() AND tablename = %s AND attname = %s + 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), column_type_sql(self.table.col_type[col])) - cur = self._execute(selecter, [self.search_table, self.search_table, col, n]) + 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): diff --git a/psycodict/table.py b/psycodict/table.py index 57520ea..4c1fa0e 100644 --- a/psycodict/table.py +++ b/psycodict/table.py @@ -328,7 +328,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): @@ -2063,9 +2066,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, ) ] @@ -2078,9 +2083,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 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/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() From 5072dd6e6e561af08afecf884fdd3405c03c873a Mon Sep 17 00:00:00 2001 From: David Roe Date: Tue, 4 Aug 2026 23:56:50 -0400 Subject: [PATCH 4/8] Rebuild in resort(), stop row-level writes from resorting, drop finalize_changes resort() was a disabled no-op: the old implementation renumbered every id with an in-place UPDATE, which stalls replication and leaves the rows in their old physical order, so the point of id-ordering -- sequential disk reads -- was never achieved. It now rebuilds. The table is dumped without ids and reloaded; reload assigns ids 1..N in sort order via a new private _generate_sorted_ids, which is a physical rebuild (INSERT ... SELECT ... ORDER BY into a fresh table that replaces the original) rather than an in-place UPDATE. Everything else -- the primary key, indexes, constraints, grants, counts/stats companions, ANALYZE and the _oldN backup -- comes from reload's one replacement path, so resort() adds almost no orchestration of its own. reload and non-inplace update_from_file call _generate_sorted_ids directly rather than public resort(), so there is no recursion, and the renumber now runs before the keys are rebuilt (it replaces the table, so the table must have no pkey/indexes at that point). Because a resort is a full-table rebuild, it is no longer a side effect of a small write. resort=True on insert_many, update, copy_from and in-place update_from_file raises, pointing at resort(); resort=False is unaffected. rewrite defaults resort to the replacement path only, and a staged table's in-place writes drop the request. An in-place update that changes a sort key now records out_of_order rather than pretending it could reorder. finalize_changes() was a documented public no-op; it is removed. The write methods already leave total, the order flag and stats_valid correct on return, which test_write_invariants now checks for every path. scripts/audit_id_order.py is a read-only check that streams each id_ordered table in sort order (server-side cursor, constant client memory) and reports whether the ids actually increase, since the flag can drift and must not be trusted blindly during the production audit. --- CHANGELOG.md | 22 +++ DataManagement.md | 42 ++++- psycodict/table.py | 289 ++++++++++++++++++++++----------- scripts/audit_id_order.py | 125 ++++++++++++++ tests/test_id_order_audit.py | 90 ++++++++++ tests/test_resort.py | 175 ++++++++++++++++++++ tests/test_write.py | 32 ++-- tests/test_write_invariants.py | 153 +++++++++++++++++ 8 files changed, 815 insertions(+), 113 deletions(-) create mode 100644 scripts/audit_id_order.py create mode 100644 tests/test_id_order_audit.py create mode 100644 tests/test_resort.py create mode 100644 tests/test_write_invariants.py diff --git a/CHANGELOG.md b/CHANGELOG.md index bb65b2b..85137f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -415,6 +415,28 @@ hardening standalone use; the highlights: value rather than interpolated. *Migration:* none unless you were relying on psycodict seeing relations outside `public`, which it did only by accident. +- **`resort()` rebuilds and swaps instead of renumbering in place.** It was a + disabled no-op (the in-place `UPDATE` of every id stalled replication and left + the rows in their old physical order, defeating the point of id-ordering). + It now dumps the table, loads it into a fresh table whose ids are assigned + `1..N` in sort order, and swaps it in through `reload`'s machinery -- primary + key, indexes, constraints, grants and counts/stats companions rebuilt, + `ANALYZE` run, previous table kept as an `_oldN` backup. **The ids change.** + A table already ordered reports nothing to do unless `force=True`. +- **Row-level writes no longer resort.** `resort=True` on `insert_many`, + `update`, `copy_from` or an in-place `update_from_file` now raises and points + at `resort()`, so a small write cannot silently trigger a full-table rebuild; + `resort=False` is unaffected. `reload`, `rewrite` and a non-inplace + `update_from_file` still establish order as part of the replacement they were + already building. *Migration:* drop `resort=True` from row-level calls and + call `table.resort()` in a maintenance window instead. +- **`finalize_changes()` is removed.** It was a documented public no-op; the + supported write methods already leave `total`, the order flag and + `stats_valid` correct when they return. +- **`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. + ### Release candidates 1.0.0 is published as a sequence of release candidates first. `pip` ignores diff --git a/DataManagement.md b/DataManagement.md index c9b9163..4b118b2 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[]`, ...). @@ -141,11 +141,45 @@ 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. +the rename; `copy_from` analyzes the live table it loaded into. Inserting rows (and updating a sort-key column) also calls `_break_order`, setting `out_of_order = true` to record that the `id` order no longer matches `sort`; `delete` leaves the order flag alone. `resort()` (or a rebuild through `reload`/`rewrite`) sets it back to `false`. + +### Resorting + +`table.resort()` rebuilds the table so that ascending `id` matches the +configured sort, laying the rows out on disk in that order. It is a full-table +operation: the data is dumped, loaded into a fresh table whose ids are assigned +`1..N` in sort order, and swapped into place through the same machinery `reload` +uses, so the primary key, indexes, constraints, grants and counts/stats +companions are rebuilt, `ANALYZE` is run, and the previous table is kept as a +`name_old` backup. **The ids change** — anything outside psycodict that +stored them as durable identifiers must be updated — and the operation needs +temporary disk space for the replacement, its indexes, the dump and the backup. +By default a table already marked ordered reports that there is nothing to do; +pass `force=True` to rebuild anyway. + +The earlier in-place implementation renumbered every id with an `UPDATE`, which +stalled replication and left the rows in their old physical order (defeating the +point of id-ordering); it was disabled, and is now replaced by this rebuild. + +Because a resort is expensive, it is no longer a side effect of a small write. +`resort=True` on `insert_many`, `update`, `copy_from` or an in-place +`update_from_file` **raises**, pointing at `resort()`; `resort=False` (the +default) is unaffected. The replacement operations that rebuild the whole table +anyway — `reload`, `rewrite`, and a non-inplace `update_from_file` — still +establish id order as part of building the new table. The workflow for a +maintained table is therefore: -### Resorting is disabled +```python +table.update(...) # marks out_of_order +# later, in a maintenance window: +table.resort() # rebuild in sort order +``` -`resort()` is a **no-op on this branch**: it prints `resorting disabled` and returns `None` without touching the table. In-place resorting was found to stall replication and to not persist correctly on disk, and since the tables are effectively read-only in production the supported way to renumber ids is to dump the data in sorted order and `reload` it. Consequently the `resort=` keyword on `insert_many`, `update`, `copy_from`, `reload` and `update_from_file` currently does nothing. +`scripts/audit_id_order.py` checks the flag against reality: it streams each +`id_ordered` table in sort order and reports `OK`, `MISMATCH` or an error +without modifying anything. Use it before trusting `out_of_order = false` on a +table whose history includes non-inplace updates, which could have left the flag +stale. ### Locking diff --git a/psycodict/table.py b/psycodict/table.py index 4c1fa0e..8e1be52 100644 --- a/psycodict/table.py +++ b/psycodict/table.py @@ -1209,17 +1209,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): """ @@ -1243,7 +1250,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, @@ -1269,7 +1276,10 @@ def rewrite( - ``func`` -- a function that takes a record (dictionary) as input and returns the modified record - ``query`` -- a query dictionary; only rows satisfying this query will be changed - - ``resort`` -- whether to resort the table after running the rewrite + - ``resort`` -- whether to resort the table after running the rewrite. + Defaults to resorting a non-inplace (replacement) rewrite and not an + in-place one; ``resort=True`` on an in-place rewrite raises, since + the in-place path cannot resort (call ``resort()`` afterward). - ``restat`` -- whether to recompute statistics after running the rewrite - ``tostr_func`` -- a function to be used when writing data to the temp file defaults to copy_dumps from encoding @@ -1291,6 +1301,12 @@ def rewrite( """ # Fail before the expensive dump below rather than deep inside update_from_file self._forbid_reindex_false(kwds.get("reindex"), kwds.get("inplace")) + if resort is None: + # Default: resort on the non-inplace (replacement) path, as before; + # an in-place rewrite edits rows on the live table and cannot + # resort, so do not ask it to. An explicit resort=True on an + # in-place rewrite still raises, in update_from_file. + resort = not kwds.get("inplace", False) sep = kwds.get("sep", "|") # An unusable separator would otherwise be rejected only by COPY itself, # after func has already been run over every row of the table. @@ -1396,6 +1412,12 @@ def update_from_file( - ``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 @@ -1487,6 +1509,19 @@ def drop_tmp(): Identifier(tmp_table), Identifier(self.search_table), Identifier(label_col))) + # Renumber into sort order before the keys are rebuilt: on the + # non-inplace path the renumber replaces the _tmp table, so it + # must run while that table has no primary key or indexes. The + # in-place path cannot renumber (resort=True was rejected up + # front); if it changed a sort-key column it just records that + # the id order no longer matches. + if not inplace and self._id_ordered and resort: + self._generate_sorted_ids(suffix=suffix) + ordered = True + else: + ordered = False + if inplace and resort and self._id_ordered: + self._break_order() if reindex and inplace: # also restores constraints self.restore_indexes(columns[1:]) @@ -1495,10 +1530,6 @@ def drop_tmp(): self.restore_indexes(suffix=suffix) # We also need to recreate the primary key self.restore_pkeys(suffix=suffix) - if self._id_ordered and resort: - ordered = self.resort(suffix=suffix) - else: - ordered = False if restat and self.stats.saving: if not inplace: for table in [self.stats.counts, self.stats.stats]: @@ -1576,6 +1607,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: @@ -1600,8 +1632,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 @@ -1727,6 +1757,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 = [] @@ -1774,8 +1805,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) @@ -1786,77 +1815,132 @@ def insert_many(self, data, resort=False, reindex=None, restat=True): finally: self._log_db_change("insert_many", aborted=aborted, logid=logid, nrows=len(search_data)) - def resort(self, suffix="", sort=None): + def _generate_sorted_ids(self, suffix=""): + """ + Rebuild ``{search}{suffix}`` so that ascending ``id`` follows the + configured sort, laying the rows out on disk in that order too. + + This is a physical rebuild, not an in-place ``UPDATE`` of every id: the + rows are copied into a fresh table in sort order, with ids drawn in + that order from a new sequence, and that table replaces the original. + So the on-disk order matches the id order -- the point of id-ordering, + which an in-place renumber does not achieve -- and there is no table + bloat or replication stall from rewriting every row. + + Must run *before* the primary key and indexes are (re)built on the + ``{suffix}`` table, since it drops and recreates that table. It is the + internal step reload and non-inplace update_from_file use; the public + entry point is :meth:`resort`. ``self._sort`` must be set. + """ + target = self.search_table + suffix + if self._sort is None: + raise ValueError( + "Cannot resort %s: it has no configured sort" % (self.search_table,) + ) + # Transient, named nowhere else, so shortened to fit rather than left + # for the server to truncate. + resorting = derived_identifier(target, "_resort") + seq = derived_identifier(target, "_resort_seq") + search_cols = SQL(", ").join(Identifier(c) for c in self.search_cols) + # A fresh table with the same columns and storage but no indexes. + self._clone(target, resorting) + try: + self._execute( + SQL("CREATE TEMP SEQUENCE {0} MINVALUE 1 START 1 CACHE 10000").format( + Identifier(seq) + ) + ) + # nextval is evaluated as rows leave the sort, so the ids run + # 1..N in sort order, and the INSERT lays them down in that order. + self._execute( + SQL( + "INSERT INTO {0} (id, {1}) " + "SELECT nextval({2}), {1} FROM {3} ORDER BY {4}, id" + ).format( + Identifier(resorting), + search_cols, + Literal(seq), + Identifier(target), + self._sort, + ) + ) + self._execute(SQL("DROP SEQUENCE {0}").format(Identifier(seq))) + self._execute(SQL("DROP TABLE {0}").format(Identifier(target))) + self._execute( + SQL("ALTER TABLE {0} RENAME TO {1}").format( + Identifier(resorting), Identifier(target) + ) + ) + except Exception: + # Leave no half-built _resort table behind for the next attempt's + # _clone to trip over. + self._execute( + SQL("DROP TABLE IF EXISTS {0}").format(Identifier(resorting)) + ) + raise + + def resort(self, force=False): """ - 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 the table so that ascending ``id`` matches the configured + sort, and lay the rows out on disk in that order. + + This is an explicit, potentially expensive full-table operation, not a + step of an ordinary write. The table is dumped, loaded into a fresh + table whose ids are assigned 1..N in sort order, and swapped into + place, reusing :meth:`reload`'s machinery: the primary key, indexes, + constraints, grant policy and counts/stats companions are rebuilt, + ``ANALYZE`` is run, and the previous table is kept as an ``_oldN`` + backup so the operation can be reverted with ``reload_revert``. + + **The ids change.** Anything outside psycodict that stored these ids + as durable identifiers must be updated. The operation needs temporary + disk space for the replacement table, its indexes, the dump file and + the backup. INPUT: - - ``suffix`` -- a string such as "_tmp" or "_old1" to be appended to the names in the command. - - ``sort`` -- -- a list, either of strings (which are interpreted as column names - in the ascending direction) or of pairs (column name, 1 or -1). - If None, will use ``self._sort_orig``. - """ - - print("resorting disabled") - # resorting without a reload makes replication stall - # and doesn't store data correctly on disk - # Given that our tables are readonly, we should just dump sorted and reload - return None - search_table = Identifier(self.search_table + suffix) - # Transient: created and dropped inside this transaction, and named - # nowhere else, so these are shortened rather than left to the server. - tmp_table = Identifier(derived_identifier(self.search_table + suffix, "_sorter")) - tmp_seq = Identifier( - derived_identifier(self.search_table + suffix, "_sorter_newid_seq") - ) - sort_order = self._sort if sort is None else self._sort_str(sort) - if sort_order is None: - print("resort failed, no sort order given") + - ``force`` -- rebuild even when the table is already marked ordered + (``id_ordered`` and not ``out_of_order``). By default that case + reports that there is nothing to do and returns without work. + + OUTPUT: + + ``True`` if the table was rebuilt, ``False`` if it was already ordered + and ``force`` was not set. + """ + if not self._id_ordered: + raise ValueError( + "%s is not id_ordered; call set_sort() to give it a sort before " + "resorting" % (self.search_table,) + ) + if self._sort is None: + raise ValueError( + "%s has no configured sort to order by" % (self.search_table,) + ) + if not self._out_of_order and not force: + print( + "%s is already ordered; pass force=True to rebuild anyway" + % (self.search_table,) + ) return False - logid = self._check_locks("resort", suffix=suffix) + logid = self._check_locks("resort") aborted = True + # Dumped without ids, then reloaded: reload clones a fresh table, + # loads the rows, calls _generate_sorted_ids to renumber them in sort + # order, and swaps -- so grants, indexes, the backup and ANALYZE all + # come from the one replacement path rather than being duplicated here. + datafile = tempfile.NamedTemporaryFile("w", delete=False) + datafile.close() try: - with DelayCommit(self, silence=True): - if (self._id_ordered and self._out_of_order) or suffix: - now = time.time() - # we will use a temporary table to avoid ACCESS EXCLUSIVE lock - self._execute(SQL( - "CREATE TEMP SEQUENCE {0} MINVALUE 0 START 0 CACHE 10000" - ).format(tmp_seq)) - - id_type = column_type_sql(self.col_type["id"]) - self._execute(SQL( - "CREATE TEMP TABLE {0} (oldid {2}, newid {3} NOT NULL DEFAULT nextval('{1}')) ON COMMIT DROP" - ).format(tmp_table, tmp_seq, id_type, id_type)) - - self._execute(SQL( - "ALTER SEQUENCE {0} OWNED BY {1}.newid" - ).format(tmp_seq, tmp_table)) - - self._execute(SQL( - "INSERT INTO {0} " - "SELECT id as oldid FROM {1} ORDER BY {2}" - ).format(tmp_table, search_table, sort_order)) - self.drop_pkeys(suffix=suffix) - self._execute(SQL( - "UPDATE {0} SET id = {1}.newid " - "FROM {1} WHERE {0}.id = {1}.oldid" - ).format(search_table, tmp_table)) - self.restore_pkeys(suffix=suffix) - if not suffix: - self._set_ordered() - print("Resorted %s in %.3f secs" % (self.search_table, time.time() - now)) - elif self._id_ordered and not self._out_of_order: - print(f"Table {self.search_table} already sorted") - else: # not self._id_ordered - print("Data does not have an id column to be sorted") + now = time.time() + self.copy_to(datafile.name, include_id=False) + self.reload(datafile.name, resort=True, final_swap=True) + print("Resorted %s in %.3f secs" % (self.search_table, time.time() - now)) aborted = False return True finally: - self._log_db_change("resort", logid=logid, aborted=aborted, sort_order=sort_order) + os.unlink(datafile.name) + self._log_db_change("resort", logid=logid, aborted=aborted) def _set_ordered(self): """ @@ -2252,18 +2336,9 @@ def reload( % (table, time.time() - now, filename) ) - self.restore_pkeys(suffix=suffix) - - # update the indexes - # these are needed before restoring indexes - if indexesfile is not None: - # we do the swap at the end - self.reload_indexes(indexesfile, sep=sep) - if constraintsfile is not None: - self.reload_constraints(constraintsfile, sep=sep) - # Also restores constraints - self.restore_indexes(suffix=suffix) - + # Renumber ids in sort order before the keys are built: the + # renumber replaces the _tmp table, so it must run while that + # table still has no primary key or indexes to rebuild. if resort: if metafile: # read the metafile @@ -2300,11 +2375,24 @@ def reload( else: if not self._id_ordered: # this table doesn't need to be sorted resort = False - # tracks the success of resort - ordered = self.resort(suffix=suffix) + if resort: + self._generate_sorted_ids(suffix=suffix) + ordered = True else: ordered = False + self.restore_pkeys(suffix=suffix) + + # update the indexes + # these are needed before restoring indexes + if indexesfile is not None: + # we do the swap at the end + self.reload_indexes(indexesfile, sep=sep) + if constraintsfile is not None: + self.reload_constraints(constraintsfile, sep=sep) + # Also restores constraints + self.restore_indexes(suffix=suffix) + # Ensure stats/counts tables are backed up and new empty ones created if self.stats.saving: for table in [self.stats.counts, self.stats.stats]: @@ -2723,7 +2811,15 @@ def _staged_enter(self): def update_from_file(datafile, label_col=None, inplace=True, **kwds): if not inplace: raise ValueError("update_from_file on a staged table is always performed in place") - return unstaged_update_from_file(datafile, label_col=label_col, inplace=True, **kwds) + # Writes to the staged copy are in place on that copy, which is + # not the live table; resorting it inline would defeat the + # point of staging. Drop any resort request rather than letting + # the in-place guard reject it -- the caller resorts the live + # table after the commit, with resort(). + kwds.pop("resort", None) + return unstaged_update_from_file( + datafile, label_col=label_col, inplace=True, resort=False, **kwds + ) staged.update_from_file = update_from_file # With reindex set, insert_many drops the primary key around the @@ -2953,6 +3049,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 @@ -2973,8 +3070,6 @@ 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() diff --git a/scripts/audit_id_order.py b/scripts/audit_id_order.py new file mode 100644 index 0000000..c6b65e5 --- /dev/null +++ b/scripts/audit_id_order.py @@ -0,0 +1,125 @@ +#!/usr/bin/env python3 +# -*- coding: utf-8 -*- +""" +Check whether the ``id_ordered`` tables really are ordered. + +A table marked ``id_ordered`` with ``out_of_order = false`` asserts that +ascending ``id`` is the configured default sort. Search code relies on that: +it may replace ``ORDER BY `` with ``ORDER BY id``. Historically the flag +could drift -- a non-inplace ``update_from_file`` left it stale -- so before +trusting it, audit it. + +This is read-only. For each selected table it streams the rows in configured +sort order (with ``id`` as the final tie-breaker) and checks that the ids come +out strictly increasing. Nothing is written. + +Usage:: + + python scripts/audit_id_order.py --all + python scripts/audit_id_order.py TABLE [TABLE ...] + +Exit status is 0 when every audited table is OK, 1 when any is out of order or +errored, 2 for a usage problem. + +The audit reads every row in sort order, which for a large table is a +substantial database sort; schedule it accordingly. Rows are streamed from the +server (a named cursor), so the client stays at constant memory however large +the table. +""" +import argparse +import sys + +from psycopg.sql import SQL, Identifier + + +def _server_side_rows(table): + """ + Stream ``(id,)`` for every row of ``table`` in configured-sort order plus + ``id`` as the final tie-breaker, using a named cursor so the whole table is + never materialized in the client. + """ + db = table._db + order = table._sort # an SQL fragment, or None + if order is None: + clause = SQL("ORDER BY id") + else: + clause = SQL("ORDER BY {0}, id").format(order) + query = SQL("SELECT id FROM {0} {1}").format(Identifier(table.search_table), clause) + # A server-side (named) cursor keeps the sort on the server and hands back + # rows in batches; see PostgresBase._cursor(buffered=True). + cur = db._cursor(buffered=True) + try: + cur.execute(query) + while True: + batch = cur.fetchmany(10000) + if not batch: + break + for row in batch: + yield row[0] + finally: + cur.close() + + +def audit_table(db, name): + """ + Return ``("OK", n)``, ``("MISMATCH", detail)`` or ``("ERROR", message)`` + for one table, reading it in sort order without modifying anything. + """ + if name not in db.tablenames: + return ("ERROR", "no such table") + table = db[name] + if not table._id_ordered: + return ("ERROR", "not id_ordered") + prev = None + count = 0 + try: + for rid in _server_side_rows(table): + if prev is not None and rid <= prev: + return ( + "MISMATCH", + "id %s follows id %s in sort order (row %s)" % (rid, prev, count + 1), + ) + prev = rid + count += 1 + except Exception as err: # pragma: no cover - surfaced to the operator + return ("ERROR", "%s: %s" % (type(err).__name__, err)) + return ("OK", count) + + +def main(argv=None): + parser = argparse.ArgumentParser(description=__doc__.splitlines()[1]) + group = parser.add_mutually_exclusive_group(required=True) + group.add_argument( + "--all", action="store_true", help="audit every id_ordered table" + ) + group.add_argument("tables", nargs="*", default=[], help="tables to audit") + args = parser.parse_args(argv) + + from psycodict import db + + if args.all: + names = sorted( + name for name in db.tablenames if db[name]._id_ordered + ) + if not names: + print("No id_ordered tables.") + return 0 + else: + names = args.tables + + worst = 0 + for name in names: + status, detail = audit_table(db, name) + if status == "OK": + print("OK %-40s %s rows" % (name, detail)) + elif status == "MISMATCH": + print("MISMATCH %-40s %s" % (name, detail)) + worst = max(worst, 1) + else: + print("ERROR %-40s %s" % (name, detail)) + worst = max(worst, 1) + return worst + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/test_id_order_audit.py b/tests/test_id_order_audit.py new file mode 100644 index 0000000..9d2dcdf --- /dev/null +++ b/tests/test_id_order_audit.py @@ -0,0 +1,90 @@ +# -*- coding: utf-8 -*- +""" +The read-only id-order audit and the write-path invariants it protects. + +``scripts/audit_id_order.py`` walks each id_ordered table in sort order and +checks the ids come out increasing. It exists because the flag can drift, so +the audit -- not the flag -- is the ground truth during the one-time +production check. +""" +import importlib.util +import pathlib + +import pytest + +from psycopg.sql import SQL, Identifier + +from conftest import sample_row + + +# Load the script as a module, since scripts/ is not a package. +_AUDIT_PATH = pathlib.Path(__file__).resolve().parent.parent / "scripts" / "audit_id_order.py" +_spec = importlib.util.spec_from_file_location("audit_id_order", _AUDIT_PATH) +audit = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(audit) + + +@pytest.fixture +def ordered_table(table_factory): + table = table_factory(sort=["n"]) + table.insert_many([sample_row(i) for i in range(20)]) + table.resort() + return table + + +def test_audit_reports_ok_for_an_ordered_table(db, ordered_table): + status, detail = audit.audit_table(db, ordered_table.search_table) + assert status == "OK" + assert detail == 20 + + +def test_audit_reads_nothing_but_reports_the_row_count(db, ordered_table): + status, detail = audit.audit_table(db, ordered_table.search_table) + assert status == "OK" and detail == ordered_table.count() + + +def test_audit_catches_a_table_whose_ids_do_not_match_the_sort(db, table_factory): + """ + A table that claims to be ordered but whose ids run against the sort must + be caught, even though its flag says out_of_order = false. + """ + table = table_factory(sort=["n"]) + table.insert_many([sample_row(i) for i in range(10)]) + # Reverse the ids against n, then lie in the metadata: ordered, not stale. + table._execute( + SQL("UPDATE {0} SET id = 100 - n").format(Identifier(table.search_table)) + ) + table._execute( + SQL("UPDATE meta_tables SET id_ordered = true, out_of_order = false WHERE name = %s"), + [table.search_table], + ) + table._db.conn.commit() + + status, detail = audit.audit_table(db, table.search_table) + assert status == "MISMATCH" + + +def test_audit_errors_on_a_non_ordered_table(db, table_factory): + table = table_factory(sort=["n"], id_ordered=False) + table.insert_many([sample_row(i) for i in range(3)]) + status, detail = audit.audit_table(db, table.search_table) + assert status == "ERROR" + assert "id_ordered" in detail + + +def test_audit_errors_on_a_missing_table(db): + status, detail = audit.audit_table(db, "no_such_table_xyz") + assert status == "ERROR" + + +def test_audit_does_not_modify_the_database(db, ordered_table): + before = [rec["id"] for rec in ordered_table.search({}, ["id"], sort=[["id", 1]])] + audit.audit_table(db, ordered_table.search_table) + after = [rec["id"] for rec in ordered_table.search({}, ["id"], sort=[["id", 1]])] + assert before == after + + +def test_audit_on_an_empty_ordered_table(db, table_factory): + table = table_factory(sort=["n"]) + status, detail = audit.audit_table(db, table.search_table) + assert status == "OK" and detail == 0 diff --git a/tests/test_resort.py b/tests/test_resort.py new file mode 100644 index 0000000..aa3c9c0 --- /dev/null +++ b/tests/test_resort.py @@ -0,0 +1,175 @@ +# -*- coding: utf-8 -*- +""" +``resort()`` rebuilds the table so ascending id follows the configured sort. + +Before rc3 it was a disabled no-op (``print("resorting disabled"); return None``) +because the old implementation renumbered every id in place, which stalls +replication and does not lay the rows out on disk in order. It is now a +physical rebuild through reload's machinery, and the row-level writes that used +to accept a ``resort=`` flag reject ``resort=True`` and point at it. +""" +import pytest + +from psycopg.sql import SQL, Identifier + +from conftest import sample_row + + +def ids_in_disk_order(table): + """ + The ids in physical (heap) order -- ``ctid`` follows the on-disk layout. + A resorted table must have these ascending, not just the logical ids. + """ + cur = table._execute( + SQL("SELECT id FROM {0} ORDER BY ctid").format(Identifier(table.search_table)) + ) + return [rec[0] for rec in cur] + + +@pytest.fixture +def shuffled_table(table_factory): + """ + An id_ordered table sorted by ``n`` whose rows were inserted in an order + that does *not* match ``n``, so its ids do not follow the sort. + """ + table = table_factory(sort=["n"]) + order = [3, 0, 4, 1, 2, 7, 5, 6, 9, 8] + table.insert_many([sample_row(i) for i in order]) + table.stats.saving = True + return table + + +def test_resort_puts_ids_in_sort_order(shuffled_table): + table = shuffled_table + # before: id order does not match n order + by_id = [rec["n"] for rec in table.search({}, ["n"], sort=[["id", 1]])] + assert by_id != sorted(by_id) + + assert table.resort() is True + + by_id = [rec["n"] for rec in table.search({}, ["n"], sort=[["id", 1]])] + assert by_id == sorted(by_id) + + +def test_resort_assigns_contiguous_ids_from_one(shuffled_table): + shuffled_table.resort() + ids = sorted(rec["id"] for rec in shuffled_table.search({}, ["id"])) + assert ids == list(range(1, len(ids) + 1)) + + +def test_resort_lays_rows_out_on_disk_in_order(shuffled_table): + """ + The point of id-ordering is sequential disk reads: an in-place renumber + would leave the heap order untouched. A rebuild must not. + """ + shuffled_table.resort() + assert ids_in_disk_order(shuffled_table) == sorted( + ids_in_disk_order(shuffled_table) + ) + + +def test_resort_marks_the_table_ordered(shuffled_table): + shuffled_table._break_order() + assert shuffled_table._out_of_order is True + shuffled_table.resort() + assert shuffled_table._id_ordered is True + assert shuffled_table._out_of_order is False + # and persisted + cur = shuffled_table._execute( + SQL("SELECT id_ordered, out_of_order FROM meta_tables WHERE name = %s"), + [shuffled_table.search_table], + ) + assert cur.fetchone() == (True, False) + + +def test_resort_preserves_the_data(shuffled_table): + before = {rec["n"]: rec["label"] for rec in shuffled_table.search({}, ["n", "label"])} + shuffled_table.resort() + after = {rec["n"]: rec["label"] for rec in shuffled_table.search({}, ["n", "label"])} + assert before == after + assert shuffled_table.count() == len(before) + + +def test_resort_keeps_a_backup(shuffled_table): + shuffled_table.resort() + # reload keeps the previous table as _old1 + assert shuffled_table._table_exists(shuffled_table.search_table + "_old1") + + +def test_resort_rebuilds_indexes(shuffled_table): + shuffled_table.resort() + built = set(shuffled_table._list_built_indexes()) + assert shuffled_table.search_table + "_pkey" in built + + +def test_resort_reports_nothing_to_do_when_already_ordered(shuffled_table): + shuffled_table.resort() + # a second resort with the table already ordered does nothing + assert shuffled_table.resort() is False + + +def test_resort_force_rebuilds_even_when_ordered(shuffled_table): + shuffled_table.resort() + assert shuffled_table.resort(force=True) is True + + +def test_resort_requires_id_ordered(table_factory): + table = table_factory(sort=["n"], id_ordered=False) + table.insert_many([sample_row(i) for i in range(5)]) + with pytest.raises(ValueError, match="id_ordered"): + table.resort() + + +def test_resort_requires_a_sort(table_factory): + table = table_factory(sort=["n"]) + table.set_sort(None) + table.insert_many([sample_row(i) for i in range(5)]) + with pytest.raises(ValueError, match="id_ordered|sort"): + table.resort() + + +def test_resort_handles_a_descending_sort(table_factory): + table = table_factory(sort=[("n", -1)]) + table.insert_many([sample_row(i) for i in [2, 0, 4, 1, 3]]) + table.resort() + by_id = [rec["n"] for rec in table.search({}, ["n"], sort=[["id", 1]])] + assert by_id == sorted(by_id, reverse=True) + + +def test_resort_on_an_empty_table(table_factory): + table = table_factory(sort=["n"]) + table._break_order() + assert table.resort() is True + assert table.count() == 0 + + +def test_resort_does_not_recurse_through_reload(shuffled_table, monkeypatch): + """ + resort() reuses reload, and reload must renumber via the private helper, + not by calling public resort() again. + """ + calls = {"resort": 0} + original = type(shuffled_table).resort + + def counting(self, *a, **k): + calls["resort"] += 1 + return original(self, *a, **k) + + monkeypatch.setattr(type(shuffled_table), "resort", counting) + shuffled_table.resort() + assert calls["resort"] == 1 # the outer call only; reload did not re-enter + + +# --------------------------------------------------------------------------- +# reload still establishes order through the rebuild helper +# --------------------------------------------------------------------------- + +def test_reload_orders_a_no_id_file(table_factory, tmp_path): + table = table_factory(sort=["n"]) + table.insert_many([sample_row(i) for i in [4, 1, 3, 0, 2]]) + dump = tmp_path / "d.txt" + table.copy_to(str(dump), include_id=False) + table.reload(str(dump), resort=True) + by_id = [rec["n"] for rec in table.search({}, ["n"], sort=[["id", 1]])] + assert by_id == sorted(by_id) + assert table._out_of_order is False diff --git a/tests/test_write.py b/tests/test_write.py index 129ffbb..ffb03df 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 @@ -559,11 +577,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 +645,6 @@ def test_drop_column_validates_its_argument(filled_table): filled_table.drop_column("nosuchcol", force=True) -def test_finalize_changes_is_a_noop(filled_table): - assert filled_table.finalize_changes() is None - assert _num_rows(filled_table) == 200 - - ################################################################## # transactions # ################################################################## diff --git a/tests/test_write_invariants.py b/tests/test_write_invariants.py new file mode 100644 index 0000000..b8719ab --- /dev/null +++ b/tests/test_write_invariants.py @@ -0,0 +1,153 @@ +# -*- coding: utf-8 -*- +""" +The meta_tables flags mean what B0 of the rc3 review says they mean, after +every supported write. + +- ``total`` is the exact live row count, independent of stat saving. +- ``out_of_order`` records whether ascending id still matches the sort. +- ``stats_valid`` records whether the cached statistics agree with the data. + +These are checked directly against meta_tables (not just the Python object), so +a write that updates one but forgets the other is caught. +""" +import pytest + +from psycopg.sql import SQL + +from conftest import sample_row + + +def flags(table): + cur = table._execute( + SQL( + "SELECT total, out_of_order, stats_valid FROM meta_tables WHERE name = %s" + ), + [table.search_table], + ) + total, out_of_order, stats_valid = cur.fetchone() + return {"total": total, "out_of_order": out_of_order, "stats_valid": stats_valid} + + +@pytest.fixture +def ordered(table_factory): + """An ordered, stats-valid table of 100 rows sorted by n.""" + table = table_factory(sort=["n"]) + table.insert_many([sample_row(i) for i in range(100)]) + table.stats.saving = True + table.resort() + table.stats.refresh_stats() + assert flags(table)["total"] == 100 + return table + + +# --------------------------------------------------------------------------- +# total is exact after every operation +# --------------------------------------------------------------------------- + +def test_insert_many_keeps_total_exact(ordered): + ordered.insert_many([sample_row(1000), sample_row(1001)], restat=False) + assert flags(ordered)["total"] == 102 + assert ordered.count() == 102 + + +def test_delete_keeps_total_exact(ordered): + ordered.delete({"n": {"$lt": 10}}, restat=False) + assert flags(ordered)["total"] == 90 + assert ordered.count() == 90 + + +def test_upsert_insert_keeps_total_exact(ordered): + ordered.upsert({"label": "brand_new"}, {"n": 5000}) + assert flags(ordered)["total"] == 101 + + +def test_upsert_update_does_not_change_total(ordered): + ordered.upsert({"label": "l5"}, {"n": 5000}) + assert flags(ordered)["total"] == 100 + + +def test_copy_from_keeps_total_exact(ordered, table_factory, tmp_path): + source = table_factory(sort=["n"]) + source.insert_many([sample_row(i) for i in range(200, 205)]) + dump = tmp_path / "d.txt" + source.copy_to(str(dump), include_id=False) + ordered.copy_from(str(dump), restat=False) + assert flags(ordered)["total"] == 105 + + +def test_total_is_exact_without_stat_saving(table_factory): + """ + total is maintained even when the stats object is not saving custom counts. + """ + table = table_factory(sort=["n"]) + assert table.stats.saving is False + table.insert_many([sample_row(i) for i in range(7)]) + assert flags(table)["total"] == 7 + table.delete({"n": 0}) + assert flags(table)["total"] == 6 + + +def test_reload_recounts_total(ordered, tmp_path): + dump = tmp_path / "d.txt" + ordered.copy_to(str(dump), include_id=False) + # dump holds 100 rows; reload replaces the table with them + ordered.reload(str(dump)) + assert flags(ordered)["total"] == 100 + + +# --------------------------------------------------------------------------- +# order-flag transitions, per B0's rules +# --------------------------------------------------------------------------- + +def test_delete_preserves_order(ordered): + ordered.delete({"n": {"$lt": 5}}) + assert flags(ordered)["out_of_order"] is False + + +def test_insert_many_breaks_order(ordered): + ordered.insert_many([sample_row(1000)], restat=False) + assert flags(ordered)["out_of_order"] is True + + +def test_update_of_a_non_sort_column_still_marks_out_of_order(ordered): + """ + update() is conservative: it marks order broken on any update. (B0 notes + this is safe if pessimistic; the important half is that it is never + falsely left ordered.) + """ + ordered.update({"n": 5}, {"label": "changed"}, restat=False) + assert flags(ordered)["out_of_order"] is True + + +def test_resort_restores_order(ordered): + ordered.insert_many([sample_row(1000)], restat=False) + assert flags(ordered)["out_of_order"] is True + ordered.resort() + assert flags(ordered)["out_of_order"] is False + + +def test_inplace_update_of_a_sort_key_breaks_order(ordered, tmp_path): + """ + B0: an in-place update_from_file that changes a sort key must set + out_of_order, since it did not rebuild. + """ + dump = tmp_path / "u.txt" + with open(dump, "w") as F: + F.write("label|n\ntext|integer\n\nl5|9999\n") + ordered.update_from_file(str(dump), inplace=True, restat=False) + assert flags(ordered)["out_of_order"] is True + + +# --------------------------------------------------------------------------- +# stats_valid transitions +# --------------------------------------------------------------------------- + +def test_writes_invalidate_stats(ordered): + ordered.insert_many([sample_row(1000)], restat=False) + assert flags(ordered)["stats_valid"] is False + + +def test_refresh_stats_revalidates(ordered): + ordered.insert_many([sample_row(1000)], restat=False) + ordered.stats.refresh_stats() + assert flags(ordered)["stats_valid"] is True From 3b2ed1946167633afea17f6e088939b06fb90690 Mon Sep 17 00:00:00 2001 From: David Roe Date: Wed, 5 Aug 2026 00:13:32 -0400 Subject: [PATCH 5/8] Packaging, generic defaults, and an export-format marker Track E: psycopg becomes a plain dependency, so `pip install psycodict` gets a working package (pure-Python driver on the system libpq). pgbinary adds the bundled binary build and pgc a locally compiled one; the pgsource extra is removed, since plain install replaces it, and a <4 ceiling keeps an unreviewed driver major out of a 1.x environment. CI's "import must fail without psycopg" step is inverted to "plain install must import", and both smoke installs now assert importlib.metadata.version == __version__ and run pip check. Track F: search-data export files gain an optional `# psycodict-export-format: N` marker. One shared reader (_read_header_lines) accepts a marked file, accepts an unmarked file as format 0 so every older export still loads, and refuses a version it does not understand before loading any data; the writers (copy_to via _write_header_lines, and rewrite's inline header) emit the current marker. This is the data-file format, kept distinct from the meta_* metadata format; Versioning.md states the realistic promise and decouples both format numbers from the package major. Track G: the default database name is the generic `postgres` rather than `lmfdb` (LMFDB names its own), and the unused `secretsfile` argument to PostgresDatabase is removed -- no caller in LMFDB or seminars passes it. Track H: MANIFEST.in makes the sdist a complete, testable checkout (tests with conftest.py, scripts, guides, metadata), and CI unpacks it and runs its database-free tests so a dropped file fails the build. The release workflow's third-party actions are pinned to commit SHAs. --- .github/workflows/ci.yml | 61 ++++++++++++------- .github/workflows/release.yml | 12 ++-- CHANGELOG.md | 27 +++++++++ DataManagement.md | 3 +- MANIFEST.in | 30 ++++++++++ README.md | 19 +++--- Versioning.md | 20 +++++-- config.ini.example | 2 +- psycodict/base.py | 51 ++++++++++++++-- psycodict/config.py | 5 +- psycodict/database.py | 2 +- psycodict/table.py | 11 +++- pyproject.toml | 20 +++++-- tests/test_export_format.py | 108 ++++++++++++++++++++++++++++++++++ tests/test_security.py | 7 ++- tests/test_write.py | 21 ++++--- 16 files changed, 330 insertions(+), 69 deletions(-) create mode 100644 MANIFEST.in create mode 100644 tests/test_export_format.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f0d6ae3..be22490 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -80,7 +80,7 @@ jobs: # supported pair, the newest pair, and the combination LMFDB actually # deploys. Add a row here when a new PostgreSQL major is released. # The extra oldest-pair row pins psycopg to the declared minimum - # (pyproject's pgbinary/pgsource floor), so the floor stays honest. + # (pyproject's pgbinary floor), so the floor stays honest. matrix: include: - {python: "3.9", postgres: "13"} @@ -162,7 +162,7 @@ jobs: # installed -- which made this check pass against a wheel with its # modules deleted. Asserting where the import came from is what makes # the test about the wheel rather than about the source tree. - - name: Wheel installs and imports + - name: Wheel installs and imports with the binary extra run: | python -m venv "$RUNNER_TEMP/smoke" "$RUNNER_TEMP/smoke/bin/pip" install "$(echo "$PWD"/dist/*.whl)[pgbinary]" @@ -178,32 +178,49 @@ jobs: "imported the checkout rather than the installed wheel: %s" % path ) assert PostgresDatabase is not None and Json is not None + assert importlib.metadata.version("psycodict") == psycodict.__version__ print("psycodict", importlib.metadata.version("psycodict"), "imports from", path) PY + "$RUNNER_TEMP/smoke/bin/pip" check - # psycopg is deliberately an optional dependency, so that users choose - # between the pure-Python and binary builds. Importing without it must - # fail with the guidance in psycodict/__init__.py rather than a bare - # ImportError -- this asserts that contract holds. - - name: Import without psycopg gives a helpful message + # psycopg is a plain dependency now, so a bare ``pip install psycodict`` + # already pulls the pure-Python driver: a plain install must import, not + # fail. (A binary build is still available as the pgbinary extra above.) + - name: Wheel installs and imports without any extra run: | - python -m venv "$RUNNER_TEMP/bare" - "$RUNNER_TEMP/bare/bin/pip" install "$(echo "$PWD"/dist/*.whl)" + python -m venv "$RUNNER_TEMP/plain" + "$RUNNER_TEMP/plain/bin/pip" install "$(echo "$PWD"/dist/*.whl)" # Outside the checkout, for the same reason as the step above. cd "$RUNNER_TEMP" - set +e - output=$(bare/bin/python -c "import psycodict" 2>&1) - status=$? - set -e - echo "$output" - if [ $status -eq 0 ]; then - echo "::error::importing psycodict without psycopg should fail" - exit 1 - fi - case "$output" in - *psycopg\[binary\]*) echo "helpful message present" ;; - *) echo "::error::missing the install-psycopg hint"; exit 1 ;; - esac + plain/bin/python - < EXPORT_FORMAT: + raise ValueError( + "This file is psycodict export format %s, but this psycodict " + "understands only up to format %s; upgrade psycodict to read " + "it" % (version, EXPORT_FORMAT) + ) + names_line = F.readline() + else: + # No marker: format 0, and this first line is the column names. + names_line = first + names = [x.strip() for x in names_line.strip().split(sep)] types = [x.strip() for x in F.readline().strip().split(sep)] blank = F.readline() if blank.strip(): diff --git a/psycodict/config.py b/psycodict/config.py index 9b9320c..c00cb82 100644 --- a/psycodict/config.py +++ b/psycodict/config.py @@ -196,7 +196,10 @@ def __init__(self, parser=None, defaults={}, writeargstofile=False, readargs=Fal dest="postgresql_dbname", metavar="DBNAME", help="PostgreSQL database name [default: %(default)s]", - default=defaults.get("postgresql_dbname", "lmfdb"), + # A generic default, not an LMFDB one: "postgres" is the + # database libpq itself falls back to. Deployments name their + # own database in the config file or constructor. + default=defaults.get("postgresql_dbname", "postgres"), ) def sec_opt(key): diff --git a/psycodict/database.py b/psycodict/database.py index c1703cb..92832c4 100644 --- a/psycodict/database.py +++ b/psycodict/database.py @@ -527,7 +527,7 @@ def _register_object(self, obj): obj.conn = self.conn self._objects.append(obj) - def __init__(self, config=None, secretsfile=None, create=False, upgrade=False, + 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 diff --git a/psycodict/table.py b/psycodict/table.py index 8e1be52..2cdd9c4 100644 --- a/psycodict/table.py +++ b/psycodict/table.py @@ -22,7 +22,7 @@ from psycopg.sql import SQL, Identifier, Placeholder, Literal from .encoding import Json, check_copy_sep, copy_dumps -from .base import PostgresBase, _meta_table_name +from .base import PostgresBase, _meta_table_name, export_format_line from .utils import DelayCommit, IdentifierWrapper, LockError from .base import ( _meta_cols_types_jsonb_idx, @@ -1338,7 +1338,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) @@ -1966,7 +1968,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"): """ 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/tests/test_export_format.py b/tests/test_export_format.py new file mode 100644 index 0000000..d41e3a2 --- /dev/null +++ b/tests/test_export_format.py @@ -0,0 +1,108 @@ +# -*- coding: utf-8 -*- +""" +Search-data files carry an optional format marker. + +A new file begins with ``# psycodict-export-format: N``; a file without one is +format 0, the historical layout, so every file psycodict has ever written still +reads. A version this psycodict does not understand is refused before any data +is loaded. This is a property of the *data file*, distinct from the metadata +format of the ``meta_*`` tables. +""" +import pytest + +from psycodict.base import EXPORT_FORMAT, EXPORT_FORMAT_MARKER, export_format_line + + +def read(path): + with open(path) as F: + return F.read() + + +def test_a_new_export_starts_with_the_marker(filled_table, tmp_path): + searchfile = str(tmp_path / "s.txt") + filled_table.copy_to(searchfile) + first = read(searchfile).split("\n")[0] + assert first == "%s %s" % (EXPORT_FORMAT_MARKER, EXPORT_FORMAT) + + +def test_a_marked_file_round_trips(filled_table, table_factory, tmp_path): + searchfile = str(tmp_path / "s.txt") + filled_table.copy_to(searchfile) + assert export_format_line() in read(searchfile) + target = table_factory() + target.copy_from(searchfile) + assert target.count() == filled_table.count() + assert {r["n"] for r in target.search({}, ["n"])} == set(range(200)) + + +def test_a_legacy_unmarked_file_still_loads(filled_table, table_factory, tmp_path): + """ + A format-0 file -- no marker, just names/types/blank/data -- is what every + older export looks like, and must keep loading. + """ + marked = str(tmp_path / "marked.txt") + filled_table.copy_to(marked, include_id=False) + # strip the marker line to reproduce a format-0 file byte-for-byte + body = read(marked).split("\n", 1)[1] + assert read(marked).startswith(EXPORT_FORMAT_MARKER) + legacy = tmp_path / "legacy.txt" + legacy.write_text(body) + assert not read(str(legacy)).startswith(EXPORT_FORMAT_MARKER) + + target = table_factory() + target.copy_from(str(legacy)) + assert target.count() == 200 + + +def test_a_future_format_is_refused_before_loading(filled_table, table_factory, tmp_path): + """ + A file claiming a newer format than this psycodict understands is rejected + with a clear message, and nothing is loaded. + """ + searchfile = filled_table_dump(filled_table, tmp_path) + lines = read(searchfile).split("\n") + lines[0] = "%s %s" % (EXPORT_FORMAT_MARKER, EXPORT_FORMAT + 5) + with open(searchfile, "w") as F: + F.write("\n".join(lines)) + + target = table_factory() + with pytest.raises(ValueError, match="export format"): + target.copy_from(searchfile) + assert target.count() == 0 + + +def test_a_malformed_marker_is_rejected(filled_table, table_factory, tmp_path): + searchfile = filled_table_dump(filled_table, tmp_path) + lines = read(searchfile).split("\n") + lines[0] = "%s banana" % EXPORT_FORMAT_MARKER + with open(searchfile, "w") as F: + F.write("\n".join(lines)) + target = table_factory() + with pytest.raises(ValueError, match="[Mm]arker"): + target.copy_from(searchfile) + + +def test_reload_reads_a_marked_file(filled_table, tmp_path): + searchfile = str(tmp_path / "s.txt") + filled_table.copy_to(searchfile, include_id=False) + filled_table.reload(searchfile) + assert filled_table.count() == 200 + + +def test_create_table_from_header_reads_the_marker(db, filled_table, tmp_path): + """ + adjust_schema builds a table from a file's header; that path reads through + the same header reader, so the marker is handled there too. + """ + folder = tmp_path / "data" + db.copy_to([filled_table.search_table], str(folder)) + searchfile = folder / (filled_table.search_table + ".txt") + assert read(str(searchfile)).startswith(EXPORT_FORMAT_MARKER) + + +# --- helpers --------------------------------------------------------------- + +def filled_table_dump(table, tmp_path): + searchfile = str(tmp_path / "s.txt") + table.copy_to(searchfile, include_id=False) + return searchfile diff --git a/tests/test_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_write.py b/tests/test_write.py index ffb03df..77ba3d0 100644 --- a/tests/test_write.py +++ b/tests/test_write.py @@ -328,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): @@ -364,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"] @@ -411,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): @@ -423,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)) From 756c3fa7e69add05b259d0cc2867d702eb8445a4 Mon Sep 17 00:00:00 2001 From: David Roe Date: Wed, 5 Aug 2026 00:55:03 -0400 Subject: [PATCH 6/8] Define the public API with __all__ and freeze it The versioning policy said "documented non-underscore names are public", while Sphinx ran with undoc-members, so any helper that happened to have a docstring became part of the permanent 1.x surface. Each module now declares __all__ -- the curated set of names psycodict actually promises -- and the API reference documents exactly those (undoc-members is off). A non-__all__ name, docstring or not, is implementation: still importable, so nothing downstream breaks, but not promised. tests/test_public_api.py freezes every module's __all__, checks each exported name resolves, checks `from psycodict import *` binds exactly the root set, and checks the specific names LMFDB and seminars import still resolve -- including private ones (seminars' _counts_cols, _meta_*_cols) and a couple kept importable but unpromised (range_formatter, KeyedDefaultDict), since __all__ governs `import *` and the docs, not explicit imports. Versioning.md is rewritten around this: public = the __all__ names and their documented behavior; db[name] is the canonical table lookup while db. is convenience a real database attribute wins over, so adding a method in a minor release never makes a table unreachable through db[name]. --- CHANGELOG.md | 11 +++++ Versioning.md | 35 ++++++++++----- docs/conf.py | 5 ++- psycodict/__init__.py | 14 ++++++ psycodict/base.py | 7 +++ psycodict/config.py | 7 +++ psycodict/database.py | 7 +++ psycodict/dbdiff.py | 8 ++++ psycodict/encoding.py | 9 ++++ psycodict/grants.py | 8 ++++ psycodict/notifications.py | 7 +++ psycodict/searchtable.py | 7 +++ psycodict/slowlog.py | 9 ++++ psycodict/statstable.py | 7 +++ psycodict/table.py | 7 +++ psycodict/utils.py | 10 +++++ psycodict/validation.py | 8 ++++ tests/test_public_api.py | 90 ++++++++++++++++++++++++++++++++++++++ 18 files changed, 244 insertions(+), 12 deletions(-) create mode 100644 tests/test_public_api.py diff --git a/CHANGELOG.md b/CHANGELOG.md index fdb4201..e3967e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -464,6 +464,17 @@ hardening standalone use; the highlights: install smoke tests assert `importlib.metadata.version == __version__` and run `pip check` for both a plain and a binary-extra install. +- **The public API is defined by `__all__`.** Each module now declares the + names psycodict promises to keep across 1.x, and the API reference documents + exactly those (Sphinx `undoc-members` is off). A non-`__all__` name -- even one + with a docstring -- is implementation: still importable, so nothing downstream + breaks, but not part of the stability promise. `tests/test_public_api.py` + freezes the surface so a change to it is deliberate. Versioning.md is rewritten + around this, and states that `db[name]` is the canonical table lookup while + `db.` is convenience syntax a real database attribute wins over -- so + adding a method in a minor release never makes a table unreachable through + `db[name]`. + ### Release candidates 1.0.0 is published as a sequence of release candidates first. `pip` ignores diff --git a/Versioning.md b/Versioning.md index faebbdb..7a051c5 100644 --- a/Versioning.md +++ b/Versioning.md @@ -9,12 +9,14 @@ metadata tables living inside your database. ## What is public - * **Non-underscore names** in the `psycodict` package that are documented — in - the specification documents ([QueryLanguage.md](QueryLanguage.md), - [Searching.md](Searching.md), [DataManagement.md](DataManagement.md), - [MetadataFormats.md](MetadataFormats.md)) or in docstrings. Names with a - leading underscore are private, whatever module they live in, and may change - in any release. + * **The names each module exports in its `__all__`**, and their documented + behavior. These are exactly the names in the [API reference](api/index.md), + and the snapshot test `tests/test_public_api.py` freezes them, so the promise + and the code cannot drift apart. A non-underscore name that is *not* in an + `__all__` (and every underscore-prefixed name, whatever module it lives in) + is implementation: it may still be importable, but it is not part of this + promise and may change in any release. Having a docstring does not make a + name public; being in `__all__` does. * **The query language** as specified in [QueryLanguage.md](QueryLanguage.md): the meaning of a query dictionary is stable within a major version: new features may be added in minor versions, but functioning queries will @@ -34,13 +36,24 @@ metadata tables living inside your database. * **The `meta_*` tables**, whose layout is governed by the metadata format protocol below. +## Reaching a table + +`db[name]` is the canonical, collision-free way to reach a search table, and the +one covered by this promise. `db.` (attribute access) is convenience +syntax for the same lookup, with one caveat: a real attribute or method of the +database object wins over a table of the same name, so a table called `config` +or `tablenames` is reachable only through `db["config"]`. Adding a method to +the database class in a minor release therefore never makes a table inaccessible +through the canonical `db[name]` lookup, even if it shadows an attribute-access +name. + ## What is not covered -Underscore-prefixed names; the exact SQL text psycodict emits (only its -semantics); performance characteristics; the contents of log files; and -undocumented behavior generally, even where observable. If something -undocumented matters to your project, open an issue — turning it into -documented (hence stable) behavior is usually easy. +Non-`__all__` names, whether or not they carry a docstring; underscore-prefixed +names; the exact SQL text psycodict emits (only its semantics); performance +characteristics; the contents of log files; and undocumented behavior generally, +even where observable. If something undocumented matters to your project, open +an issue — turning it into documented (hence stable) behavior is usually easy. ## Database metadata compatibility diff --git a/docs/conf.py b/docs/conf.py index a0d81a1..e563f91 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -52,9 +52,12 @@ # documentation conventions (INPUT:/OUTPUT: bullet blocks, EXAMPLES:: with # literal transcripts); those are plain reST, so autodoc renders them as-is. autodoc_member_order = "bysource" +# No "undoc-members": the API reference documents exactly each module's __all__ +# (its supported public surface), not every non-underscore name that happens to +# have -- or lack -- a docstring. Versioning.md defines the public API as the +# __all__ names, so the reference and the promise stay in step. autodoc_default_options = { "members": True, - "undoc-members": True, "show-inheritance": True, } diff --git a/psycodict/__init__.py b/psycodict/__init__.py index e9c9c79..efc2add 100644 --- a/psycodict/__init__.py +++ b/psycodict/__init__.py @@ -52,3 +52,17 @@ from psycopg.sql import SQL, Identifier, Placeholder, Literal, Composable, Composed assert SQL and Identifier and Placeholder and Literal and Composable and Composed + +# The names psycodict re-exports at the package root. The Postgres* classes and +# the rest of the interface are imported from their submodules; see each +# module's __all__ and Versioning.md for the full public API. +__all__ = [ + "__version__", + "SQL", + "Identifier", + "Placeholder", + "Literal", + "Composable", + "Composed", + "DelayCommit", +] diff --git a/psycodict/base.py b/psycodict/base.py index 92ab8d2..cee283b 100644 --- a/psycodict/base.py +++ b/psycodict/base.py @@ -50,6 +50,13 @@ validate_column_type, ) +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "PostgresBase", +] + ################################################################## # meta_* infrastructure # diff --git a/psycodict/config.py b/psycodict/config.py index c00cb82..efff655 100644 --- a/psycodict/config.py +++ b/psycodict/config.py @@ -17,6 +17,13 @@ from collections import defaultdict from copy import deepcopy +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "Configuration", +] + def strbool(s): """ diff --git a/psycodict/database.py b/psycodict/database.py index 92832c4..2b9b61d 100644 --- a/psycodict/database.py +++ b/psycodict/database.py @@ -55,6 +55,13 @@ from .searchtable import PostgresSearchTable from .utils import DelayCommit, safe_child_path +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "PostgresDatabase", +] + # The registry of metadata-format migrations. Entry N describes the step # from format N-1 to format N; MetadataFormats.md has the checklist a new # format must follow. diff --git a/psycodict/dbdiff.py b/psycodict/dbdiff.py index dc1cdf4..05bb816 100644 --- a/psycodict/dbdiff.py +++ b/psycodict/dbdiff.py @@ -24,6 +24,14 @@ from psycopg.sql import SQL, Identifier +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "compare_databases", + "format_differences", +] + # The columns of meta_tables that are compared for tables present in both # databases (in this order). The remaining columns are deliberately left # out: total is reported through row_counts instead, out_of_order and diff --git a/psycodict/encoding.py b/psycodict/encoding.py index 5747624..507531c 100644 --- a/psycodict/encoding.py +++ b/psycodict/encoding.py @@ -8,6 +8,15 @@ import datetime import math from psycopg.adapt import Dumper + +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "Json", + "Array", + "copy_dumps", +] try: try: # this fails in sage 9.3 diff --git a/psycodict/grants.py b/psycodict/grants.py index 4769c22..370640f 100644 --- a/psycodict/grants.py +++ b/psycodict/grants.py @@ -26,6 +26,14 @@ from .validation import InvalidDefinitionError, MAX_IDENTIFIER_LENGTH +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "GrantPolicy", + "LMFDBGrantPolicy", +] + # The actions a policy can grant. A policy manages exactly these: privileges # outside this set (TRUNCATE, REFERENCES, TRIGGER) are left alone. GRANT_ACTIONS = ("SELECT", "INSERT", "UPDATE", "DELETE") diff --git a/psycodict/notifications.py b/psycodict/notifications.py index 6152601..e6f2378 100644 --- a/psycodict/notifications.py +++ b/psycodict/notifications.py @@ -93,6 +93,13 @@ import psycopg from psycopg.sql import SQL, Identifier +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "NotificationListener", +] + # The single channel on which psycodict announces schema changes. The payload # is the affected table's name. See the module docstring for the contract. SCHEMA_CHANNEL = "psycodict_schema" diff --git a/psycodict/searchtable.py b/psycodict/searchtable.py index d0a6716..0e835d9 100644 --- a/psycodict/searchtable.py +++ b/psycodict/searchtable.py @@ -21,6 +21,13 @@ from .encoding import Json from .utils import IdentifierWrapper, DelayCommit, filter_sql_injection, postgres_infix_ops +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "PostgresSearchTable", +] + # psycopg3 splits plain and server-side cursors into two classes # (psycopg2 had a single cursor class, which this name used to alias) pg_cursor = (Cursor, ServerCursor) diff --git a/psycodict/slowlog.py b/psycodict/slowlog.py index 83fdce3..2c40182 100644 --- a/psycodict/slowlog.py +++ b/psycodict/slowlog.py @@ -40,6 +40,15 @@ from heapq import heapify, heappop, heappush from math import ceil, floor, log10 +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "parse_slow_log", + "slow_query_report", + "show_slow_report", +] + _TIMESTAMP_RE = re.compile(r"^(\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2},\d{3}) - ") _ANSI_RE = re.compile("\x1b\\[[0-9;]*m") _NUMBER = r"(?:[0-9]+\.?[0-9]*|\.[0-9]+)(?:[eE][-+]?[0-9]+)?" diff --git a/psycodict/statstable.py b/psycodict/statstable.py index 8540f4b..52eb6c4 100644 --- a/psycodict/statstable.py +++ b/psycodict/statstable.py @@ -25,6 +25,13 @@ from .encoding import Json, numeric_converter from .utils import DelayCommit, KeyedDefaultDict, make_tuple +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "PostgresStatsTable", +] + # The following is used in bucketing for statistics pg_to_py = {} for typ in [ diff --git a/psycodict/table.py b/psycodict/table.py index 2cdd9c4..531d79f 100644 --- a/psycodict/table.py +++ b/psycodict/table.py @@ -49,6 +49,13 @@ ) from .statstable import PostgresStatsTable +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "PostgresTable", +] + assert _operator_classes and _valid_storage_params diff --git a/psycodict/utils.py b/psycodict/utils.py index e2ed716..7493f5e 100644 --- a/psycodict/utils.py +++ b/psycodict/utils.py @@ -14,6 +14,16 @@ from psycopg.sql import SQL, Identifier, Placeholder +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "DelayCommit", + "IdentifierWrapper", + "LockError", + "SearchParsingError", +] + class SearchParsingError(ValueError): """ diff --git a/psycodict/validation.py b/psycodict/validation.py index 4002de8..45e53dd 100644 --- a/psycodict/validation.py +++ b/psycodict/validation.py @@ -21,6 +21,14 @@ from psycopg.sql import SQL, Identifier +# The supported public API of this module: the names psycodict promises to +# keep across 1.x. Other non-underscore names are implementation that may +# change; see Versioning.md. +__all__ = [ + "InvalidColumnTypeError", + "InvalidDefinitionError", +] + # This dictionary is used when creating new tables # The value associated to each type is the typlen from the pg_type table diff --git a/tests/test_public_api.py b/tests/test_public_api.py new file mode 100644 index 0000000..f062f11 --- /dev/null +++ b/tests/test_public_api.py @@ -0,0 +1,90 @@ +# -*- coding: utf-8 -*- +""" +The public API is exactly what ``__all__`` says it is. + +Versioning.md defines the supported surface as the names each module exports in +``__all__``. This test freezes those sets, so adding or removing a public name +is a deliberate, reviewed change rather than an accident -- and so that a name a +downstream project relies on cannot quietly leave the promise. +""" +import importlib + +import pytest + + +# The frozen public surface, module by module. Changing this is changing the +# 1.x API contract: update Versioning.md and coordinate downstream in the same +# breath. +EXPECTED = { + "psycodict": { + "__version__", "SQL", "Identifier", "Placeholder", "Literal", + "Composable", "Composed", "DelayCommit", + }, + "psycodict.base": {"PostgresBase"}, + "psycodict.database": {"PostgresDatabase"}, + "psycodict.table": {"PostgresTable"}, + "psycodict.searchtable": {"PostgresSearchTable"}, + "psycodict.statstable": {"PostgresStatsTable"}, + "psycodict.config": {"Configuration"}, + "psycodict.utils": { + "DelayCommit", "IdentifierWrapper", "LockError", "SearchParsingError", + }, + "psycodict.encoding": {"Json", "Array", "copy_dumps"}, + "psycodict.grants": {"GrantPolicy", "LMFDBGrantPolicy"}, + "psycodict.notifications": {"NotificationListener"}, + "psycodict.dbdiff": {"compare_databases", "format_differences"}, + "psycodict.slowlog": {"parse_slow_log", "slow_query_report", "show_slow_report"}, + "psycodict.validation": {"InvalidColumnTypeError", "InvalidDefinitionError"}, +} + + +@pytest.mark.parametrize("modname", sorted(EXPECTED)) +def test_all_matches_the_frozen_surface(modname): + mod = importlib.import_module(modname) + assert hasattr(mod, "__all__"), "%s has no __all__" % modname + assert set(mod.__all__) == EXPECTED[modname], ( + "%s.__all__ changed; update Versioning.md and this snapshot deliberately" + % modname + ) + + +@pytest.mark.parametrize("modname", sorted(EXPECTED)) +def test_every_exported_name_resolves(modname): + mod = importlib.import_module(modname) + for name in mod.__all__: + assert hasattr(mod, name), "%s exports %s, which does not exist" % (modname, name) + + +def test_star_import_gives_exactly_all(): + """ + ``from psycodict import *`` binds exactly the root ``__all__`` names. + """ + ns = {} + exec("from psycodict import *", ns) + bound = {k for k in ns if not k.startswith("__") or k == "__version__"} + assert bound == EXPECTED["psycodict"] + + +def test_downstream_imports_still_resolve(): + """ + The names LMFDB and seminars import, including a few private ones and a few + kept-importable-but-unpromised ones, must keep resolving: ``__all__`` governs + ``import *`` and the docs, not explicit imports. + """ + from psycodict import SQL, DelayCommit # noqa: F401 + from psycodict.database import PostgresDatabase # noqa: F401 + from psycodict.searchtable import PostgresSearchTable # noqa: F401 + from psycodict.statstable import PostgresStatsTable # noqa: F401 + from psycodict.base import PostgresBase, number_types # noqa: F401 + from psycodict.config import Configuration # noqa: F401 + from psycodict.encoding import Json, Array, copy_dumps # noqa: F401 + from psycodict.grants import LMFDBGrantPolicy # noqa: F401 + from psycodict.utils import ( # noqa: F401 + IdentifierWrapper, SearchParsingError, + range_formatter, KeyedDefaultDict, + ) + # private names seminars imports for its schema bootstrap + from psycodict.table import _counts_cols, _stats_cols # noqa: F401 + from psycodict.base import ( # noqa: F401 + _meta_indexes_cols, _meta_constraints_cols, _meta_tables_cols, + ) From 10e7d8a7955a11e672693add53b04b0709674110 Mon Sep 17 00:00:00 2001 From: David Roe Date: Wed, 5 Aug 2026 00:57:22 -0400 Subject: [PATCH 7/8] Stop applying a statement timeout based on the role name A connection as ``webserver`` was handed a 25 second statement_timeout by default. That is an LMFDB deployment fact, not something a generic library should infer from a role name, so the special case is removed: _resolve_session_settings applies nothing unless session_settings is given. Explicit settings are unchanged and still survive reconnects and are still held to the allow-list. LMFDB restores the timeout explicitly in its subclass; that change lands before LMFDB bumps to a psycodict carrying this one, or the web workers lose their timeout. --- CHANGELOG.md | 11 +++++++++++ psycodict/database.py | 25 +++++++++++-------------- tests/test_reconnect.py | 32 ++++++++++++++------------------ 3 files changed, 36 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e3967e9..3c358ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -475,6 +475,17 @@ hardening standalone use; the highlights: adding a method in a minor release never makes a table unreachable through `db[name]`. +- **No implicit statement timeout from a role name.** A connection as + `webserver` was silently given a 25 second `statement_timeout`; a generic + library should not act on a role name, so that special case is gone. + `session_settings=` still applies (and survives reconnects) -- a deployment + that wants a timeout passes it explicitly. **LMFDB does this in its subclass** + ([lmfdb#7128](https://github.com/LMFDB/lmfdb/pull/7128)); land that before + bumping LMFDB to a psycodict that includes this change, or the web workers + lose their timeout. *Migration:* pass + `session_settings={"statement_timeout": "25s"}` (or your value) when + constructing the database. + ### Release candidates 1.0.0 is published as a sequence of release candidates first. `pip` ignores diff --git a/psycodict/database.py b/psycodict/database.py index 2b9b61d..5b8509a 100644 --- a/psycodict/database.py +++ b/psycodict/database.py @@ -194,8 +194,8 @@ class PostgresDatabase(PostgresBase): (``statement_timeout``, ``lock_timeout``, ``idle_in_transaction_session_timeout``, ``application_name``) applied to the main database connection and to every replacement main connection. - Defaults to a 25 second statement timeout when connecting as - ``webserver``, and to nothing otherwise. A notification listener's + Defaults to no session settings; a deployment passes the ones it wants + (LMFDB sets its statement timeout here). A notification listener's connection deliberately does not inherit them: it exists to wait, and a statement timeout on it would be more likely to be wrong than right. It does inherit the connection overrides below, so that it reaches the same @@ -241,7 +241,7 @@ class PostgresDatabase(PostgresBase): _search_table_class_ = PostgresSearchTable # The session settings psycodict will apply, and what it applies to a - # webserver connection when the caller asks for nothing else. Settings are + # every connection. Settings are # applied with set_config, which takes the name and the value as bound # parameters; the closed set here is what keeps a caller from setting # something unrelated through the same door. @@ -251,7 +251,6 @@ class PostgresDatabase(PostgresBase): "idle_in_transaction_session_timeout", "application_name", ) - _webserver_session_settings = {"statement_timeout": "25s"} def _resolve_session_settings(self, session_settings): """ @@ -260,19 +259,17 @@ def _resolve_session_settings(self, session_settings): INPUT: - ``session_settings`` -- a dictionary of PostgreSQL settings, or None - to take the default for the connecting role + for no session settings - A connection as ``webserver`` has always been given a 25 second - statement timeout; that is now the default rather than a special case - applied once, so it survives a reconnect and can be overridden. The - settings apply to the main connection and its replacements, not to a - listener's separate connection. + psycodict applies no session setting unless one is asked for: a role + named ``webserver`` used to be handed a 25 second statement timeout, but + a generic library should not act on a role name. A deployment that + wants a timeout passes it explicitly (LMFDB does, through its + subclass), and it then applies to the main connection and every + replacement, not to a listener's separate connection. """ if session_settings is None: - user = self._connection_options().get("user") - session_settings = ( - self._webserver_session_settings if user == "webserver" else {} - ) + session_settings = {} unknown = set(session_settings) - set(self._allowed_session_settings) if unknown: raise ValueError( diff --git a/tests/test_reconnect.py b/tests/test_reconnect.py index dbe06b4..396adf3 100644 --- a/tests/test_reconnect.py +++ b/tests/test_reconnect.py @@ -134,32 +134,28 @@ def test_session_settings_are_applied_and_survive_a_reconnect(own_db): assert setting(database, "lock_timeout") == "3s" -def test_the_resolver_gives_a_webserver_connection_its_timeout(monkeypatch): +def test_no_role_gets_a_session_setting_implicitly(monkeypatch): """ - The real resolver, on the option set it would see: the 25 second timeout - for the webserver role is chosen from the effective connection user, and - the test must not stand in for the code it is checking. - - No database is opened -- this is about the choice, not the connection -- so - a server with a ``webserver`` role is not needed. + A generic library must not act on a role name: the ``webserver`` role no + longer receives an implicit statement timeout, and neither does any other. + A deployment that wants one passes it explicitly. """ database = PostgresDatabase.__new__(PostgresDatabase) - monkeypatch.setattr( - PostgresDatabase, "_connection_options", lambda self: {"user": "webserver"} - ) - assert database._resolve_session_settings(None) == {"statement_timeout": "25s"} - # an explicit setting overrides the default for that role - assert database._resolve_session_settings({"lock_timeout": "3s"}) == { - "lock_timeout": "3s" - } + for role in ("webserver", "postgres", "lmfdb"): + monkeypatch.setattr( + PostgresDatabase, "_connection_options", lambda self, r=role: {"user": r} + ) + assert database._resolve_session_settings(None) == {} -def test_the_resolver_gives_other_roles_nothing_implicitly(monkeypatch): +def test_explicit_session_settings_are_kept(monkeypatch): database = PostgresDatabase.__new__(PostgresDatabase) monkeypatch.setattr( - PostgresDatabase, "_connection_options", lambda self: {"user": "postgres"} + PostgresDatabase, "_connection_options", lambda self: {"user": "webserver"} ) - assert database._resolve_session_settings(None) == {} + assert database._resolve_session_settings({"statement_timeout": "25s"}) == { + "statement_timeout": "25s" + } def test_unknown_session_settings_are_refused(own_db): From 37239ce35ba96a3197d305a61807801040930fa7 Mon Sep 17 00:00:00 2001 From: David Roe Date: Wed, 5 Aug 2026 03:04:38 -0400 Subject: [PATCH 8/8] Fix the companion-PR link and the session-setting scope wording The changelog pointed at lmfdb#7128 ("Check the bound in PrimeBound"), which has nothing to do with this change. The migration it tells maintainers to land first is lmfdb#7131, "Set the web workers' statement timeout explicitly". The comment above _allowed_session_settings said the settings apply to "a every connection", which is both ungrammatical and wrong: a listener's notification connection deliberately does not inherit them. Both it and the first line of _resolve_session_settings's docstring now say what the rest of that docstring already said, the main connection and its replacements. No behavior change. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 2 +- psycodict/database.py | 11 +++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c358ce..68c74e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -480,7 +480,7 @@ hardening standalone use; the highlights: library should not act on a role name, so that special case is gone. `session_settings=` still applies (and survives reconnects) -- a deployment that wants a timeout passes it explicitly. **LMFDB does this in its subclass** - ([lmfdb#7128](https://github.com/LMFDB/lmfdb/pull/7128)); land that before + ([lmfdb#7131](https://github.com/LMFDB/lmfdb/pull/7131)); land that before bumping LMFDB to a psycodict that includes this change, or the web workers lose their timeout. *Migration:* pass `session_settings={"statement_timeout": "25s"}` (or your value) when diff --git a/psycodict/database.py b/psycodict/database.py index 5b8509a..8b680cc 100644 --- a/psycodict/database.py +++ b/psycodict/database.py @@ -240,11 +240,10 @@ class PostgresDatabase(PostgresBase): # Override the following to use a different class for search tables _search_table_class_ = PostgresSearchTable - # The session settings psycodict will apply, and what it applies to a - # every connection. Settings are - # applied with set_config, which takes the name and the value as bound - # parameters; the closed set here is what keeps a caller from setting - # something unrelated through the same door. + # The session settings psycodict applies to the main connection and every + # replacement main connection. Settings are applied with set_config, which + # takes the name and value as bound parameters; the closed set here is what + # keeps a caller from setting something unrelated through the same door. _allowed_session_settings = ( "statement_timeout", "lock_timeout", @@ -254,7 +253,7 @@ class PostgresDatabase(PostgresBase): def _resolve_session_settings(self, session_settings): """ - The session settings to apply to every connection of this database. + The session settings to apply to the main connection and its replacements. INPUT: