Combine Boolean filtering with BM25 ranking

[the-osiris]

Summary

• combine Boolean @@ filtering and BM25 ordering in one index scan
• adaptively materialize Boolean matches after ranked backoff and reject non-matching CTIDs before heap access
• preserve PostgreSQL rechecks and Boolean-only zero-score results

Performance

On a deterministic 200k-row corpus with five interleaved warm-cache runs:

• combined scan: 240.641 ms -> 51.949 ms median
• GIN filter + identical pg_textsearch BM25: 161.937 ms median
• rows removed by index recheck: ~34,237 -> 1,000

The combined scan returned the same top 20 as GIN + BM25.

Validation

• all 78 PostgreSQL 17 SQL regressions
• Boolean memory and NULL-key rescan guards
• focused boolean_queries, rescan, and filtered_seed regressions

Depends on #480.

[Copilot]

🟡 Changes recommended

Unresolved correctness, memory-bounding, and planner-handling issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Combines Boolean filtering with BM25 ranking in a single index scan, including adaptive CTID handling and zero-score Boolean results.

Changes:

• Adds combined planner and scan execution.
• Adds regression coverage and expected outputs.
• Documents combined-query behavior.

File summaries

File	Reviewed changes
test/sql/boolean_queries.sql	Combined-scan regression tests
test/expected/boolean_queries.out	Updated expected results
src/planner/cost.c	Combined-path planning support
src/access/scan.c	Ranking, filtering, deduplication, and Boolean-tail handling
src/access/am.h	Combined-scan state
README.md	Combined-query documentation

Review details

Suppressed comments (2)

src/access/scan.c:539

• The new result_count > max_entries branch is not exercised by the added combined-scan tests, which all use small match sets and therefore build the hash lookup. Please add a regression with a reduced work_mem and a match set above the threshold, checking both ranked results and the zero-score Boolean tail; otherwise the heap-recheck/dedup fallback can regress without detection.

	max_entries = (long)work_mem * 1024L / TP_BOOLEAN_FILTER_ENTRY_BYTES;
	if (so->result_count > max_entries)
		return;

src/planner/cost.c:204

• boolean_full_scan is only computed in the !has_orderby branch, so combined queries such as WHERE body @@ '!term' ORDER BY ... are costed as ordinary top-k scans even though tp_boolean_execute() must enumerate and materialize the full Boolean universe when the ranked batch is rejected. The planner can therefore choose this path for a small LIMIT while paying for a full index scan and temporary result file; include the full-scan cost/selectivity for these combined predicates or keep them on the fallback path.

	if (has_boolean && !has_orderby)
	{
		if (!tp_boolean_get_constant_query(path, &boolean_query))
			boolean_full_scan = true;

• Files reviewed: 6/6 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[claude]

Beyond the two inline findings, I also checked whether tp_boolean_filter_prepare/tp_begin_combined_boolean_tail could silently miss rows committed concurrently while the combined scan is in flight — that path follows the same snapshot/MVCC semantics as any other index scan re-execution here, so it's not a new correctness regression introduced by this diff.

Extended reasoning...

Two confirmed findings are already queued as inline comments: an unbounded exact-operand-count guard now reachable through the combined Boolean+ORDER BY path in src/planner/cost.c, and an unbounded materialization loop in the combined-scan tail of src/access/scan.c that can be driven by LIMIT-less queries (exactly the idiom CLAUDE.md recommends for counting matches). Given findings exist, this is not an approve. I additionally looked at the concurrent-write visibility angle for the newly-introduced Boolean-tail replay path (tp_boolean_filter_prepare re-executing/reusing the materialized match set, then tp_begin_combined_boolean_tail replaying it) and concluded it relies on ordinary index-scan snapshot semantics rather than introducing a new staleness or omission bug, so I'm not raising it as a separate finding. That check is concrete enough to be worth surfacing per the narrow exception for findings-present runs, but doesn't change the overall need for a human look given the two confirmed issues already flagged inline.

[tjgreen42]

@the-osiris Check the AI feedback on this one, I'll take a look once the robots are happy :)

[the-osiris]

Also added low-work_mem fallback coverage and full-materialization costing for negative, prefix, weight, and parameterized queries. Fixed in e0b7e10.

[codecov]

Codecov Report

❌ Patch coverage is 81.42857% with 26 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/access/scan.c	75.72%	25 Missing ⚠️
src/planner/cost.c	97.29%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[Copilot]

🟡 Changes recommended

The planner’s cap fallback ignores unflushed memtable-chain documents and can allow a combined scan to hit a runtime candidate-limit error.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

Copilot review overview

🟡 Changes recommended

The planner's shared-lock chain count can miss concurrent appends, allowing a combined plan to hit the runtime candidate cap and error instead of safely falling back.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Make planner cap check safe against concurrent memtable appends · New

Resolved since last review (1)

• num_tuples comes only from metap->total_docs, but that field explicitly excludes unflushed…