Scope RSS hydration summaries to each source - #462
Conversation
|
Read this against the current query. The diagnosis holds, and it is worse than the record states. Three things compound in
And So: the direction is right, and this is a real defect on a hot path rather than speculative tuning. One thing that needs an answer before this landsWhere do
That may well be intended — a baseline entry is pre-activation stock, not outstanding work — but the record does not say so. As written, a source whose entries are all baseline reports five zeros while the table plainly holds rows, and nothing in the response explains the gap. Worth one sentence either way. Two implementation notes
The On scopeDecision 1 (the aggregation) and decision 2 (the nested public contract) are independent changes travelling in one record. The contract change is reasonable on its own — The coupling is what I would separate. Nothing in the aggregation needs the contract to change, and nothing in the contract needs the aggregation. Landed together, a problem found in either one takes the other back with it. Suggest two implementation PRs against this one record: the lateral aggregation first (internal, independently measurable, independently revertable), then the contract with its frontend change. OtherwiseThis is a more rigorous record than most of the internal ones — four rejected alternatives with reasons, an explicit evidence requirement on the implementation PR, and what was left out sitting in Open questions rather than going unmentioned. Thanks for writing it up this way. |
|
Thanks — updated the record to address all three points.
The branch has been updated in b9f2daf. |
|
The revision answers all four points, and answers them better than I asked. The baseline paragraph states the consequence plainly — a source holding only baseline stock reports five zeros — instead of leaving the reader to infer it. The What I want to change is when it lands, not what it says. On merging this as a standalone recordI'd rather this file travelled with its first implementation PR than merged on its own. Three reasons, all from this directory's own conventions: Every record 0001–0025 sits at a built status — In progress, Built, Implemented, Cut 1 implemented. The index is scrupulous about what is not built, which is what makes it worth reading. Decision 1 does not meet the test for writing a record. The test is: someone looks at the code in six months, asks "why not simply…", and the answer is not in the code. Nobody looks at a lateral aggregate and asks why the whole table wasn't materialised six times. That is a defect fix, and #417 already specifies it down to the shape of the response object. What does meet the test is decision 2 (why the public contract broke) and the rejected alternatives (why not simply a cache table). That residue is worth keeping — it just doesn't need its own merge. Decisions 2 and 4 describe the second cut. If the whole record lands with the first PR, the status line can't be written honestly. Either the record travels with cut one and its status line says which decisions are implemented and which are still ahead, or it updates twice, once per cut. To be clear about what this is not: it isn't a request to hold the work or to re-litigate the design. The direction is approved. Write the query PR against this file, carry the file on that branch, and fill in the status line when it lands. Two notes for the query implementationBoth are things I'd want in the record before it lands, since they bear on decision 1 as written. The outer alias has to change. Name the index the plan should use. The only named index on |
Signed-off-by: danwood <118035379+Floating-Y@users.noreply.github.com>
Signed-off-by: danwood <118035379+Floating-Y@users.noreply.github.com>
b9f2daf to
77e0474
Compare
|
Thanks — I have updated this PR to carry the first implementation cut together with the decision record. The revision now:
The branch is now based on the latest I will hold the nested API and frontend cut until this first cut has landed. |
Signed-off-by: WaylandYang <wayland0916@gmail.com> # Conflicts: # docs/decisions/README.md
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: WaylandYang <wayland0916@gmail.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: WaylandYang <wayland0916@gmail.com>
|
Merged, with three additions pushed to your branch first so it could land today rather than wait another round:
The query is exactly as you last pushed it. #417 stays open for the nested contract and the Library change; same shape as this one, and welcome. |
Summary
This is the first implementation cut for #417.
It carries decision record 0026 together with the query implementation, as
requested in review. The record status now says that the query cut is
implemented while the nested public contract remains pending.
The public API remains unchanged in this PR.
Problem
sources::listpreviously wrapped the completeENTRY_SELECTprojection in aCTE referenced six times.
Because the CTE is referenced more than once, PostgreSQL materializes it. Since
ENTRY_SELECThas no source, knowledge-base or generation filter, this projectsevery row in
rss_full_content_entries, including rows belonging to otherknowledge bases and older generations.
Each correlated count then scans the unindexed materialized result for every
listed source.
The resulting shape was:
Implementation
The query now:
LEFT JOIN LATERALaggregate;rss_generation;listed_sourceas the outer alias so it cannot be shadowed by thesources sjoin insideENTRY_SELECT;pass with
count(*) FILTER;ENTRY_SELECT;SourceViewfields in this cut;SOURCE_SECRET_KEYS, andconfig - $2::text[].The source/generation lookup is served by the implicit btree behind:
PostgreSQL names that index:
The partial
rss_full_content_entries_pending_idxdoes not serve thisaggregate.
baselineobservations remain outside the five hydration work counts. Theexisting flat
baseline_countis still calculated in this first cut so thepublic API remains unchanged.
Tests
The store tests cover:
queuedandhydrating;terminal,deleted, andsuperseded;The database-backed suite was run with:
against PostgreSQL running in Docker, so database tests could not silently
skip.
Validation completed:
EXPLAIN ANALYZE
The comparison used PostgreSQL 16.15 with 22,000 RSS observations:
Both plans were captured with:
A warm-cache run with JIT disabled produced:
rss_full_content_entries_source_id_activation_generation_ex_keyThe lateral aggregate ran once per listed RSS source. For the non-RSS source,
the entry index scan was reported as
never executed.The important result is the change in scan scope: observations from older
generations and other knowledge bases are no longer projected for the target
source. Execution time is included for completeness but is secondary because
it is sensitive to cache state.
Scope
This PR includes:
It does not include:
rss_full_contentpublic contract;The nested API and Library update will follow in a separate PR after this query
cut lands.
Part of #417.