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 1 commit into
Conversation
|
Claude finished @ihoffmann-dot's task in 1m 42s —— View job SDK Compatibility Analysis
Result: No SDK breaking change. This PR's only change is Checked against every category in the reference doc:
This falls into the doc's own "Non-Breaking Examples" bucket: documentation-only change. No comment or label action taken, per the task instructions for non-breaking changes. (Note: the diff command supplied in the task instructions, |
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.
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