Stop serving cached statistics while they are marked invalid - #141
Stop serving cached statistics while they are marked invalid#141roed-math wants to merge 5 commits into
Conversation
…random edge cases Four independent defects found reviewing rc2, each with regression tests that fail on rc2 and pass here. max_id/min_id formatted their table argument into the statement as text. A name needing quotes was a syntax error and a name carrying its own statement ran it; both compose an Identifier now. While there, the empty sentinel is documented: max_id returns -1, and 0 is a real id, which is what random() got wrong below. _approx_most_common read reltuples from a hard-coded public.nf_fields but read frequencies from the owning table, so every table other than nf_fields got its own frequencies scaled by an unrelated row count. The row count now comes from the table the statistics are about, looked up by name in the current schema, and the column type goes through column_type_sql rather than being concatenated. update_from_file's logging default was a shared dictionary literal that the method wrote logid and aborted into. Consecutive default calls saw the previous call's values and a caller's dictionary came back modified. An AST scan of the package confirms this was the only mutable default anywhere that is actually mutated, so nothing else needed changing. random() raised IndexError from random.choice([]) when pick_first found no values, reported a table whose only row has id 0 as empty, and discarded rows whose projection was falsy -- a table of zeros exhausted maxtries and raised "Random selection failed!". random_sample returned None for an unrecognized mode, which reads like an empty result, and reseeded the global random module when asked for a repeatable sample.
4b3f9d7 to
055373a
Compare
Write paths called _break_stats, but read paths queried the cache tables regardless, so a count cached before a restat=False write kept being served afterwards. Reproduced against PostgreSQL 18: a query counted at 67, every matching row then changed so that none satisfy it, and quick_count still answered 67. Every lookup that would serve a cached answer now goes through one predicate, _may_use_cache, and reports a miss while stats_valid is false -- quick_count, quick_count_distinct, _quick_statistic, _has_stats, _has_numstats and null_counts, which between them are what make count, max, min, sum, column_counts and numstats recompute rather than return a stored value. Three things are deliberately outside the rule, and the predicate's docstring says why: the empty-query total, which is maintained on every write and stays exact; a suffixed table, whose caches are its own and which stats_valid says nothing about; and the _status/status/extra_counts inventory, which reports what the cache contains rather than answering a question about the data -- refresh_stats uses it to discover what to recompute, so gating it would make an invalid table forget what statistics it is supposed to have. The flag had no way back to true. _restore_stats is the counterpart of _break_stats, called at the end of refresh_stats inside the same transaction that rebuilt the caches, so a refresh that fails part-way leaves the table marked invalid rather than claiming a cache it does not have. A suffixed refresh does not touch the live flag. Separately, bulk paths now run PostgreSQL's own ANALYZE, which is a different thing from psycodict's statistics: a bulk-loaded relation has none until autovacuum reaches it and the planner costs it as though it were tiny. The _tmp copies are analyzed before the swap and outside its transaction, since the catalog entry follows the relation through a rename -- one call in _swap_in_tmp covers reload, rewrite, non-inplace update_from_file and staged commits, which all funnel through it. copy_from analyzes the live table. The existing statistics fixtures insert rows, which invalidates, and then assume a usable cache; they now say so with _restore_stats, which is true of them (nothing is cached yet and the total is maintained by the insert) and is the state a freshly loaded table is in.
055373a to
798fdd9
Compare
The gate roed314#141 added was decided by a Python attribute, and only the row-level write paths maintained the flag it read, so the invariant it advertised did not hold: a second process kept serving what it had cached, and a replacement path could swap new data under old caches without clearing anything. Database-authoritative, and atomic. quick_count, quick_count_distinct and _quick_statistic now carry AND EXISTS (SELECT 1 FROM meta_tables WHERE name = %s AND stats_valid) into the SELECT that reads the cached row, rather than consulting self.table._stats_valid. That attribute is a copy taken when the table object was built: another process's restat=False write moves the row and not the copy, which is precisely the deployment this is for, and a rolled-back transaction moves the copy and not the row. One statement rather than a flag query followed by a cache query, because two statements are two snapshots and a write can commit between them. count() on an empty query likewise answers from meta_tables.total, a single-row metadata lookup rather than the stale copy; with a suffix it reads the suffixed counts row instead, since the maintained total describes the live table. _break_stats and _restore_stats now issue their UPDATE unconditionally, through a shared _set_stats_valid: a transition skipped because the local copy already said so is a transition skipped on the strength of a value that may describe neither the database nor the present. That UPDATE's row lock is also what orders a refresh against writers, so a live refresh_stats now claims the row at the start instead of only restoring it at the end. Overlapping work is then serialized into one of the two acceptable outcomes -- the write is in the data the refresh reads, or its invalidation lands after the refresh commits -- rather than a write committing mid-rebuild and having its invalidation overwritten. Every replacement path makes the transition, with the rename. update_from_file (both forms), rewrite, reload, reload_all, reload_revert and the staged swaps could all change the live data while leaving the flag true. _swap_in_tmp and reload_final_swap now take the intended state and write it inside the transaction that does the renames, so the flag and the relations cannot come apart; _reload_stats_valid states the rule for reload once, since reload_all defers its swaps to a second pass. A metafile's own stats_valid is overruled by what the swap actually did -- the file describes the table it was exported from, and cannot know whether these relations were rebuilt. reload_revert invalidates, because a backup carries no validity bit of its own, and recounts the total for the same reason. Cache maintenance asks a different question. _record_count, _record_count_distinct and _record_statistic chose between INSERT and UPDATE/DELETE by calling the public lookups, which under the new gate report a miss for a row that is physically present -- on every invalid table, which is every table they run on. Each recorder was leaving a second row under a key the rest of the code takes to identify at most one; the plainest case was a write clearing the flag and then duplicating the {} total row it maintains. They now use private physical lookups (_cached_count, _cached_count_distinct, _cached_statistic) that ignore the flag, and _record_statistic updates in place rather than always inserting. Exercising that branch for the first time also turned up _record_count_distinct's update naming a column stats that has always been called stat. tests/test_stats_validity.py grows 30 cases: two-handle staleness for counts, distinct counts and max/min/sum and for the total; a stale local false not skipping an invalidation; a rolled-back restore; a threaded refresh/write race; one per replacement path in both directions, the metafile interaction, and a swap failing between the rename and the flag; and physical-row counts after each recorder. Every one of them fails on 798fdd9. Full suite: 1342 passed, 35 skipped, 1 xfailed. Ruff and the -W docs build clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Pushed The gate is the database's.
Every replacement path states what it left true, in the transaction that does Cache maintenance asks whether the row exists, not whether it may be served. Docs. Tests. 31 new cases in Full suite 1342 passed / 35 skipped / 1 xfailed, run twice; ruff and the Two things still not fixed, both noted at the bottom of the description: the 🤖 Generated with Claude Code |
The last place deciding "is this already recorded here" with the gated lookup. It is reached from refresh_stats, which has just claimed the table's row and marked it invalid, so quick_count reported every extra count missing and each one was recounted -- harmless, since the refresh had also just deleted them all, but it is the answer-validity gate answering an existence question, which is the confusion the rest of this commit series takes apart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Correction to my comment above: I listed That leaves one known gap rather than two, and it is blocked rather than deferred: |
reload_all runs every reload with final_swap=False and swaps in a second pass, and that second pass built each swap's list of relations from the files it had found in the input folder. That is not the list the reload prepared. A saving table's reload readies both cache companions whatever files it was given -- creating any missing _tmp copy and, under restat, refreshing both from the loaded data -- so a folder without a _counts.txt yielded a swap naming only the search table. The refreshed companions stayed under _tmp, the old live ones stayed live describing data that had just been replaced, and the validity transition added earlier in this branch then marked that pairing valid, because it was computed from restat rather than from what was being swapped. The mixed case is the same: one file present, one companion swapped, one stranded. reload now returns a ReloadPlan -- the exact base names whose _tmp copies belong at the live names, the stats_valid the swap should write, and the resolved ordered -- and reload_all passes it straight to reload_final_swap. Preparation and finalization can no longer drift, because there is only one derivation. _reload_stats_valid additionally takes that list and requires both companions to be in it, so the flag cannot outrun the swap even if some future caller does assemble a list by hand; and reload_all now passes ordered through, which it never did. Ten new cases: reload_all with neither cache file, with only counts and with only stats, under both restat and sequential_swap, asserting that nothing is left under _tmp, that the flag says what the swap did, and that the value served afterwards is the rebuilt one rather than the pre-reload cache; the restat=False counterpart, which may keep old caches but must not call them valid; the both-files case that was already correct; and the plan itself. Eight of the ten fail on 3c4c225. Full suite: 1352 passed, 35 skipped, 1 xfailed. Ruff and the -W docs build clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Pushed
Ten new cases, all through Full suite 1352 passed / 35 skipped / 1 xfailed; ruff and the |
|
GPT signed off. |
2 participants
Second of the rc3 stabilization PRs: B3 (cache validity) and B5
(
ANALYZE) of the review.Stacked on #140. The diff contains its commit too; merge that first and
this shrinks to its own.
The bug
Write paths called
_break_stats, but read paths queried the cache tablesregardless. Reproduced against PostgreSQL 18 on rc2:
The fix
stats_validbecomes an invariant rather than a hint: a cached count orstatistic is served only when the same PostgreSQL snapshot says the live
table's caches are valid, and every path that changes the data states what it
left true in the transaction that changed it.
1. The gate, and where it is decided
Every lookup that would serve a cached answer reports a miss while the flag is
false:
quick_count,quick_count_distinct,_quick_statistic. Those threeare what make
count,max,minandsumrecompute instead of returning astored value.
They test the flag in the same
SELECTthat reads the cached row:Not
self.table._stats_valid, which is a copy taken when the table object wasbuilt. A deployment runs several webserver processes: one of them committing a
restat=Falsewrite moves the row and not the other processes' copies, sobefore this every other process went on serving the counts it had cached. A
rollback moves the copy and not the row. And a flag query followed by a cache
query is two snapshots with room for a write to commit in between, so both
readings go into one statement. The attribute survives as advisory only —
nothing rests on it.
count()on an empty query now answers frommeta_tables.totalfor the samereason. That is a single-row metadata lookup, not a scan, and the total is
maintained rather than cached, which is why it stays exempt from the gate. With
a suffix it reads the suffixed counts row instead: the maintained total
describes the live table and says nothing about a
_tmpcopy._break_statsand_restore_statsissue theirUPDATEunconditionally now(shared
_set_stats_valid) — a transition skipped because the local copyalready said so is skipped on the strength of a value that may describe neither
the database nor the present. That
UPDATE's row lock is also what orders arefresh against writers, so a live
refresh_statsclaims the row at thestart instead of only restoring it at the end. Overlapping work is then
serialized into one of the two acceptable outcomes — the write is in the data
the refresh reads, or its invalidation lands after the refresh commits — rather
than a write committing mid-rebuild and having its invalidation overwritten by
a refresh that had already counted some families without it.
2. Which paths make the transition
The row-level paths always did. The replacement paths did not: they could swap
new data in while keeping the old counts and stats relations and a true flag,
which the new gate would then happily serve.
_swap_in_tmpandreload_final_swapnow take the intended state and write it inside thetransaction that does the renames, so the flag and the relations cannot come
apart:
stats_validinsert_many,upsert,update,delete,copy_from, in-placeupdate_from_filesavingandrestatrefreshed the cachesupdate_from_file,rewritesavingandrestat— exactly when the rebuilt_tmpcounts and stats are swapped in with the datareload,reload_allsaving, both cache companions are in the list actually being swapped, and eitherrestator both acountsfileand astatsfilewere suppliedstaged_force_swapreload_revertrefresh_stats()on the live tablereloadreturns aReloadPlan— the exact swap list, the intendedstats_valid, the resolvedordered— andreload_all, which runs everyreload before any swap, passes it straight through instead of rebuilding the
list from the input filenames. Those two are not the same list: a saving
table's reload readies both cache companions whatever files it was given, so
a folder with no
_counts.txtyielded a swap naming only the search table,stranding the refreshed companions under
_tmpand leaving the old live onespaired with new data.
_reload_stats_validtakes the swap list too andrequires both companions to be in it, so the flag cannot outrun the swap.
Nothing inherits the old live table's flag, which was a fact about data that is
no longer there — including a
metafile's ownstats_validcolumn, which isoverruled by what the swap actually did (the file records what was true of the
table it was exported from, and cannot know whether these relations were
rebuilt).
reload_revertinvalidates because a backup carries no validity bitof its own —
meta_tableshas one row and it stayed with the live name — andrecounts the total for the same reason.
3. "May this be served" is not "is this row here"
Cache maintenance chooses between
INSERTandUPDATE/DELETEby askingwhether the row it is about to write already exists. Routed through the public
lookups, that question now answers "missing" for a row that is physically
there — on every invalid table, which is every table the maintenance runs on.
Each recorder was leaving a second row behind under a key the rest of the code
takes to identify at most one. The plainest case is a write that clears the
flag and then maintains the total, duplicating the
{}row it was maintaining._record_count,_record_count_distinctand_record_statisticnow useprivate physical lookups (
_cached_count,_cached_count_distinct,_cached_statistic) that ignore the flag, and_record_statisticupdates inplace rather than always inserting. Sending rows down the update branch for the
first time also turned up
_record_count_distinctnaming a columnstatsthathas always been called
stat.What is deliberately not gated
Corrected from the first version of this description, which claimed all six
lookups were gated. Three lookups are, and the checked-in changelog and
DataManagement.mddescribe the rest. The line is whether a miss costs onebounded query or a rebuild:
total, maintained on every write and so exact (now readfrom
meta_tables, per §1);_tmpor_oldNcopy carries its own caches, builtor loaded with its data, and
stats_validsays nothing about them;_status/status/extra_countsinventory, which reports whatthe cache contains rather than answering a question about the data —
refresh_statsuses it to discover what to recompute, so gating it wouldmake an invalid table forget which statistics it is supposed to have;
_has_stats/_has_numstats, which decide whether a whole statisticsfamily needs computing, and
null_counts, whose fallback is one fullcount per search column. Gating those made
column_counts,numstatsandnull_countsrebuild on every call with nothing to converge on, since onlyrefresh_statsrestores the flag — measured on the LMFDB, four minutes ofdownstream suite became over forty-five, mostly inside
null_countsovernf_fieldsand friends;The gap that leaves, stated plainly:
column_counts,numstatsandnull_countscan 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. It ispinned by a
strict=Truexfail, so a future per-statistic change turns itgreen rather than going unnoticed.
B5:
ANALYZEPostgreSQL's planner statistics are a different thing from psycodict's, and a
bulk-loaded relation has none until autovacuum reaches it — so it is costed as
though it were tiny. New
_analyzehelper, called on the_tmpcopies beforethe swap (the catalog entry follows the relation through a rename, and the
swap's lock window stays as short as it was). All four replacement paths —
reload,rewrite, non-inplaceupdate_from_file, staged commits — funnelthrough
_swap_in_tmp, so that is one call.copy_fromanalyzes the live tableit loaded into.
Tests
tests/test_stats_validity.py, 55 cases (14 in the first version, 41 addedhere). 33 of the 41 fail on the head they were written against; the other
eight are the row-level paths re-checked (already safe, and kept that way), the
swap-failure rollback, which has nothing to observe until the flag is written
with the renames, and the two
reload_allcases that were already correct. Thenew ones:
PostgresDatabaseon its own connection caches acount / distinct count / max / min / sum, the first handle writes with
restat=False, and the second must stop serving it — with its own_stats_validasserted still true, so the test is about the row and not theattribute. Plus the same for the empty-query total.
write must clear the row even though A's copy was already false.
DelayCommitthat then raises: theobject is left saying true, the row says false, and the stale row the
rollback restored is still not an answer.
start of the rebuild — later than that and the total's own
UPDATEhasalready taken the row lock, and the test would pass either way. Asserts the
writer does not commit mid-rebuild and that the final flag obeys the
serialization.
reload_all. With neither cache file, with only counts, and with onlystats, under both
restatandsequential_swap: nothing left under_tmp,the flag saying what the swap did, and the value served afterwards being the
rebuilt one rather than the pre-reload cache. Plus the
restat=Falsecounterpart, the both-files case that was already correct, and the plan
itself.
update_from_file(restat=False),rewrite(restat=False),reload(restat=False),reload_revert, a staged commit, and the row-levelpaths re-checked; then the
restat=Trueswap and reload ending valid froman invalid start and serving the rebuilt value, the metafile interaction,
and a swap that fails between the renames and the flag, where neither
commits.
the maintained total while invalid, re-recording a count, a distinct count
and each of max/min/sum after a
restat=Falsechange, and arefresh_statson an invalid saving table — one row each, holding the recomputed value
rather than two that happen to agree.
Full suite: 1352 passed, 35 skipped, 1 xfailed. Ruff and the
-Wdocsbuild clean.
One thing I did not fix
add_statsreplaces the counts rows whose keys it reinserts, not the whole(cols, split)family, so when two families share acolskey and the datachanged between them, the earlier family's orphaned rows survive and
column_countsreports them: 42 groups summing to 70 where the truth is 32groups over 60 rows, in the reproduction I worked up.
Split out as #148 rather than fixed here. It predates this PR, it is about
add_statsnot owning the rows it replaces rather than about the flag, and thefix wants real design work on family bookkeeping: counts rows record neither
which family wrote them nor a threshold, and two existing tests pin why the
obvious family-wide delete is wrong (
test_range_count_survives_add_stats,test_threshold_families_share_rows).refresh_statsrepairs it, and thereload and refresh pipelines do not produce the state in the first place. The
issue records the one question worth settling before 1.0.0: whether the accepted
fix can avoid a new column on the counts relation.
(An earlier revision of this description also listed
add_column/drop_columnas leaving stale counts behind. That was wrong:drop_columnalready deletes the counts and stats rows whose
colsorconstraint_colsmention the column, in the transaction that drops it, and
add_columnadds acolumn that is NULL in every row, which makes no cached count wrong —
null_countscomputes any column missing from the counts table on the fly.)🤖 Generated with Claude Code