schema: review-report-v2
slug: commitfest-7392-review
source: postgresql-master
target: CommitFest 7392 - Let an ordering index scan hand its ORDER BY value to the target list (amorderbyvalsexact)
head: 3ff475ac12ec8d8d07cf3fd86144659285a59d83
base: 3ff475ac12ec8d8d07cf3fd86144659285a59d83
verdict: ready-for-committer (test follow-up required)
blocker_counts:
  verified: 0
  unverified: 0
coverage_status: measured
coverage_epoch: fresh
changed_executable_lines: 188
covered_changed_lines: 72
uncovered_changed_lines: 6
changed_branches: 56
covered_changed_branches: 43

CommitFest 7392 Review: amorderbyvalsexact — ORDER BY values from index scan to targetlist

Summary

This patch adds a new amorderbyvalsexact flag to IndexAmRoutine that lets an ordering index scan (GiST, SP-GiST) hand the AM's ORDER BY distance values directly to the query's target list, instead of re-evaluating the ORDER BY expression on the heap tuple. This is a performance optimization for AMs whose reported distance values are exact (not approximate), and a correctness fix for AMs (like BM25 extensions) where re-evaluating the operator would produce different values than what the index reported.

The patch modifies the planner (setrefs.c, createplan.c, plancat.c) to rewrite targetlist entries matching ORDER BY expressions into INNER_VAR references to a virtual slot filled by the executor, the executor (nodeIndexscan.c) to populate that slot with the AM's values, EXPLAIN and ruleutils.c to deparse the new Vars correctly, and adds comprehensive regression tests in gist.sql.

Verdict

ready-for-committer (test follow-up required)

No blockers, no correctness or security MAJORs. One test-only MAJOR (COV-1) for the uncovered EvalPlanQual recheck path in IndexRecheck.

Findings

Verification Summary

All findings verified. No refuted findings. No cannot-verify findings.

Repair Validation Summary

The isolation test spec was created, compiled, and run successfully against a temp PostgreSQL instance. The test exercises the concurrent-update scenario that triggers EvalPlanQual recheck through IndexRecheck. Full validation requires running the isolation test against the coverage-instrumented build.

Changed-Code Coverage

Measurement

• Build: meson with -Db_coverage=true -Dcassert=true -Doptimization=2
• Tests: postgresql:regress/regress (239 subtests), postgresql:btree_gist/regress (32 subtests), postgresql:spgist_name_ops/regress, postgresql:cube/regress
• Counter epoch: fresh (disposable build, no prior counters)
• Execution lane: host-disposable (passed)
• Total test runs: 865

Metrics

Uncovered Regions

1. src/backend/executor/nodeIndexscan.c:438-450 (function: IndexRecheck)

• Coverability: requires-special-harness-or-injection-point
• Reason: EvalPlanQual recheck callback, only invoked during concurrent

modification under READ COMMITTED with row locking. Requires an isolation test.

• Test example: isolation test spec (see COV-1 repair)

1. src/backend/utils/adt/ruleutils.c:8016 (function: resolve_special_varno)

• Coverability: infeasible-with-reason
• Reason: Defensive elog(ERROR) for corrupted-plan invariant

(bogus varattno). replace_orderby_tlist_refs only emits valid varattno values; this branch is an assertion guard.

• Waiver: not-instrumentable — defensive error check, unreachable via valid SQL

Non-instrumented Changes

• src/include/access/amapi.h — header, exercised via including C files
• src/include/nodes/execnodes.h — header, exercised via including C files
• src/include/nodes/pathnodes.h — header, exercised via including C files
• src/include/nodes/plannodes.h — header, exercised via including C files
• doc/src/sgml/indexam.sgml — documentation
• src/test/regress/expected/box.out — test expected output
• src/test/regress/expected/gist.out — test expected output
• src/test/regress/expected/polygon.out — test expected output
• src/test/regress/sql/gist.sql — test SQL

pgsql-hackers Thread Reconciliation

Thread found: One thread on pgsql-hackers.

• Subject: "Let an ordering index scan hand its ORDER BY value to the target list"
• Author: Greg Burd (greg@burd.me)
• Date: 2026-10-01
• Message-ID: 8s5lT8erXzBMugXJQ6Wginbp_gc2B4hmCYhF0Q0GpnuK87eCAOcKs5K31AWCwraCNFIQt42wwkow_MPPubHg485MWv-zDBY4NL_JX9yJEEg=@burd.me
• Replies: None (original patch submission only)

The patch author explains the motivation (BM25/full-text-search extensions where recomputing the operator gives incorrect results) and references prior discussion by Tom Lane about the subtlety of making operator values index-dependent. No review objections have been raised.

Review Analysis

Design

The patch introduces a new amorderbyvalsexact boolean on IndexAmRoutine, propagated through IndexOptInfo and IndexScan plan nodes. When set, the planner's setrefs.c replaces targetlist expressions that are equal() to ORDER BY expressions with Var(INNER_VAR, i+1) references. The executor fills a virtual slot (iss_OrderBySlot) with the AM's ORDER BY values and installs it as the projection's inner tuple.

This mirrors the existing INDEX_VAR mechanism for Index Only Scan, which is the correct design precedent. The use of INNER_VAR for a scan node with no inner plan is safe because INNER_VAR is otherwise unused for scan nodes.

Correctness

• The equal() match in replace_orderby_tlist_refs correctly identifies

targetlist entries that are the same expression as an ORDER BY key. Only top-level expressions are matched (not subexpressions), which is conservative and correct.

• The type restriction to FLOAT8OID and FLOAT4OID is correct: those are the

only types index_store_float8_orderby_distances() can deliver.

• The IndexRecheck modification correctly recomputes ORDER BY values from the

heap tuple during EPQ, ensuring the displayed values match the operator's computation rather than stale index values.

• The reorder-queue path in IndexNextWithReorder correctly stores the queue

entry's ORDER BY values before the pop frees them.

Security

No security issues identified. The patch does not introduce any new privilege-escalation paths, search-path vulnerabilities, or memory-safety concerns. The virtual slot is properly allocated and managed within the executor's tuple table.

Test Coverage

The patch includes comprehensive regression tests in gist.sql that verify:

• The counting-function test proves the operator is not re-evaluated

(sequence counter stays at zero).

• The lossy box opclass test exercises the recheck and reorder-queue paths.
• The FOR UPDATE test exercises row locking with ORDER BY value display.
• EXPLAIN output shows the "Order By Values Used" property.

The only gap is the EPQ recheck path (COV-1), which requires an isolation test.

Aggregated Test Plan

1. Existing: src/test/regress/sql/gist.sql — exercises exact, recheck,

reorder-queue, and FOR UPDATE paths (passing)

1. Existing: src/test/regress/sql/box.sql, polygon.sql — EXPLAIN output

changes (passing)

1. Proposed: src/test/isolation/specs/gist-orderby-epq.spec — isolation

test for EPQ recheck path (COV-1 repair, not yet applied)

Residual Risk

Low. The patch is well-structured, the design follows established patterns (INDEX_VAR for Index Only Scan), and the type restriction to float8/float4 limits the blast radius. The main risk is the uncovered EPQ path, which is addressed by the proposed isolation test.

Provenance

• Model: glm-5.2:cloud
• Build: meson + gcc 15.2.0 with coverage instrumentation
• Tests: 239 + 32 + 1 + 2 subtests passed
• Coverage: gcov -b -f -l -p per changed .c file