docs(content-drive): spec for materialized folder-first CTE fix (#37229) - #37230
docs(content-drive): spec for materialized folder-first CTE fix (#37229)#37230ihoffmann-dot wants to merge 7 commits into
Conversation
|
Claude finished @ihoffmann-dot's task in 1m 20s —— View job Spec review — round 3 (re-check of prior findings)
This PR carries Resolved
New Issues
Both are non-blocking wording-propagation gaps, not new substantive problems — the requirement-level text (FR-001, SC-003) is now correct; two acceptance-scenario/narrative lines just didn't inherit the carve-out. AssessmentAll round-1 and round-2 blockers are resolved, and FR-010's mandated pre-implementation measurement was performed rather than deferred. The remaining two items are minor consistency fixes in the acceptance-scenario prose; they don't block spec approval but are worth tightening so the test author isn't handed a criterion that fails for reasons that aren't bugs. · branch |
fabrizzio-dotCMS
left a comment
fabrizzio-dotCMS
left a comment
There was a problem hiding this comment.
Reviewed against main at 88af0bad55.
This is the best-written spec in the set, and FR-010 is why. It takes the one genuine unknown, refuses to dress it up as a decision, makes it a mandatory measurement before design is finalized rather than a post-implementation discovery, and pre-declares both outcomes. It also says outright that SC-001's number is a target and not a result. That is rare and it is the right instinct. I'd copy the pattern into the sibling specs.
FR-004's predicate inventory is also complete and accurate — I checked it line by line against selectQuery and it invents nothing and misses nothing.
Two substantive problems, one of which will stop the test author on day one.
FR-001 demands byte-identical ordering from a query whose ordering is undefined
FR-001 requires "identical result sets (same items, same order, same pagination cursors)"; SC-003 repeats "the exact same items in the exact same order."
But there is no tiebreaker today. appendOrderByQuery (BrowserAPIImpl.java:2513-2520) emits exactly:
sqlQuery.append(" order by ");
if (orderByDesc) { sqlQuery.append(" c.mod_date desc"); }
else { sqlQuery.append(" c.mod_date asc"); }#37148 measured that 1.2% of rows in the tested folder share a mod_date with another row (256 of 21,423), and flagged it as a defect: a row can appear on two pages or be skipped while paginating.
So rows tied on mod_date come back in whatever physical order the access path produces — and changing the access path is the entire point of this fix. FR-001 is unsatisfiable in its strict reading, and the only way to make it satisfiable is to add the deterministic tiebreaker that FR-001's own "same order" clause forbids.
The spec needs to pick one:
- "Same set; order among
mod_dateties may differ." Honest and testable. It also means the pagination-cursor guarantee needs the same caveat, because a tied row can still be duplicated or skipped across pages — exactly as today. - Add the tiebreaker. Accepts a documented, intentional order change and actually fixes the paging defect.
Either is defensible; leaving FR-001 as written hands the test author a criterion that can fail for reasons that aren't bugs.
#37148's two correctness findings are descoped without saying so, and one is inverted into a requirement to preserve the defect
#37148 had a section titled "Two correctness findings in the same query" and made both item-1 acceptance criteria: host_inode must become part of the folder predicate, and ORDER BY must get a deterministic tiebreaker.
Neither survives. The tiebreaker is gone (above). And the host filter isn't merely dropped — this spec's Edge Cases make preserving its absence a requirement:
A folder whose content spans more than one site sharing the same relative path (the case where today's query applies no host filter at all) must keep returning exactly the same cross-site result it does today — the fix must not silently add a host restriction that wasn't there before.
I verified the no-host-predicate case is real: inside shouldApplySiteFiltering, when site == null && !forceSystemHost, nothing is appended (:1959-1970).
Descoping a correctness fix out of a performance fix is good practice — the problem is that it isn't stated anywhere. The spec claims "0 open clarifications" and lists two resolved scoping decisions (the filename filter, the test-matrix sizing); neither is this. #37229's own AC list drops them too, so they were lost when the spike replaced #37148's item 1, and nothing records where they went. #37148 is closed, so right now they have no home.
There's also a mechanical tension worth resolving explicitly. #37148 noted that the folder-first rewrite "adds the filter as a side effect", and its PoC CTE is literally:
with folder_ids as materialized (
select id, asset_subtype from identifier
where parent_path = ? and host_inode = ?
)So implementing the shape this spec says it is implementing adds the host predicate, and the Edge Case forbids it. The implementer would have to strip host_inode out of the CTE deliberately — which makes the materialized set match on parent_path alone across every site, i.e. less selective, which is the opposite of what materializing it is for.
FR-009 asserts an invariant that doesn't exist today
FR-009 The fix MUST remain compatible with the existing single-scan-per-request behavior of the folder-listing candidate query — i.e., it must not change how many times the underlying scan query executes per request
There is no existing single-scan-per-request behaviour. getContentByChunks (:262-309) is a loop: it pulls 900 candidate inodes (BROWSER_CONTENT_CHUNK_SIZE, :547), filters that chunk, and returns for the next OFFSET until the page is full. #37184 exists precisely because that runs roughly four times on a field-filtered request.
#37229's AC words it correctly — "the single-scan-per-request assumption that fix depends on" — i.e. #37184's post-fix state. The spec turns it into a claim about current behaviour, which reads as a requirement to preserve something that isn't there and manufactures a conflict with #37184 that doesn't exist.
Smaller items
SC-002 fails against the spike's own reference case. It says no folder "including folders that already load quickly today — becomes noticeably slower." The spike measured an already-fast folder going from 39 ms to 63–68 ms, a 67% increase. User Story 1 states the same thing correctly as a bounded absolute (+25–30 ms). SC-002 should use that absolute bound; as worded, the validated reference case doesn't meet it.
The index assumption is optimistic about column order. The index is unique (parent_path, asset_name, host_inode) (postgres.sql:1033) — confirmed, and it does support the lookup. But asset_name sits between the two columns the CTE filters on, so parent_path gives the index range and host_inode can't narrow it; it becomes an index-tuple filter after the prefix scan. Cheap (index-only, no heap) but not quite "the index supports the materialized candidate-set lookup" in the strong sense, and FR-003 forbids adding one. Moot if host_inode leaves the CTE per the previous section.
Related: appendFileNameQuery emits LOWER(id.asset_name) = ? (:2479-2487). The decision to put it inside the CTE is right — one predicate, same identifier row, no new join — but LOWER() makes asset_name non-indexable, so it contributes no index selectivity there.
"Three site-scoping sub-cases" is four SQL shapes:
appendSiteQuerywithforceSystemHost→and (id.host_inode = ? or id.host_inode = 'SYSTEM_HOST')(:2116)appendSiteQuerywithout →and (id.host_inode = ?)(:2118)appendSystemHostQuery→and (id.host_inode = 'SYSTEM_HOST')(:2124)- nothing at all — reachable two ways:
ignoreSiteForFolders = true, orsite == null && !forceSystemHost
FR-004 collapses forceSystemHost-with-a-site and forceSystemHost-without-a-site, which are different SQL. The CTE has to reproduce all four.
This is a restructure, not a wrap. The PR summary describes "wrapping the folder-scoped candidate lookup in WITH folder_ids AS MATERIALIZED (...)". But buildSelectBaseQuery (:2040-2048) builds old-style comma joins with the join predicates in WHERE, and roughly a dozen append* methods each concatenate " and …" fragments into that one StringBuilder. The CTE splits those predicates in two — parent_path / host_inode / LOWER(asset_name) inside, and variant_id, lang, deleted, structuretype, struc.inode not in, the workflow EXISTS, the tag/relationship IN, jsonb_path_exists for MIME, show-on-menu, the contentlet_as_json::text ILIKE and the ORDER BY outside. Every appender then has to know which buffer it targets, and the base query has to move to explicit JOIN syntax as the PoC does. FR-007 is safe (these are private methods), but the plan should size this honestly rather than as a wrap.
No test type is named. Assumptions defers test design to the plan phase; FR-010 partly compensates by naming EXPLAIN ANALYZE as the first step. But SC-003/SC-004 assert identical results across the full size matrix, both roles and every filter combination, and nothing says what runs that. BrowserAPITest, ContentDriveHelperContentletAPIComparisonTest and ContentDriveFieldFilterTest already exist in dotcms-integration and are the natural homes.
Verified, no action needed
- Index exists exactly as claimed:
unique (parent_path, asset_name, host_inode),postgres.sql:1033✅ - No tiebreaker today (
:2513-2520) ✅ — which is what makes the FR-001 problem real - The no-host-predicate path is real (
:1959-1970) ✅ - FR-004's predicate inventory matches
selectQueryexactly: language (:1954), site (:1963), folder (:1973), workflow + archive-step branching (:1976-1983), filter/fileName when not ES (:1985-1992), DB-routed field criteria (:1995-1998), show-on-menu (:2000), exclude-archived (:2005-2007), MIME (:2009) ✅ AS MATERIALIZEDis PG12+, but PG12+ is already a hard floor —jsonb_path_exists(:2547) andcontentlet_as_json::text ILIKE(:2165) are Postgres SQL/JSON, onlypostgres.sqlships, and the test environment runs PG 18.4. No new portability constraint ✅- ADR-0018: correct — folder, site, language, type, workflow, sort, pagination and permissions all stay DB-resolved, nothing moves to the index ✅
- The filename-filter scoping decision is technically right ✅
- The PR description matches the spec: 2 user stories, 10 FRs, 5 SCs, edge cases, Legacy Considerations all genuinely present ✅
What I'd ask for
- Resolve FR-001 versus the missing tiebreaker. Pick "same set, ties may reorder" or "add the tiebreaker and document the order change." This is the one that blocks test design.
- State the descoping of #37148's two correctness findings explicitly, with a pointer to where each goes, and reconcile the host Edge Case with the PoC CTE shape the spec says it is implementing.
- Reword FR-009 to reference #37184's post-fix invariant rather than a current-behaviour claim.
- Restate SC-002 as the absolute +25–30 ms bound instead of "noticeably slower."
- Note the index's middle column, and size the refactor as a restructure.
Sequencing: this is the dominant item — 74% of the measured time — and #37184's SC-002 in particular is arithmetically unreachable until it lands. It is also the only spec of the four that is honest about not yet knowing whether its own measurement holds, which is why FR-010 matters as much as it does.
|
@fabrizzio-dotCMS thanks for the review. Updated the spec: FR-001 now requires a deterministic tiebreaker in the |
fabrizzio-dotCMS
left a comment
fabrizzio-dotCMS
left a comment
There was a problem hiding this comment.
Round 2 — re-review of a94481da
Re-reviewed against main @ 340c703feb.
All five asks were answered, and asks 3/4/5b are cleanly done. Ask 2 you answered by including both #37148 correctness findings rather than descoping them — defensible, and it resolves the mechanical irony from round-1 B2 in the right direction: the CTE now keeps host_inode and is therefore more selective, instead of the implementer having to strip it out.
Two problems: FR-004a targets a code path Content Drive never takes, and ask 1 is only half-resolved — SC-003 got the tiebreaker carve-out, FR-001 didn't.
✅ Resolved
- FR-009 (
:148-155) — reworded to the post-#37184 invariant, and it now states the ~4-loop current behaviour explicitly instead of asserting an invariant that doesn't exist. ✅ - SC-002 (
:214-219) — restated as the absolute +25–30 ms bound. ✅ - Test homes named (
:270-277). ✅ See the last item below — FR-004a needs a fixture none of them has.
🔴 FR-004a is either a no-op for Content Drive or a backward-compat break, and the spec doesn't distinguish
Round-1 S3 listed two ways to reach "no host predicate". Re-verified at BrowserAPIImpl.java:2020-2031:
final boolean shouldApplySiteFiltering = !browserQuery.ignoreSiteForFolders && browserQuery.folder != null;
if (shouldApplySiteFiltering) {
if (browserQuery.site != null) { appendSiteQuery(...); }
else { if (browserQuery.forceSystemHost) { appendSystemHostQuery(...); } } // site==null && !force → nothing
}ignoreSiteForFolders == true→ the whole block is skipped. Deliberate.shouldApplySiteFiltering && site == null && !forceSystemHost→ nothing appended. The accidental gap #37148 found.
FR-004a (:132-139) says to add host_inode for "the 'no host restriction at all' sub-case" without saying which one.
And Content Drive only ever reaches it through path 1. ContentDriveHelper.java:164-175:
} else {
builder.withHostOrFolderId(folder.getInode())
// When a specific folder is selected, enable ignoreSiteForFolders to allow
// folder selection without being limited by site filtering
.ignoreSiteForFolders(true);
}Every specific-folder Drive listing — the exact request shape this spec optimizes, the one behind the 20k-folder measurement — sets that flag. Path 2 isn't how Drive gets there.
So FR-004a forks, and the branches differ by "does this fix do anything at all for Content Drive":
| Reading | Consequence |
|---|---|
| Scope to path 2 (the safe one) | FR-004a is a no-op for Content Drive. The cross-site over-return stays open in the portlet it was found in — and round-1 B2's irony returns in full: Drive's CTE still matches parent_path alone across every site, a less selective materialized set, which is the opposite of what materializing it is for. |
| Apply to path 1 | Reverses a deliberate decision — the ContentDriveHelper comment states the intent — and changes what a Drive folder shows. |
The second is a product question: should a Content Drive folder listing show content from other sites sharing the same relative path? That isn't something a performance spec settles by adding an FR, and it's the one thing here that needs an owner outside this document.
Also, the host value has to come from somewhere. In path 2 there's no browserQuery.site to bind; in path 1 there is a folder, so folder.getHostId() is available — but using it is the behaviour change. FR-004a and the PoC snippet (host_inode = ?) don't say where the value originates.
🔴 Round-1 B1 is half-resolved: FR-001 still promises what the tiebreaker breaks
FR-001 (:110-118) now reads:
identical result sets (same items, same order, same pagination cursors) … except for the cross-site correctness gap addressed by … FR-004a … Because
ORDER BY mod_datealone has no tiebreaker … the fix MUST add a deterministic tiebreaker column.
The exception clause covers only FR-004a. Two sentences later the same FR mandates the change that makes "same order" false for the ~1.2% of tied rows. SC-003 got the tie carve-out (:220-224); FR-001 didn't, so the two now disagree.
And "same pagination cursors" is false by construction. contentCursor is a DB row offset (getContentByChunks / generateNextContentCursor), so changing ORDER BY changes which row sits at a given offset. Round 2 fixed the ordering wording in SC-003 and left both halves standing in FR-001.
Should fix
FR-010's validation scope wasn't extended to either change round 2 added. It still requires EXPLAIN ANALYZE only "with workflow (scheme/step) and per-field Tag/Relationship sub-queries present", but round 2 added two plan-relevant changes:
- The
ORDER BYtiebreaker.appendOrderByQuery(:2691-2698) emitsorder by c.mod_date asc|desc, and there's no index oncontentlet.mod_date—postgres.sqlhascontentlet_ident,contentlet_moduser,contentlet_lang, none onmod_date— so the sort is already an explicit sort node and adding, c.inodeis probably cheap. But in a spec whose whole purpose is plan stability (FR-002), a sort-key change belongs in the validation step rather than being assumed free. host_inodeback inside the CTE, which revives round-1 S2 — and S2 wasn't carried over. The unique index is(parent_path, asset_name, host_inode)(postgres.sql:1033), soasset_namesits between the two filtered columns:host_inodecan only be an index-tuple filter after theparent_pathprefix scan, never a range narrower. FR-003 forbids adding an index, so FR-010's EXPLAIN should confirm this stays index-only and doesn't force a heap access.
SC-003 now mixes three oracles into one sentence. After round 2 there are three classes of case:
| Case | Oracle |
|---|---|
| Untied, same-site rows | strict before/after identity |
mod_date-tied rows |
set identity + a new determinism assertion (there's no "before" order to match) |
| Cross-site rows (FR-004a) | cannot be a before/after comparison — expected to differ |
SC-003 states all three as one sentence with two trailing caveats. As a test oracle that's unbuildable; split it.
Round-1 S3 only partly addressed — "forced system host" is still two SQL shapes. FR-004 (:124-131) now lists "explicit site and forced system host", but that's two shapes the CTE has to reproduce: with a site → and (id.host_inode = ? or id.host_inode = 'SYSTEM_HOST') (:2189), without → and (id.host_inode = 'SYSTEM_HOST') (:2197).
FR-004a has no regression guard and no existing fixture. BrowserAPITest:1958-1973 and :2058-2073 do use ignoreSiteForFolders(true), but on single-site fixtures — they pin the flag's use, not its cross-site effect. Nothing in the suite asserts today's cross-site behaviour in either direction, so FR-004a would ship with no before-state pinned. It needs a new two-sites-sharing-a-relative-path fixture, which is the kind of setup ContentDriveHelperContentletAPIComparisonTest is built for — worth naming alongside BrowserAPITest.
Adjacent — not a gate item
With ignoreSiteForFolders(true) set for every specific-folder Drive request, shouldApplySiteFiltering is false, so the site block never runs and forceSystemHost is never consulted on the DB candidate scan — even though ContentDriveHelper:177 sets it unconditionally from requestForm.includeSystemHost(). So on a folder listing that toggle currently has no effect on the SQL path (it still matters on buildPureESQuery). Not this spec's problem, but if FR-004a does touch path 1, this is the flag whose behaviour changes with it.
Gate status and suggestion
Asks 3/4/5b are done and ask 2 was answered honestly. FR-004a is the blocker, and it needs a decision this document can't make: whether a Content Drive folder listing should be site-scoped. Until that's answered FR-004a is unimplementable as written — an implementer will pick a path, and either pick is a significant unreviewed behaviour decision.
My suggestion: split FR-004a out. The tiebreaker belongs here — it's ordering, the CTE touches ordering, and FR-001 is meaningless without it. The host_inode fix is a separate correctness change with a product question attached and its own rollback profile; bundling it costs this spec the "identical results" property that was its cleanest safety guarantee, and it doesn't need the CTE to ship. If it stays bundled, FR-004a should name path 2 explicitly (shouldApplySiteFiltering && site == null && !forceSystemHost), state that ignoreSiteForFolders keeps its current cross-site behaviour, and then say plainly that Content Drive is unaffected.
Worth flagging on sequencing: this spec is the hard dependency for #37188's SC-002 and bounds #37185's latency share, so it's the one whose approval unblocks other people.
Comment rather than a change request again, same as the others.
…sub-cases, FR-010 validation scope
… pagination-cursor claim
# Conflicts: # .gitignore
…esolve merge conflict with main
fabrizzio-dotCMS
left a comment
fabrizzio-dotCMS
left a comment
There was a problem hiding this comment.
Approved — the spec phase is cleared. This was the only one of the four still carrying blockers, and both are closed.
B1″ — you split it instead of guessing, which is the right call. #37347 is accurate and independently useful: it names shouldApplySiteFiltering = !ignoreSiteForFolders && folder != null, quotes ContentDriveHelper's own comment, blames it, and puts (a)/(b) as an open product question rather than presupposing the answer. And it says plainly that this fix doesn't depend on it — which is true, and which is what unblocks the CTE. The spec gets back its cleanest safety property (identical results, one named exception) and the host-scoping question lands where a product owner can actually answer it.
B2″ — FR-001. The exception is now bounded to tied mod_date rows, and you extended it to pagination cursors, which is the part I'd have missed if you hadn't. Verified: generateNextContentCursor returns startOfCurrentChunk + chunkInodesOrdered.indexOf(lastOnPage.getInode()) + 1 — a DB row offset computed off the DB-ordered inode list, so reordering ties does move the cursor value. Your derivation reproduces exactly.
S1″/S2″/S3″ all done: FR-010 now carries the tiebreaker into the EXPLAIN ANALYZE and says why (new query surface the spike never measured); SC-003 is three separate oracles, one per case; and the site sub-cases are enumerated as SQL shapes rather than labels — all four reproduce against selectQuery and appendSiteQuery/appendSystemHostQuery.
Three nits, none blocking, none worth another round — carry them into planning:
- FR-004's annotation contradicts its own body — the header still reads "three SQL variants, not two" while the body enumerates four. The body is right; the header is a stale label from the previous pass.
- The state list is one path short.
shouldApplySiteFiltering == true && site == null && !forceSystemHostalso falls through with no host predicate at all — a second route to (d)'s SQL shape thatignoreSiteForFoldersdoesn't cover. Harmless for a preserve-everything requirement, but "four distinct states today" is really five states collapsing onto four SQL shapes, and the FR-004 parity matrix shouldn't skip a reachable path. - Line refs drifted with the
mainmerge —BrowserAPIImpl.java:1958-1959is now2019-2020,:2113-2125is now2186-2199,ContentDriveHelper.java:161-166is now170-174(same drift in #37347's body). Every code claim reproduces; only the numbers moved.
One thing for FR-010 specifically: the index name in Assumptions is install-lineage-dependent. A fresh Postgres install gets identifier_parent_path_asset_name_host_inode_key from the inline unique (parent_path, asset_name, host_inode) on create table identifier, but an install upgraded through Task00785DataModelChanges gets identifier_unique_key for the same three columns in the same order. Column order — the part the folder-first CTE actually depends on — is identical on both paths, so just assert the columns in the EXPLAIN check, not the index name.
Nice work on the split in particular. Unbundling a product decision from a performance fix is the kind of thing that's easy to argue against and almost always right.
… per FR-010 re-validation (#37229) Re-checked against dotcms/dotcms:issue-37229-content-drive-folder-cte_SNAPSHOT (commit 95b6031) with real EXPLAIN ANALYZE + pg_stat_statements attribution, per FR-010's mandatory gate. SC-001 confirmed: the pathological /outreach/ case now enters via the correct index, combined with workflow/tag filters without reintroducing plan instability. SC-002's documented +25-30ms overhead was optimistic -- a second reference folder measured +40ms; widened to +25-40ms.
|
FR-010 (the mandatory EXPLAIN ANALYZE gate) was re-validated against the real dataset from #37148/#37183 — see PR #37397 for the full writeup. The fix is confirmed to do what it claims (SC-001), but the re-check found SC-002's documented "+25-30ms" overhead figure was optimistic: a second reference folder measured +40ms. Pushed a correction to SC-001 (confirmed) and SC-002 (widened to +25-40ms) on this spec branch. Since the spec is being edited after your original approval, this needs a re-approval per our sign-off process before PR #37397 merges. |
2 participants
Spec-Kit PR 1 of 2. Carries the spec alone. Needs a developer approval (not a merge) before /speckit-plan runs.
Resolves the spec phase of #37229.
Proposed Changes
spec.md— 2 prioritized user stories, 10 functional requirements (no open clarifications remaining — one former open question was reframed as FR-010, a required validation step rather than a decision; two others were resolved as scoping decisions inline), 5 success criteria, edge cases, and the dotCMS Legacy Considerations section.Summary
Implements the direction validated by spike #37183: wrapping the folder-scoped candidate lookup in
WITH folder_ids AS MATERIALIZED (...)before joining out tocontentlet_version_info/contentlet/structure. The spike measured this on a simplified representative query (parent_path + host_inode + deleted + lang only) directly against Postgres — this spec scopes applying it to the real, more complex queryBrowserAPIImplactually builds (workflow scheme/step, tag/relationship criteria, content-type include/exclude, MIME type, free-text/filename filtering, and three site-scoping sub-cases).No schema change required — an existing index (
identifier_parent_path_asset_name_host_inode_key) already supports the materialized candidate-set lookup.Scoping decisions made explicit in this spec
identifierrow already inside the CTE, not a new join. Including it also narrows the risk surface described below.EXPLAIN ANALYZEon the real query against a folder already known to trigger today's slow plan), not deferred to post-implementation discovery.Checklist
Additional Info
Parent epic #36814. Implements the direction from spike #37183 (full investigation and live-instance measurements). Coordinates with #37184 (shares this same query; this fix changes its shape, not how many times it executes per request).
/speckit-adr-contextconsulted ADR-0018 (database-first search for Content Drive) — this fix keeps folder-scoping and ordering fully DB-resolved via a different access path; no criterion moves to the index. No new ADR proposed.🤖 Generated with Claude Code
This PR fixes: #37229