From a36cb8a208a926de811984c0ed167762f57088fd Mon Sep 17 00:00:00 2001 From: David Roe Date: Tue, 4 Aug 2026 17:52:18 -0400 Subject: [PATCH 1/9] 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/9] 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/9] 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/9] 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/9] 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 fcd7d322d7b66875350b025ba12b8dd082ae4aee Mon Sep 17 00:00:00 2001 From: David Roe Date: Wed, 5 Aug 2026 03:35:22 -0400 Subject: [PATCH 6/9] Read the export marker exactly, count data rows, reconcile the docs Review follow-ups to the export-format marker and the packaging changes. Marker recognition. `_read_header_lines` decided on `startswith`, but a column may be called anything printable, so a format-0 names row can begin with the marker prefix -- `# psycodict-export-format: 1|another_column` is a valid two-column header, and a one-column header may be a valid marker character for character. Both were refused, breaking the promise that every older export still loads. The reader now looks ahead: a format-0 header is names/types/blank and a marked one marker/names/types/blank, so a blank third physical line means the first line was column names. Only then is the line parsed as a marker, against the whole grammar (`export_format_version`), which also rejects the negative versions `int()` used to accept as "not newer than the current format". The blank-line error no longer names a line number that is right for only one of the two layouts. Reindex threshold. `copy_from`'s automatic choice counted physical lines against 1003, so a marked file of exactly 1000 rows reindexed although the documented policy is *more than* 1000, and moving the constant would only shift the bug to format-0 files. New `_count_data_rows` consumes the header through the shared reader (honoring `sep`) and counts what is left. Tests. `test_create_table_from_header_reads_the_marker` asserted only that an export starts with the marker and never entered the header-reading path it was named for; it is replaced by one that reloads with `adjust_schema=True` from a file naming a strict subset of the columns, so the reloaded table proves the header drove its creation. Added: the two marker-like legacy headers, a negative version, the grammar itself, custom separators, and the 1000/1001 reindex boundary for both formats (asked of `drop_indexes`, since both branches leave the same indexes behind). Docs. MetadataFormats.md, the `META_FORMAT` comment and DataManagement.md still said the metadata format number tracks the package major version; Versioning.md now says otherwise, and it is the source of truth -- a compatible additive revision may ship in a minor release, one that raises `min_compat` past a supported client may not. CONTRIBUTING.md and the psycopg migration note still recommended the removed `pgsource` extra; README now states the psycopg range as `>=3.2.4,<4`. `postgres` is described as the conventional maintenance database rather than as libpq's default, which is actually the connection user name. Packaging. `config.ini.example` was tracked, changed by this branch and absent from the sdist; MANIFEST.in names it, and the unpacked-sdist CI step now asserts the files no test would miss. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 8 ++ CHANGELOG.md | 21 +-- CONTRIBUTING.md | 6 +- DataManagement.md | 7 +- MANIFEST.in | 7 + MetadataFormats.md | 36 +++-- README.md | 2 +- psycodict/base.py | 117 ++++++++++++---- psycodict/config.py | 7 +- psycodict/table.py | 16 ++- tests/test_export_format.py | 265 +++++++++++++++++++++++++++++++++--- 11 files changed, 411 insertions(+), 81 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index be22490..21a2bb3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -216,6 +216,14 @@ jobs: run: | mkdir -p "$RUNNER_TEMP/sdist" && tar xzf dist/*.tar.gz -C "$RUNNER_TEMP/sdist" src="$(echo "$RUNNER_TEMP"/sdist/psycodict-*)" + # Files the sdist must carry that no test would miss: name them, so + # dropping one fails here instead of shipping. + for f in config.ini.example CHANGELOG.md CONTRIBUTING.md SECURITY.md \ + CITATION.cff DataManagement.md MetadataFormats.md \ + QueryLanguage.md Searching.md Versioning.md \ + tests/conftest.py; do + test -f "$src/$f" || { echo "sdist is missing $f"; exit 1; } + done python -m venv "$RUNNER_TEMP/sdist-venv" "$RUNNER_TEMP/sdist-venv/bin/pip" install "${src}[pgbinary,test]" cd "$src" diff --git a/CHANGELOG.md b/CHANGELOG.md index fdb4201..d53c688 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,10 +21,13 @@ migration note. - **Ported from psycopg2 to psycopg 3.** psycodict no longer depends on psycopg2, and it re-exports `SQL`, `Identifier`, `Placeholder`, `Literal`, `Composable` and `Composed` from `psycodict` so callers need not import a - driver directly. *Migration:* install with the `pgbinary` or `pgsource` extra - (`pip install "psycodict[pgbinary]"`; psycopg 3.2.4 or newer -- the + driver directly. *Migration:* psycopg is a plain dependency, so + `pip install psycodict` brings the pure-Python driver for your system + `libpq`; add the `pgbinary` extra for the bundled binary build + (`pip install "psycodict[pgbinary]"`) or `pgc` for a locally compiled one. + The supported range is psycopg 3.x, version 3.2.4 or later -- the notification listener relies on psycopg 3.2 APIs and on 3.2.4's - notification-delivery fix), and import the SQL composition classes + notification-delivery fix. Import the SQL composition classes from `psycodict` rather than `psycopg2.sql`. (#88) - **The old `join_search` method is removed.** *Migration:* pass `join=` to `search` (or `count` / `lucky`) instead; see the joins section of @@ -52,7 +55,7 @@ migration note. *Migration:* pass `include_nones=False` at `create_table` to keep the old behavior of omitting `None`-valued keys from result dictionaries. (#103) - **Metadata format 1.** The layout of the `meta_*` tables is now versioned - (format 0 is the unstamped 0.x baseline; format 1, aligned with this major + (format 0 is the unstamped 0.x baseline; format 1, which arrives with this release, adds `meta_indexes.whereclause`), stamped in the new `meta_format` table and checked on connect. A format-0 database keeps working — every connection warns and operates at the old format, with the format-1 features @@ -453,10 +456,12 @@ hardening standalone use; the highlights: not necessarily the reverse) and decouples both format numbers from the package major version. - **Generic defaults instead of LMFDB ones.** The default database name is - `postgres` (libpq's own default) rather than `lmfdb`; deployments name their - own database in the config or constructor, as LMFDB already does. The unused - `secretsfile` argument to `PostgresDatabase` is removed (secrets-file - resolution lives in `Configuration`). + `postgres` rather than `lmfdb` -- the maintenance database conventionally + created with a PostgreSQL cluster, so the one most likely to exist. (Note + that it is not libpq's own default, which is the connection user name.) + Deployments name their own database in the config or constructor, as LMFDB + already does. The unused `secretsfile` argument to `PostgresDatabase` is + removed (secrets-file resolution lives in `Configuration`). - **Packaging.** The source distribution is complete -- tests including `conftest.py`, the maintenance scripts, the guides and metadata files -- and CI unpacks it and runs its database-free tests, so a missing file fails the diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4aa5f8d..da90b64 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -59,8 +59,10 @@ python -m pip install -e ".[pgbinary,test]" python -m pip install ruff ``` -The `pgsource` extra may be used instead of `pgbinary` when testing against a -system installation of libpq. +To test against a system installation of libpq, leave `pgbinary` out and run +`pip install -e ".[test]"` instead: psycopg is a plain dependency, so the base +install already brings the pure-Python driver, which uses whatever libpq the +system provides. (`pgc` compiles against it instead, and needs build tools.) ## PostgreSQL test database diff --git a/DataManagement.md b/DataManagement.md index 854f3b8..8ee40d6 100644 --- a/DataManagement.md +++ b/DataManagement.md @@ -75,7 +75,7 @@ after a reconnect. * **`meta_tables`** — one row per search table, holding `name`, `sort`, `count_cutoff`, `id_ordered`, `out_of_order`, `stats_valid`, `label_col`, `total`, `important` and `include_nones`. This is the source of truth psycodict reads on connection to reconstruct each table object. * **`meta_indexes`** and **`meta_constraints`** — one row per index / constraint, recording how to rebuild it. `reload` and `restore_indexes` rebuild from these rows, **not** from whatever is physically on the table (see [reload](#reload)). * **`meta_tables_hist`**, **`meta_indexes_hist`**, **`meta_constraints_hist`** — versioned history of the three tables above, so that `reload_meta`/`revert_meta` can roll a table's metadata forward and back. - * **`meta_format`** — a single-row `(version, min_compat)` stamp of the metadata *format*: the layout of the `meta_*` tables, versioned by an integer aligned with psycodict's major version (`META_FORMAT`). Every connection checks it. The same format connects silently; an older but compatible format connects with a warning and operates at that older format (newer features unavailable) so a not-yet-migrated or read-only database — the LMFDB devmirror, say — keeps working; a newer format connects when its stamped `min_compat` admits this psycodict, and is otherwise refused. A database that has meta tables but no `meta_format` is the unstamped 0.x baseline (format 0); one with no meta tables at all is fresh and must be connected to with `PostgresDatabase(create=True)`, which bootstraps all of the above. Migrating to a newer format is deliberate — `db.upgrade_metadata()` or `PostgresDatabase(upgrade=True)`, never a side effect of connecting. See [MetadataFormats.md](MetadataFormats.md) for the full policy (including why a pre-1.0 psycodict must not be pointed at a migrated database). + * **`meta_format`** — a single-row `(version, min_compat)` stamp of the metadata *format*: the layout of the `meta_*` tables, versioned by an integer of its own (`META_FORMAT`), independent of psycodict's major version. Every connection checks it. The same format connects silently; an older but compatible format connects with a warning and operates at that older format (newer features unavailable) so a not-yet-migrated or read-only database — the LMFDB devmirror, say — keeps working; a newer format connects when its stamped `min_compat` admits this psycodict, and is otherwise refused. A database that has meta tables but no `meta_format` is the unstamped 0.x baseline (format 0); one with no meta tables at all is fresh and must be connected to with `PostgresDatabase(create=True)`, which bootstraps all of the above. Migrating to a newer format is deliberate — `db.upgrade_metadata()` or `PostgresDatabase(upgrade=True)`, never a side effect of connecting. See [MetadataFormats.md](MetadataFormats.md) for the full policy (including why a pre-1.0 psycodict must not be pointed at a migrated database). Rows of `meta_indexes` and `meta_constraints` become DDL when an index or constraint is rebuilt, so psycodict validates them both when they are imported @@ -215,6 +215,7 @@ The reader (`_read_header_lines`/`_check_header_lines`) checks that these names Here is a genuine two-row export (`db.demo_circles.copy_to("demo_circles.txt")`) of the table created above: ``` +# psycodict-export-format: 1 id|center|label|props|radius bigint|integer[]|text|jsonb|integer @@ -222,11 +223,11 @@ bigint|integer[]|text|jsonb|integer 1|{3,4}|big|{"note": "3-4-5", "unit": false}|5 ``` -Note the id column first, the `integer[]` written as `{0,0}`, the `jsonb` keys reordered by Postgres, and (had a value contained a `|`) it would be escaped as `\|`. +Note the marker line, the id column first, the `integer[]` written as `{0,0}`, the `jsonb` keys reordered by Postgres, and (had a value contained a `|`) it would be escaped as `\|`. ### `copy_to` and `copy_from` -**`copy_to(searchfile, countsfile=None, statsfile=None, indexesfile=None, constraintsfile=None, metafile=None, columns=None, query=None, include_id=True)`** exports via `COPY ... TO STDOUT`. Only the *search* file gets the three header lines; the counts, stats, meta, index and constraint files are headerless raw rows (their column layout is fixed and known). `query` restricts which rows are exported; `columns` restricts which columns. The counts and stats files are written only if the table has `saving` on. +**`copy_to(searchfile, countsfile=None, statsfile=None, indexesfile=None, constraintsfile=None, metafile=None, columns=None, query=None, include_id=True)`** exports via `COPY ... TO STDOUT`. Only the *search* file gets the marker and the three header lines; the counts, stats, meta, index and constraint files are headerless raw rows (their column layout is fixed and known). `query` restricts which rows are exported; `columns` restricts which columns. The counts and stats files are written only if the table has `saving` on. **`copy_from(searchfile, resort=False, reindex=None, restat=True)`** streams a search file into the **existing** table via `COPY ... FROM STDIN`, *appending* rows. It validates the header, and if the file has no `id` column it assigns ids contiguously starting just after the current maximum (using a temporary sequence). `reindex` defaults to `True` for files with more than 1000 data rows. `total` is incremented and statistics invalidated. diff --git a/MANIFEST.in b/MANIFEST.in index 190855c..5a8d1ca 100644 --- a/MANIFEST.in +++ b/MANIFEST.in @@ -21,6 +21,13 @@ include CHANGELOG.md include CONTRIBUTING.md include SECURITY.md +# The configuration example the guides tell users to copy to config.ini. It is +# named here rather than left to setuptools' implicit list, which does not +# cover it. (The remaining tracked files at the top level are either included +# by setuptools itself -- LICENSE, README.md, pyproject.toml, MANIFEST.in -- or +# development-only: .gitignore and .readthedocs.yaml.) +include config.ini.example + # The documentation sources, so the sdist can rebuild the docs. graft docs prune docs/_build diff --git a/MetadataFormats.md b/MetadataFormats.md index 55222d3..3e7ab0c 100644 --- a/MetadataFormats.md +++ b/MetadataFormats.md @@ -20,9 +20,19 @@ deliberate step rather than a side effect of connecting. | 0 | 0.x | Baseline: `meta_tables`, `meta_indexes`, `meta_constraints` and their `_hist` counterparts, with no format stamp. | | 1 | 1.0 | `meta_indexes` / `meta_indexes_hist` gain a nullable `whereclause` column, holding the predicate of a partial index (NULL for an ordinary index). Compatible with format 0: everything keeps working against a format-0 database except creating partial indexes. | -The format number is aligned with psycodict's major version: format *N* is -introduced by psycodict *N*.0, and plans to change the format should be -scheduled for a major release. +The format number is a protocol revision in its own right, not psycodict's +major version; format 1 arriving with psycodict 1.0 is where the two happened +to start, not a rule that ties them together. Which release a revision may +ship in follows from what it does to `min_compat` (see +[Versioning.md](Versioning.md)): + + * a **compatible, additive** revision — new columns, `min_compat` left low + enough that every supported client still connects — may ship in a minor + release; + * a **breaking** revision — one that raises `min_compat` past a client of the + current major series, or otherwise makes that client unsafe — requires a new + major version, since under semantic versioning that is what a break in the + compatibility promise means. ## The stamp @@ -102,7 +112,7 @@ it never reads them. > mixed-version rollout; once every process has been upgraded to 1.0+ there is > nothing to watch for. (The alternative — leaving a tombstone in the old > `meta_version` table so pre-1.0 clients refuse — was declined in favor of the -> single-stamp design; the numbering aligns with the major version instead.) +> single-stamp design, which pre-1.0 clients simply cannot see.) ## Migrating @@ -206,22 +216,26 @@ Suppose the layout of the meta tables must change. Then: reordering or repurposing an existing column is a *breaking* change; avoid it if at all possible, and see step 4 if not. 2. **Bump `META_FORMAT`** in `psycodict/base.py`, add a line to its History - comment and to the table at the top of this file. Schedule the bump for - the next major release, so the format number and the major version stay - aligned. + comment and to the table at the top of this file. The bump is a protocol + revision, so which release it may ship in is decided in step 4, by what it + does to `min_compat` — not by matching the format number to the major + version. 3. **Register the migration** in `META_MIGRATIONS` in `psycodict/database.py`: a one-line description, the `compatible` flag, the `min_compat` to stamp, and the name of a `PostgresDatabase` method performing the DDL. The method does only the DDL, idempotently (`IF NOT EXISTS` / `IF EXISTS`); stamping is handled by `upgrade_metadata`. -4. **Choose the compatibility flags honestly.** +4. **Choose the compatibility flags honestly** — they decide which release + the revision may ship in. - A purely additive change: `compatible=True`, and `min_compat` stays - what it was (0 today). + what it was (0 today). No supported client is shut out, so this may go + out in a minor release. - A breaking change: `compatible=False` (older-format databases are refused rather than warned) and `min_compat` raised to the new format - (older psycodicts are refused by newer databases). Say so loudly in - the CHANGELOG. + (older psycodicts are refused by newer databases). That shuts out + clients of the current major series, so it requires a new major version. + Say so loudly in the CHANGELOG. 5. **Gate the feature.** Code that reads or writes the new columns must consult `self._db._meta_format` and either degrade (reads: the column is absent, so treat it as its default) or raise with instructions (writes diff --git a/README.md b/README.md index 2d96906..85126e1 100644 --- a/README.md +++ b/README.md @@ -131,7 +131,7 @@ See the [CHANGELOG](https://github.com/roed314/psycodict/blob/main/CHANGELOG.md) - Python 3.9 or newer. - PostgreSQL 13 through 18. -- psycopg 3.2.4 or newer (a plain dependency; the `pgbinary`/`pgc` extras above pick a build). +- psycopg 3.x, version 3.2.4 or later (`psycopg>=3.2.4,<4`, a plain dependency; the `pgbinary`/`pgc` extras above pick a build). Python 3.9 and PostgreSQL 13 are already past their upstream end of life; psycodict keeps supporting them as legacy compatibility for downstream diff --git a/psycodict/base.py b/psycodict/base.py index 92ab8d2..c621210 100644 --- a/psycodict/base.py +++ b/psycodict/base.py @@ -71,9 +71,11 @@ def jsonb_idx(cols, cols_type): # The version of the metadata format described by the constants below: the -# layout of the meta_* tables, versioned by a single integer aligned with -# psycodict's major version (format N is introduced by psycodict N.0). The -# format of a database is stamped into the single-row meta_format table as +# layout of the meta_* tables, versioned by a single integer. That integer is +# a protocol revision of its own, not psycodict's major version -- a compatible +# additive revision may ship in a minor release, while one that raises +# min_compat past a supported client needs a major one. The format of a +# database is stamped into the single-row meta_format table as # (version, min_compat), and every connection checks it: an older but # compatible format connects with a warning and reduced functionality, while # a layout this psycodict cannot safely use is refused. The policy, and the @@ -102,6 +104,15 @@ def jsonb_idx(cols, cols_type): EXPORT_FORMAT = 1 EXPORT_FORMAT_MARKER = "# psycodict-export-format:" +# The whole of the marker grammar: the prefix, then a nonnegative decimal +# version and nothing else. Only a line matching this is a version; a line +# that merely begins with the prefix may well be a format-0 row of column +# names, since a column may be called anything that is not empty and holds no +# control characters (see validate_column_name). +_EXPORT_FORMAT_MARKER_RE = re.compile( + r"%s[ \t]*([0-9]+)" % re.escape(EXPORT_FORMAT_MARKER) +) + def export_format_line(): """ @@ -110,6 +121,29 @@ def export_format_line(): return "%s %s" % (EXPORT_FORMAT_MARKER, EXPORT_FORMAT) +def export_format_version(line): + """ + The export-format version a marker line declares. + + INPUT: + + - ``line`` -- a line known to be a format marker rather than a row of + column names. + + OUTPUT: + + The nonnegative integer version, or a ``ValueError`` if ``line`` is not + exactly ``# psycodict-export-format: N``. A signed or non-decimal version + is malformed: psycodict has never written one, so a file carrying one was + not written by any psycodict. + """ + marker = line.strip() + match = _EXPORT_FORMAT_MARKER_RE.fullmatch(marker) + if match is None: + raise ValueError("Malformed export-format marker: %r" % (marker,)) + return int(match.group(1)) + + _meta_tables_cols = ( "name", "sort", @@ -921,9 +955,10 @@ def _check_header_lines( self, F, table_name, columns_set, sep="|", prohibit_missing=True ): """ - Reads the header lines from a file (row of column names, row of column - types, blank line), checking if these names match the columns set and - the types match the expected types in the table. + Reads the header lines from a file (an optional format marker, the row + of column names, the row of column types and the blank line), checking + if these names match the columns set and the types match the expected + types in the table. Returns a list of column names present in the header. INPUT: @@ -1043,7 +1078,7 @@ def _copy_from(self, filename, table, columns, header, kwds): with DelayCommit(self, silence=True): with open(filename) as F: if header: - # This consumes the first three lines + # This consumes the header, leaving F at the first data row columns = self._check_header_lines(F, table, set(columns), sep=sep) addid = "id" not in columns else: @@ -1359,32 +1394,41 @@ def _read_header_lines(self, F, sep="|"): one is format 0, the historical layout, so every file psycodict has ever written still reads. A version newer than this psycodict understands is refused here, before any of the data is loaded. + + A format-0 names row may itself begin with the marker prefix, since a + column may be called anything printable, so the prefix alone does not + decide which layout this is. The blank line does: a format-0 header is + ``names / types / blank`` and a marked one ``marker / names / types / + blank``, so if the third physical line is blank the first line was a + row of column names. """ first = F.readline() - marker = first.strip() - if marker.startswith(EXPORT_FORMAT_MARKER): - version_text = marker[len(EXPORT_FORMAT_MARKER):].strip() - try: - version = int(version_text) - except ValueError: - raise ValueError( - "Malformed export-format marker: %r" % (marker,) - ) - if version > EXPORT_FORMAT: - raise ValueError( - "This file is psycodict export format %s, but this psycodict " - "understands only up to format %s; upgrade psycodict to read " - "it" % (version, EXPORT_FORMAT) - ) - names_line = F.readline() + if first.strip().startswith(EXPORT_FORMAT_MARKER): + second = F.readline() + third = F.readline() + if third.strip(): + # Not a format-0 header, so the first line really is a marker. + version = export_format_version(first) + if version > EXPORT_FORMAT: + raise ValueError( + "This file is psycodict export format %s, but this psycodict " + "understands only up to format %s; upgrade psycodict to read " + "it" % (version, EXPORT_FORMAT) + ) + names_line, types_line = second, third + blank = F.readline() + else: + # Format 0, whose one column happens to be named like a marker. + names_line, types_line, blank = first, second, third else: # No marker: format 0, and this first line is the column names. names_line = first + types_line = F.readline() + blank = F.readline() names = [x.strip() for x in names_line.strip().split(sep)] - types = [x.strip() for x in F.readline().strip().split(sep)] - blank = F.readline() + types = [x.strip() for x in types_line.strip().split(sep)] if blank.strip(): - raise ValueError("The third line must be blank") + raise ValueError("The header must end with a blank line") if len(names) != len(types): raise ValueError( "The first line specifies %s columns, while the second specifies %s" @@ -1392,6 +1436,27 @@ def _read_header_lines(self, F, sep="|"): ) return list(zip(names, types)) + def _count_data_rows(self, filename, sep="|"): + """ + The number of data rows in a search-data file. + + INPUT: + + - ``filename`` -- the search-data file to count. + - ``sep`` -- a string giving the column separator, since the header is + parsed to find where the data starts. + + OUTPUT: + + The number of lines after the header. Counting physical lines instead + would count the header too, and a marked file's header is one line + longer than an unmarked one's, so the two formats would not agree on + what "1000 rows" means. + """ + with open(filename) as F: + self._read_header_lines(F, sep=sep) + return sum(1 for _ in F) + ################################################################## # Exporting, importing, reloading and reverting meta_* # ################################################################## diff --git a/psycodict/config.py b/psycodict/config.py index c00cb82..1779999 100644 --- a/psycodict/config.py +++ b/psycodict/config.py @@ -197,8 +197,11 @@ def __init__(self, parser=None, defaults={}, writeargstofile=False, readargs=Fal metavar="DBNAME", help="PostgreSQL database name [default: %(default)s]", # 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. + # maintenance database conventionally created with a PostgreSQL + # cluster, so it is the one most likely to exist. (It is not + # libpq's own default, which is the connection user name.) + # Deployments name their own database in the config file or + # constructor. default=defaults.get("postgresql_dbname", "postgres"), ) diff --git a/psycodict/table.py b/psycodict/table.py index 2cdd9c4..6353260 100644 --- a/psycodict/table.py +++ b/psycodict/table.py @@ -1956,8 +1956,9 @@ def _set_ordered(self): def _write_header_lines(self, F, cols, sep="|", include_id=True): """ - Writes the header lines to a file - (row of column names, row of column types, blank line). + Writes the header lines to a file (the format marker, the row of column + names, the row of column types and the blank line that ends the + header). INPUT: @@ -3062,11 +3063,12 @@ def copy_from( try: with DelayCommit(self, silence=True): if reindex is None: - rowcount = 0 - with open(searchfile) as F: - for line in F: - rowcount += 1 - reindex = rowcount > 1003 # 3 header lines + # Data rows, not physical lines: the header is three lines + # in a format-0 file and four in a marked one, so counting + # lines would put the documented boundary in a different + # place for each format. + rowcount = self._count_data_rows(searchfile, sep=kwds.get("sep", "|")) + reindex = rowcount > 1000 if reindex: self.drop_indexes(permanent=False) now = time.time() diff --git a/tests/test_export_format.py b/tests/test_export_format.py index d41e3a2..009c438 100644 --- a/tests/test_export_format.py +++ b/tests/test_export_format.py @@ -10,7 +10,16 @@ """ import pytest -from psycodict.base import EXPORT_FORMAT, EXPORT_FORMAT_MARKER, export_format_line +from psycodict.base import ( + EXPORT_FORMAT, + EXPORT_FORMAT_MARKER, + export_format_line, + export_format_version, +) + +# A column may be called anything printable, so this is a legal column name -- +# and it is character for character what a current marker line looks like. +MARKER_NAME = export_format_line() def read(path): @@ -18,6 +27,16 @@ def read(path): return F.read() +def strip_marker(path, dest): + """ + The same file as a format-0 file: byte for byte, minus the marker line. + """ + body = read(str(path)).split("\n", 1)[1] + assert read(str(path)).startswith(EXPORT_FORMAT_MARKER) + dest.write_text(body) + return str(dest) + + def test_a_new_export_starts_with_the_marker(filled_table, tmp_path): searchfile = str(tmp_path / "s.txt") filled_table.copy_to(searchfile) @@ -42,15 +61,11 @@ def test_a_legacy_unmarked_file_still_loads(filled_table, table_factory, tmp_pat """ 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) + legacy = strip_marker(marked, tmp_path / "legacy.txt") + assert not read(legacy).startswith(EXPORT_FORMAT_MARKER) target = table_factory() - target.copy_from(str(legacy)) + target.copy_from(legacy) assert target.count() == 200 @@ -60,10 +75,7 @@ def test_a_future_format_is_refused_before_loading(filled_table, table_factory, 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)) + rewrite_first_line(searchfile, "%s %s" % (EXPORT_FORMAT_MARKER, EXPORT_FORMAT + 5)) target = table_factory() with pytest.raises(ValueError, match="export format"): @@ -72,13 +84,140 @@ def test_a_future_format_is_refused_before_loading(filled_table, table_factory, def test_a_malformed_marker_is_rejected(filled_table, table_factory, tmp_path): + searchfile = filled_table_dump(filled_table, tmp_path) + rewrite_first_line(searchfile, "%s banana" % EXPORT_FORMAT_MARKER) + target = table_factory() + with pytest.raises(ValueError, match="[Mm]arker"): + target.copy_from(searchfile) + assert target.count() == 0 + + +def test_a_negative_version_is_rejected(filled_table, table_factory, tmp_path): + """ + A version is a nonnegative decimal. ``int()`` would happily read ``-1``, + but psycodict has never written such a marker, so a file carrying one was + not written by any psycodict and must not be read as format -1 (which, + being "not newer than the current format", would otherwise be accepted). + """ + searchfile = filled_table_dump(filled_table, tmp_path) + rewrite_first_line(searchfile, "%s -1" % EXPORT_FORMAT_MARKER) + target = table_factory() + with pytest.raises(ValueError, match="[Mm]arker"): + target.copy_from(searchfile) + assert target.count() == 0 + + +def test_export_format_version_reads_only_the_exact_grammar(): + assert export_format_version(export_format_line()) == EXPORT_FORMAT + assert export_format_version("%s 0" % EXPORT_FORMAT_MARKER) == 0 + assert export_format_version(" %s 12\n" % EXPORT_FORMAT_MARKER) == 12 + for bad in [ + "%s -1" % EXPORT_FORMAT_MARKER, + "%s +1" % EXPORT_FORMAT_MARKER, + "%s 1.0" % EXPORT_FORMAT_MARKER, + "%s banana" % EXPORT_FORMAT_MARKER, + "%s" % EXPORT_FORMAT_MARKER, + "%s 1 and more" % EXPORT_FORMAT_MARKER, + "%s 1|another_column" % EXPORT_FORMAT_MARKER, + ]: + with pytest.raises(ValueError, match="[Mm]arker"): + export_format_version(bad) + + +# --- legacy headers that look like a marker -------------------------------- +# +# validate_column_name rules out only an empty name and control characters, so +# a format-0 names row may begin with the marker prefix, or be exactly a valid +# marker. Deciding on the prefix alone would refuse to load those older files. + +MARKER_LIKE_COLUMNS = [(MARKER_NAME, "integer"), ("another_column", "text")] +MARKER_ONLY_COLUMNS = [(MARKER_NAME, "integer")] + + +def marker_like_table(table_factory, columns): + table = table_factory(columns=columns, label_col=None, sort=None) + table.insert_many([{col: i if typ == "integer" else "v%d" % i + for col, typ in columns} for i in range(5)]) + return table + + +def test_a_legacy_names_row_beginning_with_the_prefix_loads(table_factory, tmp_path): + """ + ``# psycodict-export-format: 1|another_column`` is a valid format-0 names + row for a two-column table, not a malformed marker. + """ + source = marker_like_table(table_factory, MARKER_LIKE_COLUMNS) + marked = str(tmp_path / "marked.txt") + source.copy_to(marked, include_id=False) + legacy = strip_marker(marked, tmp_path / "legacy.txt") + assert read(legacy).split("\n")[0] == "%s|another_column" % MARKER_NAME + + target = table_factory(columns=MARKER_LIKE_COLUMNS, label_col=None, sort=None) + target.copy_from(legacy) + assert target.count() == 5 + assert {r[MARKER_NAME] for r in target.search({}, [MARKER_NAME])} == set(range(5)) + + +def test_a_one_column_legacy_names_row_equal_to_a_marker_loads(table_factory, tmp_path): + """ + The exactly ambiguous case: a one-column format-0 file whose names row *is* + a syntactically valid marker. The blank line tells the two apart -- a + format-0 header is names/types/blank, a marked one marker/names/types/blank. + """ + source = marker_like_table(table_factory, MARKER_ONLY_COLUMNS) + marked = str(tmp_path / "marked.txt") + source.copy_to(marked, include_id=False) + legacy = strip_marker(marked, tmp_path / "legacy.txt") + assert read(legacy).split("\n")[0] == MARKER_NAME + + target = table_factory(columns=MARKER_ONLY_COLUMNS, label_col=None, sort=None) + target.copy_from(legacy) + assert target.count() == 5 + assert {r[MARKER_NAME] for r in target.search({}, [MARKER_NAME])} == set(range(5)) + + +def test_a_marker_like_legacy_names_row_loads_with_a_custom_separator( + table_factory, tmp_path +): + """ + The parser takes ``sep=``, and the names row is split with it, so the + disambiguation must hold for a file written with a separator other than + ``|`` too. + """ + source = marker_like_table(table_factory, MARKER_LIKE_COLUMNS) + marked = str(tmp_path / "marked.txt") + source.copy_to(marked, include_id=False, sep=";") + legacy = strip_marker(marked, tmp_path / "legacy.txt") + assert read(legacy).split("\n")[0] == "%s;another_column" % MARKER_NAME + + target = table_factory(columns=MARKER_LIKE_COLUMNS, label_col=None, sort=None) + target.copy_from(legacy, sep=";") + assert target.count() == 5 + + +def test_a_marked_file_round_trips_with_a_custom_separator(table_factory, tmp_path): + source = marker_like_table(table_factory, MARKER_LIKE_COLUMNS) + marked = str(tmp_path / "marked.txt") + source.copy_to(marked, include_id=False, sep=";") + assert read(marked).split("\n")[0] == MARKER_NAME + + target = table_factory(columns=MARKER_LIKE_COLUMNS, label_col=None, sort=None) + target.copy_from(marked, sep=";") + assert target.count() == 5 + + +def test_a_header_without_its_blank_line_says_so(filled_table, table_factory, tmp_path): + """ + The error names what is wrong, not which physical line it was on: the blank + line is the third line of a format-0 header and the fourth of a marked one. + """ searchfile = filled_table_dump(filled_table, tmp_path) lines = read(searchfile).split("\n") - lines[0] = "%s banana" % EXPORT_FORMAT_MARKER + del lines[3] # the blank line of a marked header with open(searchfile, "w") as F: F.write("\n".join(lines)) target = table_factory() - with pytest.raises(ValueError, match="[Mm]arker"): + with pytest.raises(ValueError, match="blank line"): target.copy_from(searchfile) @@ -89,15 +228,92 @@ def test_reload_reads_a_marked_file(filled_table, tmp_path): assert filled_table.count() == 200 -def test_create_table_from_header_reads_the_marker(db, filled_table, tmp_path): +def test_adjust_schema_builds_the_table_from_a_marked_header(db, filled_table, tmp_path): + """ + ``adjust_schema=True`` builds the replacement table from the file's header + instead of cloning the current one, and it reads that header through the + same marker-aware reader. Exporting a strict subset of the columns proves + the header really drove the creation: the reloaded table has exactly the + columns the file named. + """ + searchfile = str(tmp_path / "s.txt") + name = filled_table.search_table + filled_table.copy_to(searchfile, columns=["n", "label"], include_id=False) + assert read(searchfile).startswith(EXPORT_FORMAT_MARKER) + + # resort=False because renumbering ids selects the *old* table's columns, + # which a narrowed schema no longer has; that is a limitation of + # adjust_schema, not of the header reader this test is about. + filled_table.reload(searchfile, adjust_schema=True, resort=False) + + # reload swaps in a new table, so the assertions must be made against the + # object the database hands out now, not the pre-reload one. + reloaded = db[name] + assert set(reloaded.search_cols) == {"n", "label"} + assert reloaded.count() == 200 + assert {r["n"] for r in reloaded.search({}, ["n"])} == set(range(200)) + assert reloaded.lucky({"n": 7}, "label") == "l7" + + +# --- the automatic reindex decision ---------------------------------------- +# +# copy_from reindexes when more than 1000 *rows* are added. A marked file's +# header is four lines and a format-0 file's is three, so a count of physical +# lines would put that boundary in a different place for each format. + +SIMPLE_COLUMNS = [("n", "integer"), ("label", "text")] + + +def write_search_file(path, nrows, marked=True, sep="|"): + with open(path, "w") as F: + if marked: + F.write(export_format_line() + "\n") + F.write(sep.join(col for col, _ in SIMPLE_COLUMNS) + "\n") + F.write(sep.join(typ for _, typ in SIMPLE_COLUMNS) + "\n\n") + for i in range(nrows): + F.write("%d%sl%d\n" % (i, sep, i)) + return str(path) + + +@pytest.mark.parametrize("marked", [True, False], ids=["marked", "legacy"]) +@pytest.mark.parametrize("nrows", [0, 1, 1000, 1001]) +def test_count_data_rows_ignores_the_header(empty_table, tmp_path, marked, nrows): + searchfile = write_search_file(tmp_path / "s.txt", nrows, marked=marked) + assert empty_table._count_data_rows(searchfile) == nrows + + +def test_count_data_rows_honors_a_custom_separator(empty_table, tmp_path): + searchfile = write_search_file(tmp_path / "s.txt", 3, sep=";") + assert empty_table._count_data_rows(searchfile, sep=";") == 3 + + +@pytest.mark.parametrize("marked", [True, False], ids=["marked", "legacy"]) +@pytest.mark.parametrize( + "nrows,expected", [(1000, False), (1001, True)], ids=["at-the-limit", "over-it"] +) +def test_copy_from_reindexes_above_1000_rows( + table_factory, tmp_path, monkeypatch, marked, nrows, expected +): """ - 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. + Whether the indexes were dropped is asked of ``drop_indexes`` itself: both + branches leave the same indexes in place afterward, so the final state of + the database cannot tell them apart. """ - 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) + target = table_factory(columns=SIMPLE_COLUMNS) + searchfile = write_search_file(tmp_path / "s.txt", nrows, marked=marked) + + dropped = [] + original = target.drop_indexes + + def spy(*args, **kwds): + dropped.append((args, kwds)) + return original(*args, **kwds) + + monkeypatch.setattr(target, "drop_indexes", spy) + target.copy_from(searchfile) + + assert bool(dropped) is expected + assert target.count() == nrows # --- helpers --------------------------------------------------------------- @@ -106,3 +322,10 @@ def filled_table_dump(table, tmp_path): searchfile = str(tmp_path / "s.txt") table.copy_to(searchfile, include_id=False) return searchfile + + +def rewrite_first_line(path, line): + lines = read(path).split("\n") + lines[0] = line + with open(path, "w") as F: + F.write("\n".join(lines)) From 7eb9eb73e716d71edbeacab1201e4f63fbea245e Mon Sep 17 00:00:00 2001 From: David Roe Date: Wed, 5 Aug 2026 04:01:12 -0400 Subject: [PATCH 7/9] Address review of the resort rebuild: audit CLI, order and stats bookkeeping Four correctness problems in the reviewed head, none of them changing the architecture the PR set out: resort() stays an explicit dump/rebuild/swap, row-level writes still reject resort=True, replacement writes may establish order while building their replacement, finalize_changes stays removed, and the audit stays read-only and streaming. 1. scripts/audit_id_order.py could not start: it imported a configured `db` object the package does not export. main() now builds a PostgresDatabase from the usual configuration, and takes one as an argument so the tests audit their own fixture connection instead of opening a second one. Its named-cursor read also left a transaction open, so a successful multi-table run held one MVCC snapshot until exit, and a table that errored left the connection in an aborted transaction -- every table after it then reported InFailedSqlTransaction rather than being audited. Each table now audits in a read transaction of its own, closed on every exit path (including the early mismatch return) without masking the audit's own error. id_ordered with no configured sort is an error rather than an OK obtained by ordering on id itself. 2. update_from_file overloaded `resort` for two different facts: whether the caller wants a rebuild, and whether the input touches a sort key. Since the sort-column check ran only when resort was None, an explicit resort=False suppressed the _break_order bookkeeping as well as the rebuild -- so an in-place or unrebuilt replacement update of a sort column, a rewrite(inplace=True) (whose default forces resort=False), and every staged write (the staged wrapper forces it too) left out_of_order = false standing. On a staged sort-key rewrite that false travelled through _staged_commit into the live meta_tables row, which search code is allowed to act on by replacing ORDER BY with ORDER BY id. sort_changed is now computed from the file's columns, independently of the option: resort=False means do not rebuild, not pretend the sort was untouched. rewrite(inplace=True, resort=True) raises before func runs over the table rather than after. 3. _generate_sorted_ids built its INSERT ... SELECT column list from self.search_cols, but under reload(adjust_schema=True) the _tmp relation comes from the file header: an added column was omitted from the copy and loaded as NULL, a removed one failed with an undefined column, and a metafile changing the sort got ids assigned by the sort it was replacing and then metadata claiming the new one. The helper now reads the target relation's own columns, takes the sort to number by as an argument (reload passes the one the metafile will install), and validates the relation has an id, has columns besides id, and has every sort column -- before the first destructive statement, so a file and a metafile that disagree leave the relation as it was. 4. update_from_file, rewrite and reload with restat=False swapped changed data in beside cloned, partial or untouched counts and stats while leaving stats_valid true, so a count cached before the write was still served after it -- defeating the enforcement added in the preceding PR. The decision now lives in one place, _set_stats_validity, called inside the same transaction as the update or the swap: valid only when the caches live were built from the data live with them (recomputed here, or supplied whole by an export carrying both countsfile and statsfile). A metafile no longer installs a stats_valid describing the database it came from. Regression tests for all four, in test_id_order_audit, test_write_invariants, test_staged_writes, test_resort and test_stats_validity; each new test was checked to fail against the reviewed head. Full suite green (1397 passed), ruff clean, docs build clean under -W. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 21 ++- DataManagement.md | 33 +++- psycodict/table.py | 275 ++++++++++++++++++++++++++------- scripts/audit_id_order.py | 92 ++++++++--- tests/test_id_order_audit.py | 164 ++++++++++++++++++++ tests/test_resort.py | 206 ++++++++++++++++++++++++ tests/test_staged_writes.py | 110 +++++++++++++ tests/test_stats_validity.py | 179 +++++++++++++++++++++ tests/test_write_invariants.py | 120 ++++++++++++++ 9 files changed, 1124 insertions(+), 76 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 85137f9..6269d91 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -433,9 +433,28 @@ hardening standalone use; the highlights: - **`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. +- **`resort=False` no longer suppresses the order bookkeeping.** Whether an + update can move the id order is now decided from the file's columns rather + than from the `resort` option, so an `update_from_file` or `rewrite` that + touches a sort column marks the table `out_of_order` even when it was told + not to rebuild — on the in-place path, on the replacement path, and through a + staged commit. Previously such a write could leave `out_of_order = false` + standing, an assertion search code is allowed to act on by replacing + `ORDER BY ` with `ORDER BY id`. *Migration:* none; run `resort()` on + tables the audit below reports as `MISMATCH`. +- **Every write that does not rebuild the caches clears `stats_valid`.** + `update_from_file`, `rewrite` and `reload` with `restat=False` (and any + replacement that installs cloned, partial or untouched counts and stats + beside changed data) now mark the table invalid, in the same transaction as + the swap. A `metafile` no longer installs a `stats_valid = true` describing + the database it was exported from. Without this a count cached before such a + write was still served afterwards, defeating the enforcement above. - **`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. + increase, since the flag can drift. Each table is audited in a read + transaction of its own, so one table's error does not poison the rest of the + run, and `id_ordered` with no configured sort is reported as an error rather + than trivially passing. ### Release candidates diff --git a/DataManagement.md b/DataManagement.md index 4b118b2..c0a8d90 100644 --- a/DataManagement.md +++ b/DataManagement.md @@ -137,6 +137,18 @@ the transaction that rebuilt the caches, so a refresh that fails part-way leaves the table marked invalid rather than claiming a cache it does not have. Refreshing a `_tmp` copy does not validate the live table. +The replacement paths — `reload`, `rewrite`, a non-inplace `update_from_file`, +and the staged swap — decide the flag in one place, in the same transaction as +the swap. The table ends up valid only when the counts and stats that go live +were built from the data that goes live with them: either recomputed here +(`restat`, on a table with `saving` on) or supplied whole by the export +(`reload` given **both** `countsfile` and `statsfile`). Anything else — a +`restat=False` write, a partial cache set, companions that were merely cloned +or left in place beside replaced rows — is marked invalid. A `metafile` +carries a `stats_valid` of its own describing the database it was exported +from; it does not vouch for caches the reload did not install, and is +overridden to false with a message when it disagrees. + Bulk paths also run PostgreSQL's own `ANALYZE`, which is a different thing from psycodict's statistics: a freshly loaded relation has no planner statistics until autovacuum reaches it. Replacement tables are analyzed while still named @@ -175,11 +187,26 @@ table.update(...) # marks out_of_order table.resort() # rebuild in sort order ``` +`resort=False` says *do not rebuild*; it does not say the sort was left alone. +Whether an update can move the ids is decided from the file — does it carry a +sort column? — independently of the option, so a write that touches one always +records `out_of_order = true`, on the in-place path and on the replacement path +alike. A `rewrite` runs an arbitrary function and dumps every search column, so +one that is not rebuilt always marks the table out of order; there is no way to +tell that it did not move a sort key. A staged write is not allowed to rebuild +inside the staged context, but it carries that fact through the commit, so the +swapped-in table is marked out of order and `resort()` afterwards puts it right. + `scripts/audit_id_order.py` checks the flag against reality: it streams each `id_ordered` table in sort order and reports `OK`, `MISMATCH` or an error -without modifying anything. Use it before trusting `out_of_order = false` on a -table whose history includes non-inplace updates, which could have left the flag -stale. +without modifying anything. Each table is audited in a read transaction of its +own, so one that errors does not affect the tables audited after it. Use it +before trusting `out_of_order = false` on a table whose history includes +non-inplace updates, which could have left the flag stale. + +```bash +python scripts/audit_id_order.py --all +``` ### Locking diff --git a/psycodict/table.py b/psycodict/table.py index 8e1be52..538e9ce 100644 --- a/psycodict/table.py +++ b/psycodict/table.py @@ -12,6 +12,7 @@ which is what ``db.`` actually returns. """ import csv +import json import os import tempfile import time @@ -1199,6 +1200,28 @@ def _restore_stats(self): self._execute(updater, [self.search_table], silent=True) self._stats_valid = True + def _set_stats_validity(self, fresh): + """ + Record whether the counts and statistics now live were built from the + data now live. + + The one place the replacement paths -- ``reload``, the non-inplace + ``update_from_file`` and ``rewrite``, and the staged swap -- decide the + flag, so that they cannot drift apart on it. Call it inside the + transaction that does the swap: a failure part way through then leaves + the flag as it was rather than describing companions that never + arrived. + + Unlike :meth:`_break_stats` and :meth:`_restore_stats` this writes + unconditionally rather than trusting the cached flag, because a + metafile import replaces the whole meta_tables row (``stats_valid`` + included) behind this object's back. + """ + fresh = bool(fresh) + updater = SQL("UPDATE meta_tables SET stats_valid = %s WHERE name = %s") + self._execute(updater, [fresh, self.search_table], silent=True) + self._stats_valid = fresh + def _break_order(self): """ This function should be called when the id ordering is invalidated by an insertion or update. @@ -1280,6 +1303,10 @@ def rewrite( Defaults to resorting a non-inplace (replacement) rewrite and not an in-place one; ``resort=True`` on an in-place rewrite raises, since the in-place path cannot resort (call ``resort()`` afterward). + ``func`` may return anything, and the dump covers every search + column, so a rewrite that is not resorted always leaves the table + marked ``out_of_order``; there is no way to tell that it did not + move a sort key. - ``restat`` -- whether to recompute statistics after running the rewrite - ``tostr_func`` -- a function to be used when writing data to the temp file defaults to copy_dumps from encoding @@ -1304,9 +1331,12 @@ def rewrite( 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, so do not ask it to. resort = not kwds.get("inplace", False) + elif kwds.get("inplace"): + # update_from_file would reject this too, but only after func has + # been run over every row and the result dumped to disk. + self._forbid_row_level_resort(resort, "rewrite with inplace=True") sep = kwds.get("sep", "|") # An unusable separator would otherwise be rejected only by COPY itself, # after func has already been run over every row of the table. @@ -1403,7 +1433,7 @@ def update_from_file( - ``datafile`` -- a file with header lines (unlike ``reload``, does not need to include all columns) and rows containing data to be updated. - ``label_col`` -- a column specifying which row(s) of the table should be updated corresponding to each row of the input file. This will usually be the label for the table, in which case it can be omitted. - ``inplace`` -- whether to do the update in place. If set, the operation cannot be undone with ``reload_revert``. - - ``resort`` -- whether this table should be resorted after updating (default is to resort when the sort columns intersect the updated columns) + - ``resort`` -- whether the replacement table should be built with its ids renumbered in sort order (default is to renumber when the updated columns intersect the sort columns). Only the replacement path can do this, so ``resort=True`` with ``inplace`` raises. ``resort=False`` says not to renumber, not that the sort was left alone: an update that touches a sort column marks the table ``out_of_order`` either way, so that ``resort()`` can put it right later. - ``reindex`` -- only meaningful when ``inplace`` is set: whether to drop the indexes touching the updated columns before the update and recreate them afterward, which is faster when many rows change (by default this is done when more than 1000 rows are updated). Without ``inplace``, all indexes are necessarily recreated on the replacement table, so ``reindex=True`` is redundant and ``reindex=False`` raises an error. - ``restat`` -- whether to recompute stats for the table - ``logging`` -- a dictionary of keyword arguments for _log_db_change. @@ -1454,8 +1484,16 @@ def update_from_file( for line in F: rowcount += 1 reindex = rowcount > 1000 + # Two separate facts, which one variable used to conflate. Whether + # the input can change the id ordering is a property of the file; + # whether to rebuild the table in sort order is the caller's + # choice. Deriving the first from the second made resort=False + # mean "the sort was not touched" as well as "do not rebuild", so + # a write that reordered the table left out_of_order = false + # behind -- an assertion search code is allowed to act on. + sort_changed = bool(set(columns[1:]).intersection(self._sort_keys)) if resort is None: - resort = bool(set(columns[1:]).intersection(self._sort_keys)) + resort = sort_changed # Create a temp table to hold the data tmp_table = "tmp_update_from_file" @@ -1513,14 +1551,17 @@ def drop_tmp(): # 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. + # front). Whenever the rows are not renumbered and the file + # could have moved a sort key, the ordering is recorded as + # broken instead -- including on the non-inplace path, where + # the flag set here survives the swap below, which only ever + # clears it via _set_ordered. if not inplace and self._id_ordered and resort: self._generate_sorted_ids(suffix=suffix) ordered = True else: ordered = False - if inplace and resort and self._id_ordered: + if sort_changed: self._break_order() if reindex and inplace: # also restores constraints @@ -1530,7 +1571,14 @@ def drop_tmp(): self.restore_indexes(suffix=suffix) # We also need to recreate the primary key self.restore_pkeys(suffix=suffix) - if restat and self.stats.saving: + # The only way this write ends with caches describing its own + # data: recompute them, in place against the live table or on + # the _tmp companions swapped in beside the replacement below. + # With restat off, or on a table that does not save + # statistics, whatever is cached was computed from the rows as + # they were before this write. + refreshed = bool(restat and self.stats.saving) + if refreshed: if not inplace: for table in [self.stats.counts, self.stats.stats]: if not self._table_exists(physical_table_name(table, lifecycle="_tmp")): @@ -1545,7 +1593,7 @@ def drop_tmp(): # and the _tmp tables are left orphaned. reload builds its # swap list the same way. tables = [self.search_table] - if restat and self.stats.saving: + if refreshed: tables += [self.stats.counts, self.stats.stats] if self.stats.counts in tables: # _clone built the _tmp counts table with a bare LIKE, @@ -1557,6 +1605,9 @@ def drop_tmp(): self._swap_in_tmp(tables) if ordered: self._set_ordered() + # In the same transaction as the update or the swap, so that + # the flag and the data it describes move together. + self._set_stats_validity(refreshed) # Delete the temporary table used to load the data drop_tmp() log_data["logid"] = logid @@ -1815,10 +1866,10 @@ 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 _generate_sorted_ids(self, suffix=""): + def _generate_sorted_ids(self, suffix="", sort=None): """ Rebuild ``{search}{suffix}`` so that ascending ``id`` follows the - configured sort, laying the rows out on disk in that order too. + intended sort, laying the rows out on disk in that order too. This is a physical rebuild, not an in-place ``UPDATE`` of every id: the rows are copied into a fresh table in sort order, with ids drawn in @@ -1830,18 +1881,64 @@ def _generate_sorted_ids(self, suffix=""): 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. + entry point is :meth:`resort`. + + INPUT: + + - ``suffix`` -- names the relation to rebuild, ``{search}{suffix}``. + - ``sort`` -- the sort to number the rows by, in the form ``set_sort`` + takes. Defaults to this table's configured sort. A reload that + installs a metafile passes the sort *that file* will install: ids + assigned by the sort being replaced would not describe the sort the + table ends up with, and the reload would then mark it ordered. + + The relation being rebuilt is not necessarily described by this table + object. Under ``reload(adjust_schema=True)`` the ``_tmp`` relation is + built from the data file's header, so it may have gained or lost + columns relative to ``self.search_cols``; its own columns are read + from the catalog rather than assumed, and everything is checked before + the first destructive statement. """ target = self.search_table + suffix - if self._sort is None: + sort = self._sort_orig if sort is None else sort + if not sort: raise ValueError( "Cannot resort %s: it has no configured sort" % (self.search_table,) ) + order = self._sort_str(sort) + cols, _, has_id = self._column_types(target) + if not has_id: + raise ValueError( + "Cannot resort %s: it has no id column to renumber " + "(does the relation exist?)" % (target,) + ) + if not cols: + raise ValueError( + "Cannot resort %s: it has no columns besides id, so there is " + "nothing to sort it by" % (target,) + ) + missing = sorted( + {col if isinstance(col, str) else col[0] for col in sort}.difference(cols) + ) + if missing: + # Checked here, before anything is dropped, so that a data file + # and a metafile that disagree about the schema leave the + # relation as it was rather than half rebuilt. + raise ValueError( + "Cannot resort %s by %s: the relation has no column%s %s. The " + "imported schema and the sort being installed do not agree." + % ( + target, + sort, + "" if len(missing) == 1 else "s", + ", ".join(missing), + ) + ) # Transient, named nowhere else, so shortened to fit rather than left # for the server to truncate. resorting = derived_identifier(target, "_resort") seq = derived_identifier(target, "_resort_seq") - search_cols = SQL(", ").join(Identifier(c) for c in self.search_cols) + target_cols = SQL(", ").join(Identifier(c) for c in cols) # A fresh table with the same columns and storage but no indexes. self._clone(target, resorting) try: @@ -1858,10 +1955,10 @@ def _generate_sorted_ids(self, suffix=""): "SELECT nextval({2}), {1} FROM {3} ORDER BY {4}, id" ).format( Identifier(resorting), - search_cols, + target_cols, Literal(seq), Identifier(target), - self._sort, + order, ) ) self._execute(SQL("DROP SEQUENCE {0}").format(Identifier(seq))) @@ -2100,6 +2197,62 @@ def _swap_in_tmp(self, tables): "to save disc space" ).format(self.search_table, backup_number)) + def _metafile_order_state(self, metafile, sep="|"): + """ + The ordering metadata a metafile will install: ``(id_ordered, + out_of_order, sort)``. + + A reload that carries a metafile replaces this table's sort with the + file's, so the renumbering it does has to follow *that* sort; reading + it here, before anything is rebuilt, is what lets + ``_generate_sorted_ids`` number the rows for the sort the table ends + up with rather than the one it is losing. The file is validated as + strictly as ``_reload_meta`` validates it later. + + INPUT: + + - ``metafile`` -- a file holding this table's meta_tables row + - ``sep`` -- the column separator it was written with + """ + meta_name = "meta_tables" + meta_cols, _, _ = _meta_cols_types_jsonb_idx(meta_name) + # the column which will match search_table + table_name = _meta_table_name(meta_name) + table_name_idx = meta_cols.index(table_name) + with open(metafile, "r") as F: + lines = list(csv.reader(F, delimiter=str(sep))) + if len(lines) != 1: + raise RuntimeError("%s has more than one line" % (metafile,)) + line = lines[0] + if line[table_name_idx] != self.search_table: + raise RuntimeError( + f"column {table_name_idx} (= {line[table_name_idx]}) " + f"in the file {metafile} doesn't match " + f"the search table name {self.search_table}" + ) + flags = {} + for col in ["id_ordered", "out_of_order"]: + idx = meta_cols.index(col) + if line[idx] not in ['t', 'f']: + raise RuntimeError( + f"column {idx} (= {line[idx]}) " + f"in the file {metafile} is different from 't' or 'f'" + ) + flags[col] = line[idx] == 't' + sort_idx = meta_cols.index("sort") + raw = line[sort_idx] + if raw == r"\N": # COPY's null marker: a table with no sort at all + sort = None + else: + try: + sort = json.loads(raw) + except ValueError as err: + raise RuntimeError( + f"column {sort_idx} (= {raw}) in the file {metafile} " + f"is not valid JSON: {err}" + ) + return flags["id_ordered"], flags["out_of_order"], sort + def _check_file_input(self, searchfile, kwds): """ Utility function for validating the inputs to ``rewrite``, ``reload`` and ``copy_from``. @@ -2339,44 +2492,21 @@ def reload( # Renumber ids in sort order before the keys are built: the # renumber replaces the _tmp table, so it must run while that # table still has no primary key or indexes to rebuild. + resort_sort = None if resort: if metafile: - # read the metafile - # using code from _reload_meta - meta_name = 'meta_tables' - meta_cols, _, _ = _meta_cols_types_jsonb_idx(meta_name) - # the column which will match search_table - table_name = _meta_table_name(meta_name) - table_name_idx = meta_cols.index(table_name) - with open(metafile, "r") as F: - lines = list(csv.reader(F, delimiter=str(sep))) - if len(lines) != 1: - raise RuntimeError( - "%s has more than one line" % (metafile,) - ) - line = lines[0] - if line[table_name_idx] != self.search_table: - raise RuntimeError( - f"column {table_name_idx} (= {line[table_name_idx]}) " - f"in the file {metafile} doesn't match " - f"the search table name {self.search_table}" - ) - for col in ["id_ordered", "out_of_order"]: - idx = meta_cols.index(col) - if line[idx] not in ['t', 'f']: - raise RuntimeError( - f"column {idx} (= {line[idx]}) " - f"in the file {metafile} is different from 't' or 'f'" - ) - resort = ( - line[meta_cols.index("id_ordered")] == 't' - and line[meta_cols.index("out_of_order")] == 'f' - ) - else: - if not self._id_ordered: # this table doesn't need to be sorted - resort = False + id_ordered, out_of_order, resort_sort = self._metafile_order_state( + metafile, sep=sep + ) + resort = id_ordered and not out_of_order + # resort_sort, not the sort currently configured: the + # metafile replaces that one, and numbering the rows by + # a sort the table is about to stop having would install + # ids that satisfy nothing while claiming to be ordered. + elif not self._id_ordered: # this table doesn't need to be sorted + resort = False if resort: - self._generate_sorted_ids(suffix=suffix) + self._generate_sorted_ids(suffix=suffix, sort=resort_sort) ordered = True else: ordered = False @@ -2411,10 +2541,21 @@ def reload( # create index on counts table self._create_counts_indexes(suffix=suffix) + # The replacement's caches agree with its data in exactly two + # cases: the export supplied both of them alongside the search + # file, or they were recomputed here from what was loaded. + # Otherwise the counts and stats that go live are the ones + # already there (untouched, since they are not in ``tables``) + # or an empty or reused ``_tmp`` clone -- in neither case a + # description of the rows now in the table. + stats_fresh = (countsfile is not None and statsfile is not None) or ( + self.stats.saving and restat + ) if final_swap: self.reload_final_swap(tables=tables, metafile=metafile, - ordered=ordered) + ordered=ordered, + stats_fresh=stats_fresh) elif metafile is not None and not silence_meta: print("Warning: since the final swap was not requested, we have not updated meta_tables") print("when performing the final swap with reload_final_swap, pass the metafile as an argument to update the meta_tables") @@ -2429,7 +2570,7 @@ def reload( stats=(statsfile is not None), ) - def reload_final_swap(self, tables=None, metafile=None, ordered=False, sep="|"): + def reload_final_swap(self, tables=None, metafile=None, ordered=False, sep="|", stats_fresh=False): """ Renames the ``_tmp`` versions of ``tables`` to the live versions, and updates the corresponding meta_tables row if ``metafile`` is provided. @@ -2439,6 +2580,13 @@ def reload_final_swap(self, tables=None, metafile=None, ordered=False, sep="|"): - ``tables`` -- list of strings (optional), of the tables to be renamed. If None is provided, renames all the tables ending in ``_tmp`` - ``metafile`` -- a string (optional), giving a file containing the meta information for the table. - ``sep`` -- a character (default ``|``) to separate columns + - ``ordered`` -- whether ascending id follows the sort in the tables + being swapped in, so that the live table may be marked ordered. + - ``stats_fresh`` -- whether the counts and statistics being swapped in + were built from the data being swapped in. Defaults to False, the + safe answer for a swap finished by hand: a cached count that + describes the previous data is a wrong answer, whereas an + unnecessary ``false`` only costs a ``refresh_stats()``. """ with DelayCommit(self, silence=True): if tables is None: @@ -2454,8 +2602,23 @@ def reload_final_swap(self, tables=None, metafile=None, ordered=False, sep="|"): self._swap_in_tmp(tables) if metafile is not None: self.reload_meta(metafile, sep=sep) + if not stats_fresh: + # A metafile carries the stats_valid of the database it was + # exported from, which says nothing about whether this + # reload installed matching caches. Importing it as-is + # would let a true written elsewhere vouch for counts that + # were never rebuilt here. + print( + "Marking %s's cached statistics invalid: this reload " + "did not install counts and statistics built from the " + "data it loaded, whatever %s says. Recompute them " + "with db.%s.stats.refresh_stats()." + % (self.search_table, metafile, self.search_table) + ) if ordered: self._set_ordered() + # After reload_meta, which replaces the whole meta_tables row + self._set_stats_validity(stats_fresh) # The swapped-in table's row count bears no relation to the old # total, so recount and store it before the reinitialization # below reads meta_tables. @@ -2815,7 +2978,11 @@ def update_from_file(datafile, label_col=None, inplace=True, **kwds): # 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(). + # table after the commit, with resort(). Dropping the request + # does not hide the fact that the ordering moved: whether the + # file touches a sort column is decided from the file, so the + # write still records it in staged._out_of_order, which the + # commit transfers to the live table. kwds.pop("resort", None) return unstaged_update_from_file( datafile, label_col=label_col, inplace=True, resort=False, **kwds diff --git a/scripts/audit_id_order.py b/scripts/audit_id_order.py index c6b65e5..075ab2b 100644 --- a/scripts/audit_id_order.py +++ b/scripts/audit_id_order.py @@ -24,11 +24,15 @@ 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. +the table. Each table is audited in a read transaction of its own, so one +snapshot is not held open across the whole run and a table that errors does not +carry that error into the tables audited after it. """ import argparse +import contextlib import sys +import psycopg from psycopg.sql import SQL, Identifier @@ -37,13 +41,14 @@ def _server_side_rows(table): Stream ``(id,)`` for every row of ``table`` in configured-sort order plus ``id`` as the final tie-breaker, using a named cursor so the whole table is never materialized in the client. + + ``table._sort`` must be set; :func:`audit_table` has already checked. The + caller owns the read transaction the named cursor runs in and must close + this generator, so that the ``finally`` below closes the cursor even when + the audit stops early on a mismatch. """ db = table._db - 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) + clause = SQL("ORDER BY {0}, id").format(table._sort) query = SQL("SELECT id FROM {0} {1}").format(Identifier(table.search_table), clause) # A server-side (named) cursor keeps the sort on the server and hands back # rows in batches; see PostgresBase._cursor(buffered=True). @@ -57,36 +62,81 @@ def _server_side_rows(table): for row in batch: yield row[0] finally: - cur.close() + # CLOSE is itself a statement, so it fails when the audit failed and + # left the transaction aborted. The audit's own error is the one + # worth reporting, and the cursor dies with the transaction that + # audit_table is about to end regardless. + with contextlib.suppress(psycopg.Error): + cur.close() + + +def _end_read_transaction(db): + """ + Finish the transaction the audit read in, leaving the connection idle. + + The audit writes nothing, so rolling back is as good as committing, and + unlike a commit it also works after a failed statement: without this, one + table's error would leave the connection in an aborted transaction and + every table audited afterwards would fail with ``InFailedSqlTransaction`` + instead of being audited. Ending the transaction also stops a long + multi-table run from holding a single MVCC snapshot open from the first + table to process exit. + + Never raises: a failure to clean up must not replace the audit result the + caller is about to return. + """ + with contextlib.suppress(psycopg.Error): + db.conn.rollback() def audit_table(db, name): """ Return ``("OK", n)``, ``("MISMATCH", detail)`` or ``("ERROR", message)`` for one table, reading it in sort order without modifying anything. + + Each call runs in a read transaction of its own which is finished on every + exit path, so that tables are audited independently of each other. """ if name not in db.tablenames: return ("ERROR", "no such table") table = db[name] if not table._id_ordered: return ("ERROR", "not id_ordered") + if table._sort is None: + # Falling back to "ORDER BY id" here would report every such table as + # OK by construction. id_ordered with no sort does not describe an + # invariant, so there is nothing this script can confirm. + return ("ERROR", "id_ordered but no sort configured") prev = None count = 0 try: - 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 + # closing() so that returning early on a mismatch runs the generator's + # finally and closes the named cursor there and then, rather than + # leaving it open until the generator is garbage collected. + with contextlib.closing(_server_side_rows(table)) as rows: + for rid in rows: + if prev is not None and rid <= prev: + return ( + "MISMATCH", + "id %s follows id %s in sort order (row %s)" % (rid, prev, count + 1), + ) + prev = rid + count += 1 + except Exception as err: return ("ERROR", "%s: %s" % (type(err).__name__, err)) + finally: + _end_read_transaction(db) return ("OK", count) -def main(argv=None): +def main(argv=None, db=None): + """ + Run the audit over the tables named on the command line. + + ``db`` is the database to audit; when it is None (the command-line case) + one is opened from the usual configuration. The tests pass their own + fixture database rather than opening a second connection to it. + """ parser = argparse.ArgumentParser(description=__doc__.splitlines()[1]) group = parser.add_mutually_exclusive_group(required=True) group.add_argument( @@ -95,7 +145,13 @@ def main(argv=None): group.add_argument("tables", nargs="*", default=[], help="tables to audit") args = parser.parse_args(argv) - from psycodict import db + if db is None: + # The package exports no ready-made database object, so build one the + # supported way: PostgresDatabase() picks up $PSYCODICT_CONFIG or + # config.ini exactly as an application using psycodict would. + from psycodict.database import PostgresDatabase + + db = PostgresDatabase() if args.all: names = sorted( diff --git a/tests/test_id_order_audit.py b/tests/test_id_order_audit.py index 9d2dcdf..ea65d30 100644 --- a/tests/test_id_order_audit.py +++ b/tests/test_id_order_audit.py @@ -12,11 +12,19 @@ import pytest +import psycopg from psycopg.sql import SQL, Identifier from conftest import sample_row +def _idle(db): + """ + Whether the connection is between transactions rather than inside one. + """ + return db.conn.info.transaction_status == psycopg.pq.TransactionStatus.IDLE + + # Load the script as a module, since scripts/ is not a package. _AUDIT_PATH = pathlib.Path(__file__).resolve().parent.parent / "scripts" / "audit_id_order.py" _spec = importlib.util.spec_from_file_location("audit_id_order", _AUDIT_PATH) @@ -88,3 +96,159 @@ def test_audit_on_an_empty_ordered_table(db, table_factory): table = table_factory(sort=["n"]) status, detail = audit.audit_table(db, table.search_table) assert status == "OK" and detail == 0 + + +def test_audit_errors_when_id_ordered_but_no_sort(db, table_factory): + """ + ``id_ordered`` with no sort configured cannot be audited: ordering by id + itself would call every such table OK by construction, which asserts + nothing. It is a broken metadata state, so say so. + """ + table = table_factory(sort=None, id_ordered=True) + table.insert_many([sample_row(i) for i in range(5)]) + assert table._id_ordered and table._sort is None + + status, detail = audit.audit_table(db, table.search_table) + assert status == "ERROR" + assert "sort" in detail + + +def _break_the_sort_under_the_metadata(table): + """ + Drop the column a table is sorted by, behind psycodict's back, leaving + meta_tables still naming it in the sort. + + That is the shape of failure the audit meets in the wild -- metadata that + no longer describes the database -- and it makes the generated ``SELECT + ... ORDER BY`` fail on the server, aborting the audit's transaction. + """ + table._execute( + SQL("ALTER TABLE {0} DROP COLUMN {1}").format( + Identifier(table.search_table), Identifier("n") + ) + ) + table._db.conn.commit() + + +def test_an_audit_error_does_not_poison_the_tables_audited_after_it(db, table_factory): + """ + A failed audit must end its own transaction. Otherwise the connection + stays in an aborted transaction and every later table reports + InFailedSqlTransaction instead of being audited -- so a single broken + table would turn a whole ``--all`` run into a wall of false errors. + """ + broken = table_factory(sort=["n"]) + broken.insert_many([sample_row(i) for i in range(5)]) + healthy = table_factory(sort=["n"]) + healthy.insert_many([sample_row(i) for i in range(5)]) + healthy.resort() + _break_the_sort_under_the_metadata(broken) + + status, detail = audit.audit_table(db, broken.search_table) + assert status == "ERROR" + assert "InFailedSqlTransaction" not in detail + + status, detail = audit.audit_table(db, healthy.search_table) + assert (status, detail) == ("OK", 5) + + +def test_the_connection_is_idle_after_every_kind_of_result(db, table_factory, ordered_table): + """ + Each table is audited in a bounded read transaction, whatever the result, + so the next table starts from a fresh snapshot rather than inheriting a + transaction opened by the previous one. + """ + assert audit.audit_table(db, ordered_table.search_table)[0] == "OK" + assert _idle(db) + + mismatched = table_factory(sort=["n"]) + mismatched.insert_many([sample_row(i) for i in range(10)]) + mismatched._execute( + SQL("UPDATE {0} SET id = 100 - n").format(Identifier(mismatched.search_table)) + ) + mismatched._db.conn.commit() + assert audit.audit_table(db, mismatched.search_table)[0] == "MISMATCH" + assert _idle(db) + + broken = table_factory(sort=["n"]) + broken.insert_many([sample_row(i) for i in range(3)]) + _break_the_sort_under_the_metadata(broken) + assert audit.audit_table(db, broken.search_table)[0] == "ERROR" + assert _idle(db) + + +def test_audit_streams_through_a_server_side_cursor_and_closes_it(db, ordered_table, monkeypatch): + """ + The audit reads whole tables, so it must keep the sort and the result set + on the server: a client-side cursor would pull every row of a production + table into memory. The cursor must also be closed once the audit is done + with it, including when it stops early. + """ + handed_out = [] + real_cursor = db._cursor + + def spy(buffered=False): + cur = real_cursor(buffered=buffered) + handed_out.append((buffered, cur)) + return cur + + monkeypatch.setattr(db, "_cursor", spy) + + assert audit.audit_table(db, ordered_table.search_table)[0] == "OK" + assert handed_out, "the audit did not open a cursor" + assert all(buffered for buffered, _ in handed_out) + # A named cursor is what makes it server-side; see _cursor(buffered=True). + assert all(cur.name for _, cur in handed_out) + assert all(cur.closed for _, cur in handed_out) + + +def test_mismatch_closes_the_cursor_before_returning(db, table_factory, monkeypatch): + """ + The mismatch return leaves the streaming generator part way through, so + the cursor is only closed if the audit closes it deliberately. + """ + table = table_factory(sort=["n"]) + table.insert_many([sample_row(i) for i in range(10)]) + table._execute( + SQL("UPDATE {0} SET id = 100 - n").format(Identifier(table.search_table)) + ) + table._db.conn.commit() + + handed_out = [] + real_cursor = db._cursor + + def spy(buffered=False): + cur = real_cursor(buffered=buffered) + handed_out.append(cur) + return cur + + monkeypatch.setattr(db, "_cursor", spy) + + assert audit.audit_table(db, table.search_table)[0] == "MISMATCH" + assert handed_out and all(cur.closed for cur in handed_out) + + +def test_main_audits_the_database_it_is_given(db, ordered_table, capsys): + """ + The documented command line has to actually run. ``main`` takes the + database as an argument so the test can hand it the fixture connection; + without one it builds a PostgresDatabase from the usual configuration. + """ + rc = audit.main([ordered_table.search_table], db=db) + out = capsys.readouterr().out + assert rc == 0 + assert "OK" in out + assert ordered_table.search_table in out + + +def test_main_reports_a_mismatch_with_a_nonzero_status(db, table_factory, capsys): + table = table_factory(sort=["n"]) + table.insert_many([sample_row(i) for i in range(10)]) + table._execute( + SQL("UPDATE {0} SET id = 100 - n").format(Identifier(table.search_table)) + ) + table._db.conn.commit() + + rc = audit.main([table.search_table], db=db) + assert rc == 1 + assert "MISMATCH" in capsys.readouterr().out diff --git a/tests/test_resort.py b/tests/test_resort.py index aa3c9c0..7e59e8c 100644 --- a/tests/test_resort.py +++ b/tests/test_resort.py @@ -173,3 +173,209 @@ def test_reload_orders_a_no_id_file(table_factory, tmp_path): by_id = [rec["n"] for rec in table.search({}, ["n"], sort=[["id", 1]])] assert by_id == sorted(by_id) assert table._out_of_order is False + + +# --------------------------------------------------------------------------- +# the rebuild follows the relation it is actually rebuilding +# +# Under reload(adjust_schema=True) the _tmp relation is built from the data +# file's header, so it need not have the columns of the live table object. +# --------------------------------------------------------------------------- + +def _reload_file(path, header, types, rows, sep="|"): + """ + A reload search file: names, types, a blank line, then the rows. + """ + with open(path, "w") as F: + F.write(sep.join(header) + "\n") + F.write(sep.join(types) + "\n\n") + for row in rows: + F.write(sep.join(row) + "\n") + return str(path) + + +ROW_ORDER = [3, 1, 4, 0, 2] + + +@pytest.fixture +def two_col_table(table_factory): + """ + A small id_ordered table sorted by ``n``, with rows loaded out of order. + """ + table = table_factory(columns=[("n", "integer"), ("label", "text")], sort=["n"]) + table.insert_many([{"n": i, "label": "l%d" % i} for i in ROW_ORDER]) + return table + + +@pytest.fixture +def three_col_table(table_factory): + """ + The same, plus an ``extra`` column running against ``n``. + """ + table = table_factory( + columns=[("n", "integer"), ("label", "text"), ("extra", "text")], sort=["n"] + ) + table.insert_many( + [{"n": i, "label": "l%d" % i, "extra": "e%d" % (4 - i)} for i in ROW_ORDER] + ) + return table + + +def test_adjusted_schema_reload_preserves_an_added_column(db, two_col_table, tmp_path): + """ + A column the file adds exists in the _tmp relation but not in the live + table object. Copying the rows by the object's columns dropped it from + the INSERT, so every value loaded from the file came back NULL. + """ + name = two_col_table.search_table + path = _reload_file( + tmp_path / "d.txt", + ["n", "label", "extra"], + ["integer", "text", "text"], + [[str(i), "l%d" % i, "e%d" % i] for i in ROW_ORDER], + ) + two_col_table.reload(path, adjust_schema=True, resort=True) + + table = db[name] + rows = list(table.search({}, projection=["n", "label", "extra"], sort=[["id", 1]])) + assert [rec["n"] for rec in rows] == [0, 1, 2, 3, 4] + assert [rec["extra"] for rec in rows] == ["e0", "e1", "e2", "e3", "e4"] + + +def test_adjusted_schema_reload_can_drop_a_column(db, three_col_table, tmp_path): + """ + And the other direction: a column the file leaves out is gone from the + _tmp relation, so naming it in the copy failed with an undefined column. + """ + name = three_col_table.search_table + path = _reload_file( + tmp_path / "d.txt", + ["n", "label"], + ["integer", "text"], + [[str(i), "l%d" % i] for i in ROW_ORDER], + ) + three_col_table.reload(path, adjust_schema=True, resort=True) + + table = db[name] + assert "extra" not in table.search_cols + rows = list(table.search({}, projection=["n", "label"], sort=[["id", 1]])) + assert [rec["n"] for rec in rows] == [0, 1, 2, 3, 4] + assert [rec["label"] for rec in rows] == ["l0", "l1", "l2", "l3", "l4"] + + +def _metafile_with_sort(table, tmp_path, sort_json, name="meta.txt"): + """ + This table's metafile with its ``sort`` column replaced, still claiming + to be ordered. + """ + from psycodict.base import _meta_tables_cols + + path = tmp_path / name + table.copy_to_meta(str(path)) + with open(path) as F: + fields = F.read().rstrip("\n").split("|") + fields[_meta_tables_cols.index("sort")] = sort_json + fields[_meta_tables_cols.index("id_ordered")] = "t" + fields[_meta_tables_cols.index("out_of_order")] = "f" + with open(path, "w") as F: + F.write("|".join(fields) + "\n") + return str(path) + + +def test_a_metafile_reload_numbers_by_the_sort_it_installs(db, two_col_table, tmp_path): + """ + A reload carrying a metafile replaces the sort. Numbering the rows by the + sort being replaced and then installing the new one asserts an ordering the + ids do not have -- the metadata would claim ORDER BY id is ORDER BY extra + while the ids in fact followed n. + """ + name = two_col_table.search_table + path = _reload_file( + tmp_path / "d.txt", + ["n", "label", "extra"], + ["integer", "text", "text"], + # extra runs against n, so ordering by one is visibly not the other + [[str(i), "l%d" % i, "e%d" % (4 - i)] for i in ROW_ORDER], + ) + metafile = _metafile_with_sort(two_col_table, tmp_path, '["extra"]') + two_col_table.reload(path, metafile=metafile, adjust_schema=True, resort=True) + + table = db[name] + assert table._sort_orig == ["extra"] + rows = list(table.search({}, projection=["n", "extra"], sort=[["id", 1]])) + # the added column survived the rebuild ... + assert [rec["extra"] for rec in rows] == ["e0", "e1", "e2", "e3", "e4"] + # ... and the ids follow the new sort, not the old one + assert [rec["n"] for rec in rows] == [4, 3, 2, 1, 0] + assert table._out_of_order is False + + from test_id_order_audit import audit + + assert audit.audit_table(db, name)[0] == "OK" + + +def test_a_metafile_reload_refuses_a_sort_the_imported_schema_lacks(db, three_col_table, tmp_path): + """ + If the file and the metafile disagree -- the sort names a column the + imported schema does not have -- say so before anything is dropped, rather + than failing part way through the rebuild. + """ + name = three_col_table.search_table + before = list(three_col_table.search({}, projection=["n", "extra"], sort=[["n", 1]])) + path = _reload_file( + tmp_path / "d.txt", + ["n", "label"], + ["integer", "text"], + [[str(i), "l%d" % i] for i in ROW_ORDER], + ) + metafile = _metafile_with_sort(three_col_table, tmp_path, '["extra"]') + with pytest.raises(ValueError, match="extra"): + three_col_table.reload(path, metafile=metafile, adjust_schema=True, resort=True) + + # The live table is untouched, and no half-built relation is left behind. + table = db[name] + assert list(table.search({}, projection=["n", "extra"], sort=[["n", 1]])) == before + assert not table._table_exists(name + "_tmp") + + +def test_resort_refuses_a_relation_with_no_columns_besides_id(db, table_factory): + """ + An id-only table has nothing to sort by. Building the copy from an empty + column list would emit ``INSERT INTO t (id, ) SELECT ...``; refuse it with + an explanation instead. + """ + table = table_factory(columns=[("n", "integer")], label_col=None, sort=None) + table.drop_column("n", force=True) + assert table.search_cols == [] + + with pytest.raises(ValueError, match="no columns besides id"): + table._generate_sorted_ids(sort=["n"]) + # and through the public entry point it never gets that far + with pytest.raises(ValueError, match="id_ordered"): + table.resort() + + +def test_a_metafile_sort_may_be_a_direction_pair(db, two_col_table, tmp_path): + """ + The sort arrives from the metafile as JSON, so a ``(column, -1)`` pair + comes back as a two-element list rather than a tuple. + """ + name = two_col_table.search_table + path = _reload_file( + tmp_path / "d.txt", + ["n", "label"], + ["integer", "text"], + [[str(i), "l%d" % i] for i in ROW_ORDER], + ) + metafile = _metafile_with_sort(two_col_table, tmp_path, '[["n", -1]]') + two_col_table.reload(path, metafile=metafile, resort=True) + + table = db[name] + assert table._sort_orig == [["n", -1]] + assert [rec["n"] for rec in table.search({}, projection=["n"], sort=[["id", 1]])] == [ + 4, 3, 2, 1, 0 + ] + + from test_id_order_audit import audit + + assert audit.audit_table(db, name)[0] == "OK" diff --git a/tests/test_staged_writes.py b/tests/test_staged_writes.py index 8cd6d22..bc4c325 100644 --- a/tests/test_staged_writes.py +++ b/tests/test_staged_writes.py @@ -582,3 +582,113 @@ def att(prop): if version >= 140000: assert att("attcompression") == "p" assert db[name].lucky({"n": 0}, projection="label") == "stored" + + +################################################################## +# order bookkeeping across the swap # +################################################################## + + +def _flags(table): + """ + ``out_of_order`` as meta_tables records it for the live table. + """ + cur = table._execute( + SQL("SELECT out_of_order FROM meta_tables WHERE name = %s"), [table.search_table] + ) + return cur.fetchone()[0] + + +@pytest.fixture +def staged_ordered(table_factory): + """ + An ordered table of 20 rows sorted by n, ready to be staged. + """ + table = table_factory(sort=["n"]) + table.insert_many([sample_row(i) for i in range(20)]) + table.resort() + assert _flags(table) is False + return table + + +def test_staged_rewrite_of_a_sort_key_lands_out_of_order(db, staged_ordered): + """ + A staged write is deliberately not allowed to rebuild in sort order, so + whatever it did to the sort keys has to reach the live table as + out_of_order. The staged wrapper drops the rewrite's resort request; when + that request was also the only record that a sort column had moved, the + replacement went live claiming ORDER BY id still matched the sort. + """ + name = staged_ordered.search_table + + def reverse_n(rec): + rec["n"] = 1000 - rec["n"] + return rec + + with staged_ordered.staged() as staged: + staged.rewrite(reverse_n) + + live = db[name] + assert live._out_of_order is True + assert _flags(live) is True + # And the claim is true: ids no longer follow the sort. + ns = list(live.search({}, projection="n", sort=[["id", 1]])) + assert ns != sorted(ns) + + +def test_staged_rewrite_of_a_sort_key_is_seen_by_the_audit(db, staged_ordered): + """ + The audit is the ground truth the flag is checked against: it must report + the same broken ordering, and report OK again once resort() has rebuilt. + """ + from test_id_order_audit import audit + + name = staged_ordered.search_table + + def reverse_n(rec): + rec["n"] = 1000 - rec["n"] + return rec + + with staged_ordered.staged() as staged: + staged.rewrite(reverse_n) + + assert audit.audit_table(db, name)[0] == "MISMATCH" + db[name].resort() + assert audit.audit_table(db, name)[0] == "OK" + assert _flags(db[name]) is False + + +def test_staged_update_from_file_records_the_broken_order(db, staged_ordered, tmp_path): + """ + An explicit resort=True on a staged update still does not rebuild inside + the staged context, but it must not lose the fact that the file moves a + sort column either. + """ + name = staged_ordered.search_table + dump = tmp_path / "u.txt" + with open(dump, "w") as F: + F.write("label|n\ntext|integer\n\nl5|9999\n") + + with staged_ordered.staged() as staged: + staged.update_from_file(str(dump), resort=True) + # The staged copy was edited in place, not rebuilt: no renumbering. + assert staged._out_of_order is True + + assert _flags(db[name]) is True + assert db[name].lucky({"label": "l5"}, projection="n") == 9999 + + +def test_staged_write_leaves_untouched_sort_columns_alone(db, staged_ordered, tmp_path): + """ + Staging does not break the order flag by itself; only a write that could + have moved a sort key does. + """ + name = staged_ordered.search_table + dump = tmp_path / "u.txt" + with open(dump, "w") as F: + F.write("label|data\ntext|jsonb\n\nl5|{\"a\": 1}\n") + + with staged_ordered.staged() as staged: + staged.update_from_file(str(dump)) + + assert _flags(db[name]) is False diff --git a/tests/test_stats_validity.py b/tests/test_stats_validity.py index a19b9c4..fe555a5 100644 --- a/tests/test_stats_validity.py +++ b/tests/test_stats_validity.py @@ -323,3 +323,182 @@ def record(self, tables, suffix=""): assert seen == [([target.search_table], "")] assert target.count() == 200 + + +# --------------------------------------------------------------------------- +# no changed-data path keeps a cached answer by forgetting the flag +# +# The whole gate above is worth nothing if a write changes rows, does not +# refresh the caches, and leaves stats_valid true: the cached rows are then +# still served as answers. These check the flag, the cache and the truth. +# --------------------------------------------------------------------------- + +QUERY = {"flag": True} + + +@pytest.fixture +def counted(cached_table): + """ + ``cached_table`` with a nonempty-query count actually recorded and served. + """ + before = cached_table.stats.count(QUERY, record=True) + assert before == 67 + assert cached_table.stats.quick_count(QUERY) == before + assert stats_valid_in_meta(cached_table) is True + return cached_table + + +def assert_invalidated(table, expected): + """ + The flag is false, no cached answer is served, and the truth is available. + """ + assert table._stats_valid is False + assert stats_valid_in_meta(table) is False + assert table.stats.quick_count(QUERY) is None + assert table.count(QUERY) == expected + + +def _flag_flip_file(table, tmp_path, name="u.txt"): + """ + An update file clearing ``flag`` on the rows the cached query counts. + """ + labels = [ + rec["label"] + for rec in table.search(QUERY, projection=["label"]) + ] + path = tmp_path / name + with open(path, "w") as F: + F.write("label|flag\ntext|boolean\n\n") + for label in labels: + F.write("%s|f\n" % label) + return str(path) + + +def test_inplace_update_from_file_without_restat_invalidates(counted, tmp_path): + counted.update_from_file( + _flag_flip_file(counted, tmp_path), inplace=True, restat=False + ) + assert_invalidated(counted, 0) + + +def test_replacement_update_from_file_without_restat_invalidates(db, counted, tmp_path): + name = counted.search_table + counted.update_from_file(_flag_flip_file(counted, tmp_path), restat=False) + assert_invalidated(db[name], 0) + + +def test_rewrite_without_restat_invalidates(db, counted): + name = counted.search_table + + def clear_flag(rec): + rec["flag"] = False + return rec + + counted.rewrite(clear_flag, restat=False) + assert_invalidated(db[name], 0) + + +def test_inplace_rewrite_without_restat_invalidates(counted): + def clear_flag(rec): + rec["flag"] = False + return rec + + counted.rewrite(clear_flag, inplace=True, restat=False) + assert_invalidated(counted, 0) + + +def test_reload_without_matching_caches_invalidates(db, counted, tmp_path): + """ + A reload with no counts/stats files swaps changed search data in beside + companions it merely cloned; with restat off nothing rebuilds them. + """ + name = counted.search_table + searchfile = tmp_path / "data.txt" + counted.copy_to(str(searchfile), include_id=False) + # a file whose rows differ from the cached ones + text = open(searchfile).read().replace("|t|", "|f|") + with open(searchfile, "w") as F: + F.write(text) + + counted.reload(str(searchfile), restat=False) + assert_invalidated(db[name], 0) + + +def test_a_metafile_cannot_vouch_for_caches_the_reload_did_not_build(db, counted, tmp_path): + """ + meta_tables is replaced wholesale by the metafile, stats_valid included. + That flag describes the database the file came from; it must not be + installed over caches this reload never rebuilt. + """ + name = counted.search_table + searchfile = tmp_path / "data.txt" + metafile = tmp_path / "meta.txt" + counted.copy_to(str(searchfile), include_id=False, metafile=str(metafile)) + assert "|t|" in open(metafile).read() # the file asserts stats_valid + + text = open(searchfile).read().replace("|t|", "|f|") + with open(searchfile, "w") as F: + F.write(text) + counted.reload(str(searchfile), metafile=str(metafile), restat=False) + + assert_invalidated(db[name], 0) + + +def test_a_reload_carrying_its_own_caches_stays_valid(db, counted, tmp_path): + """ + The other half of the rule: an export that supplies counts and stats + alongside the search file installed caches built for exactly that data, + so the table is entitled to go on serving them. + """ + name = counted.search_table + searchfile = tmp_path / "data.txt" + countsfile = tmp_path / "counts.txt" + statsfile = tmp_path / "stats.txt" + counted.copy_to(str(searchfile), countsfile=str(countsfile), statsfile=str(statsfile)) + + counted.reload(str(searchfile), countsfile=str(countsfile), statsfile=str(statsfile)) + + table = db[name] + assert table._stats_valid is True + assert stats_valid_in_meta(table) is True + assert table.stats.quick_count(QUERY) == 67 + + +def test_a_refreshing_replacement_restores_validity(db, counted, tmp_path): + """ + And a replacement that does recompute its caches ends valid, serving the + new answer rather than the old one. + """ + name = counted.search_table + counted.update_from_file(_flag_flip_file(counted, tmp_path), restat=True) + + table = db[name] + assert table._stats_valid is True + assert stats_valid_in_meta(table) is True + assert table.count(QUERY) == 0 + + +def test_a_failed_swap_does_not_leave_validity_behind(counted, tmp_path, monkeypatch): + """ + The flag moves in the same transaction as the swap, so a swap that fails + leaves the table exactly as it was -- still valid, still holding the data + the caches describe. + """ + name = counted.search_table + searchfile = tmp_path / "data.txt" + counted.copy_to(str(searchfile), include_id=False) + + def boom(*args, **kwargs): + raise RuntimeError("swap blew up") + + monkeypatch.setattr(type(counted), "_swap_in_tmp", boom) + with pytest.raises(RuntimeError): + counted.reload(str(searchfile), restat=False) + counted._db.conn.rollback() + counted.drop_tmp() + + counted._refresh() + assert counted._stats_valid is True + assert stats_valid_in_meta(counted) is True + assert counted.stats.quick_count(QUERY) == 67 + assert counted.search_table == name diff --git a/tests/test_write_invariants.py b/tests/test_write_invariants.py index b8719ab..63d8a58 100644 --- a/tests/test_write_invariants.py +++ b/tests/test_write_invariants.py @@ -138,6 +138,80 @@ def test_inplace_update_of_a_sort_key_breaks_order(ordered, tmp_path): assert flags(ordered)["out_of_order"] is True +def _sort_key_update(tmp_path, name="u.txt"): + """ + An update file moving one row's ``n``, the configured sort column. + """ + dump = tmp_path / name + with open(dump, "w") as F: + F.write("label|n\ntext|integer\n\nl5|9999\n") + return str(dump) + + +def test_explicit_resort_false_still_records_a_broken_order_inplace(ordered, tmp_path): + """ + ``resort=False`` asks not to rebuild; it does not claim the sort was left + alone. The two used to be one variable, so passing it suppressed the + bookkeeping as well as the rebuild and left the table asserting that + ORDER BY id still matched the sort. + """ + ordered.update_from_file( + _sort_key_update(tmp_path), inplace=True, resort=False, restat=False + ) + assert flags(ordered)["out_of_order"] is True + assert ordered._out_of_order is True + + +def test_explicit_resort_false_survives_the_swap_on_the_replacement_path(ordered, tmp_path): + """ + Same for the non-inplace path: the replacement table was not renumbered, + so the table that goes live is out of order and must say so afterwards. + """ + ordered.update_from_file(str(_sort_key_update(tmp_path)), resort=False, restat=False) + assert flags(ordered)["out_of_order"] is True + + +def test_a_resorted_replacement_is_marked_ordered(ordered, tmp_path): + """ + The other half: a replacement that really was renumbered in sort order may + claim to be ordered. + """ + ordered.update_from_file(_sort_key_update(tmp_path), resort=True, restat=False) + assert flags(ordered)["out_of_order"] is False + + +def test_inplace_rewrite_marks_the_table_out_of_order(ordered): + """ + An in-place rewrite runs an arbitrary function over every row and dumps + every search column, so nothing can establish that the sort keys came + through unchanged. Its forced resort=False must not be read as evidence + that they did. + """ + ordered.rewrite(lambda rec: rec, inplace=True, restat=False) + assert flags(ordered)["out_of_order"] is True + + +def test_inplace_rewrite_refuses_an_explicit_resort(ordered): + """ + And it refuses to pretend it can rebuild, before running func over the + table rather than after. + """ + with pytest.raises(ValueError, match="resort"): + ordered.rewrite(lambda rec: rec, inplace=True, resort=True) + + +def test_an_update_touching_no_sort_column_keeps_the_order_flag(ordered, tmp_path): + """ + The flag is only pessimistic where it has to be: a file that cannot move + any sort key leaves a healthy table healthy. + """ + dump = tmp_path / "u.txt" + with open(dump, "w") as F: + F.write("label|data\ntext|jsonb\n\nl5|{\"a\": 1}\n") + ordered.update_from_file(str(dump), inplace=True, restat=False) + assert flags(ordered)["out_of_order"] is False + + # --------------------------------------------------------------------------- # stats_valid transitions # --------------------------------------------------------------------------- @@ -151,3 +225,49 @@ def test_refresh_stats_revalidates(ordered): ordered.insert_many([sample_row(1000)], restat=False) ordered.stats.refresh_stats() assert flags(ordered)["stats_valid"] is True + + +def test_file_updates_without_restat_clear_stats_valid(db, ordered, tmp_path): + """ + Every file-based write that does not rebuild the caches has to say so in + meta_tables, on both the in-place and the replacement path. Otherwise a + count cached before the write is still served after it; the observable + half of that is pinned in test_stats_validity.py. + """ + dump = tmp_path / "u.txt" + with open(dump, "w") as F: + F.write("label|num\ntext|numeric\n\nl5|12345\n") + + ordered.update_from_file(str(dump), inplace=True, restat=False) + assert flags(ordered)["stats_valid"] is False + + ordered.stats.refresh_stats() + assert flags(ordered)["stats_valid"] is True + + name = ordered.search_table + ordered.update_from_file(str(dump), restat=False) + assert flags(db[name])["stats_valid"] is False + + +def test_rewrite_without_restat_clears_stats_valid(db, ordered): + name = ordered.search_table + ordered.rewrite(lambda rec: rec, restat=False) + assert flags(db[name])["stats_valid"] is False + + +def test_reload_without_fresh_caches_clears_stats_valid(db, ordered, tmp_path): + name = ordered.search_table + dump = tmp_path / "d.txt" + ordered.copy_to(str(dump), include_id=False) + ordered.reload(str(dump), restat=False) + assert flags(db[name])["stats_valid"] is False + # and total is still exact: it is maintained separately, on every write + assert flags(db[name])["total"] == 100 + + +def test_a_restatted_replacement_keeps_stats_valid(db, ordered, tmp_path): + name = ordered.search_table + dump = tmp_path / "d.txt" + ordered.copy_to(str(dump), include_id=False) + ordered.reload(str(dump), restat=True) + assert flags(db[name])["stats_valid"] is True From a39070feeecfe5dd2402646c42af924a68ea0e95 Mon Sep 17 00:00:00 2001 From: David Roe Date: Wed, 5 Aug 2026 04:08:18 -0400 Subject: [PATCH 8/9] Document and pin where the stats-validity rule is deliberately pessimistic resort() renumbers the rows without changing them, so the cached counts and statistics would still be accurate afterwards; but it goes through the replacement path, which can only tell that the caches match when it rebuilt them, and it rebuilds them only with `saving` on. On a table that does not save statistics a resort therefore ends with stats_valid false and needs a refresh_stats() to serve cached counts again. That is the safe direction (a miss recomputes; a false true serves a wrong answer), so leave the behavior alone and make it a documented, tested quantity rather than a surprise: a note in resort()'s docstring and in the CHANGELOG, and a pair of tests pinning both halves. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 6 ++++- psycodict/table.py | 6 +++++ tests/test_stats_validity.py | 46 ++++++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6269d91..2f4839c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -448,7 +448,11 @@ hardening standalone use; the highlights: beside changed data) now mark the table invalid, in the same transaction as the swap. A `metafile` no longer installs a `stats_valid = true` describing the database it was exported from. Without this a count cached before such a - write was still served afterwards, defeating the enforcement above. + write was still served afterwards, defeating the enforcement above. The rule + is deliberately conservative in one place: `resort()` leaves the rows + unchanged, so the caches would still be accurate, but on a table without + `saving` nothing rebuilds them and it ends invalid. *Migration:* run + `refresh_stats()` after a `resort()` on such a table. - **`scripts/audit_id_order.py`**, a read-only check that streams each `id_ordered` table in sort order and reports whether its ids actually increase, since the flag can drift. Each table is audited in a read diff --git a/psycodict/table.py b/psycodict/table.py index 538e9ce..56103cc 100644 --- a/psycodict/table.py +++ b/psycodict/table.py @@ -1994,6 +1994,12 @@ def resort(self, force=False): disk space for the replacement table, its indexes, the dump file and the backup. + The rows themselves are untouched, so the cached counts and statistics + would still be accurate; but the replacement path marks the table + invalid unless it rebuilt them, which it does only with ``saving`` on. + On a table that does not save statistics, run ``refresh_stats()`` + afterwards to serve cached counts again. + INPUT: - ``force`` -- rebuild even when the table is already marked ordered diff --git a/tests/test_stats_validity.py b/tests/test_stats_validity.py index fe555a5..ae9cc42 100644 --- a/tests/test_stats_validity.py +++ b/tests/test_stats_validity.py @@ -502,3 +502,49 @@ def boom(*args, **kwargs): assert stats_valid_in_meta(counted) is True assert counted.stats.quick_count(QUERY) == 67 assert counted.search_table == name + + +def test_resort_of_a_non_saving_table_ends_invalid(db, table_factory): + """ + Pins the one place the rule is deliberately pessimistic. ``resort()`` + renumbers rows without changing them, so the cached counts would still be + accurate -- but it goes through the replacement path, which rebuilds the + caches only with ``saving`` on and otherwise cannot know they still match. + Documented in ``resort``'s docstring: refresh afterwards. + """ + table = table_factory(sort=["n"]) + table.insert_many([sample_row(i) for i in [3, 1, 2, 0]]) + assert table.stats.saving is False + name = table.search_table + table._execute( + SQL("UPDATE meta_tables SET stats_valid = true WHERE name = %s"), [name] + ) + table._db.conn.commit() + table._refresh() + + table.resort() + assert stats_valid_in_meta(db[name]) is False + + +def test_resort_of_a_saving_table_stays_valid(db, counted): + """ + With saving on, the resort rebuilds the counts against the renumbered + rows, so the table is entitled to go on serving cached answers. (The + rebuild recomputes the recorded statistics families; a one-off count + recorded by hand beforehand is not among them and is not restored.) + """ + name = counted.search_table + # sample_row(1000) has flag False, so the cached query still counts 67; + # the insert is here to break the order and give resort() work to do. + counted.insert_many([sample_row(1000)], restat=False) + counted.stats.refresh_stats() + counted.resort() + + # The swap installs a fresh table object, and `saving` lives on the + # object rather than in meta_tables, so turn it back on to record. + table = db[name] + table.stats.saving = True + assert table._stats_valid is True + assert stats_valid_in_meta(table) is True + assert table.stats.count(QUERY, record=True) == 67 + assert table.stats.quick_count(QUERY) == 67 From c395752ab859dd4ebf97d6b6c59abf56e39d9116 Mon Sep 17 00:00:00 2001 From: David Roe Date: Wed, 5 Aug 2026 04:19:58 -0400 Subject: [PATCH 9/9] Let the adjust_schema test resort, now that #143 renumbers correctly The test reloaded with resort=False and a comment saying why: renumbering selected the pre-reload table's columns, which the narrowed relation adjust_schema had built no longer had. #143 now reads the target relation's columns from the catalog, so the default (resort, since the file carries no ids) works, and the test pins the interaction from this side by checking that the ids were renumbered in sort order. Co-Authored-By: Claude Opus 5 --- tests/test_export_format.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/test_export_format.py b/tests/test_export_format.py index 009c438..a095bf9 100644 --- a/tests/test_export_format.py +++ b/tests/test_export_format.py @@ -241,10 +241,7 @@ def test_adjust_schema_builds_the_table_from_a_marked_header(db, filled_table, t filled_table.copy_to(searchfile, columns=["n", "label"], include_id=False) assert read(searchfile).startswith(EXPORT_FORMAT_MARKER) - # resort=False because renumbering ids selects the *old* table's columns, - # which a narrowed schema no longer has; that is a limitation of - # adjust_schema, not of the header reader this test is about. - filled_table.reload(searchfile, adjust_schema=True, resort=False) + filled_table.reload(searchfile, adjust_schema=True) # reload swaps in a new table, so the assertions must be made against the # object the database hands out now, not the pre-reload one. @@ -253,6 +250,9 @@ def test_adjust_schema_builds_the_table_from_a_marked_header(db, filled_table, t assert reloaded.count() == 200 assert {r["n"] for r in reloaded.search({}, ["n"])} == set(range(200)) assert reloaded.lucky({"n": 7}, "label") == "l7" + # The file carries no ids, so the reload renumbers in sort order -- over + # the relation the header built, whose columns are not this table's. + assert [r["id"] for r in reloaded.search({"n": {"$lt": 3}}, ["id"], sort=["n"])] == [1, 2, 3] # --- the automatic reindex decision ----------------------------------------