schema: review-report-v2
source: postgresql-master
slug: commitfest-6960-review
target: commitfest-6960 COALESCE selectivity estimation in src/backend/utils/adt/selfuncs.c (uncommitted staged change vs HEAD a20d3e35)
base: a20d3e35f2c00938c5329cbf570160184f4f0a69
head: a20d3e35f2c00938c5329cbf570160184f4f0a69
diff_sha: 9e1c7ac320c0085792d033848aefaaf1a62451bf
verdict: needs-review
blocker_counts:
  verified: 0
  unverified: 0

Review Report — CommitFest 6960: COALESCE Selectivity Estimation

Patch: COALESCE selectivity estimation in src/backend/utils/adt/selfuncs.c (+434 / -30 lines) Author: Egor Savelev (Tantor Labs) — pgsql-hackers thread 650175 Verdict: needs-review Blockers: 0 verified, 0 unverified

Executive Summary

The patch adds COALESCE selectivity estimation to selfuncs.c, decomposing COALESCE(...) = ... predicates and hash keys into reach-probability-weighted sums of per-branch selectivities. The approach is architecturally sound and the common cases (COALESCE of Vars, COALESCE with a trailing const) are estimated well. The build is clean with --enable-cassert.

Two MAJOR correctness defects are verified and must be fixed before commit:

1. PR-1 (neqsel <> bypass): COALESCE(a,1) <> 5 returns the un-negated eq estimate (~10187 rows) instead of the negated estimate (~90000 rows) — off by ~9x. This is corroborated live in pgsql-hackers thread 650175 (Ilya Evdokimov, 2026-08-17). Repair validated: COALESCE(a,1) <> 5 → 89703 rows after fix.

1. PR-3 (is_eqjoin dispatch broken under DirectFunctionCall): try_coalesce_eq detects eqjoin via fcinfo->flinfo->fn_oid, but DirectFunctionCall*Coll sets flinfo=NULL, so nested-COALESCE join sub-pairs are dispatched to eqsel instead of eqjoinsel. Edge case reproduced (5000 → 1000 rows after fix). Common cases bail harmlessly.

One MAJOR process gap (PR-4): no regression tests added. A proposal-only test suite (13 scenarios) was produced and validated.

Version discrepancy: the frozen worktree is v3 (2026-08-07), not the latest v4 (2026-08-17). v4 moved the leading-Const check before the loop and changed the early-stop threshold to a product — neither change is in the frozen diff.

Findings Summary

Verification tally: verified = 6 (PR-1, PR-3, PR-4, SEC-2, COV-1, COV-2); cannot-verify = 7; refuted = 0.

Verification Summary

Repair Validation Summary

Combined repair patch (PR-1, PR-2, PR-3, PR-8, PR-10, SEC-2)

A single HEAD-bound unified diff covering all verified findings plus cosmetic hardening for cannot-verify findings:

• patch sha256: b0a446c39e6ee826006569a276c61d138509a967d3df7870716489d70611a38a
• apply-check: git apply --check exit 0 ✅
• compile: make -s -j4 -C src/backend clean (no warnings) ✅
• targeted tests (port 55556, repaired cluster):
• PR-1: COALESCE(a,1)<>5 → 89703 (was 10187) ✅
• PR-3: nested-COALESCE-join with expr-index stats → 1000 (was 5000) ✅
• No regressions across the 13-scenario test matrix ✅
• No ERROR/FATAL/PANIC in any test run ✅

Changed-Code Coverage

Status: measured (fresh disposable counter epoch, coverage-instrumented build) Execution lane: host-disposable (coverage-instrumented PostgreSQL 20devel on port 55555)

Test commands

1. psql -p 55555 -d postgres -v ON_ERROR_STOP=1 -f test-proposal/selectivity_coalesce.sql (13-scenario proposal suite)
2. psql -p 55555 -d postgres -v ON_ERROR_STOP=0 -f src/test/regress/sql/stats.sql (existing stats regress test)

Metrics (changed implementation lines only)

All 8 changed/added functions are entered at least once (no wholly-dead function).

Uncovered changed lines (28 total)

Coverable-now (4 regions, 9 lines):

• L487-488 try_coalesce_eq: left-non-coalesce single-element-list (no test places plain operand on LEFT with COALESCE on RIGHT) — COV-1 finding
• L4716 hash_bucket_stats_coalesce_dispatch: non-COALESCE hash key with default ndistinct reaches dispatch but match fails
• L4723-4725 hash_bucket_stats_coalesce_dispatch: COALESCE hash key with unanalyzed branch → get_coalesce_prefix_probs false
• L4751-4754 hash_bucket_stats_coalesce_dispatch: COALESCE hash key branch with isdefault ndistinct

Requires-special-harness-or-injection-point (2 regions, 2 lines):

• L339 match_coalesce_join_side: RelabelType strip loop body (no test coerces a COALESCE branch type)
• L4787 hash_bucket_stats_coalesce_dispatch: nd_mix > nbuckets branch — COV-2 finding (not reachable by plain SQL)

Infeasible-with-reason (11 regions, 17 lines): defensive guards unreachable via normal SQL due to eval_const_expressions folding, parser type resolution, CLAMP_PROBABILITY clamps, or operator arity guarantees. These are design-choice waivers, not findings.

Touched functions diagnostics

Non-instrumentable changes

None — the patch is entirely C implementation in src/backend/utils/adt/selfuncs.c.

Residual limitations

• nbuckets (and thus the COV-2 branch) is derived internally by the executor from work_mem; a test-only injection point is required to force nd_mix > nbuckets.
• The .gcov branch evidence uses the gcov -b interleaved N* partial-marker convention; branch totals are derived from conditional/loop branch outcomes.
• Coverage reflects line/branch execution, not assertion quality; the proposal suite's row-count oracles pin behavior but do not by themselves prove estimator correctness.

pgsql-hackers Thread Ledger

Thread: 650175 ("COALESCE patch") — reconciled Ledger report: commitfest-6960-review-thread-ledger Messages: 9 (2026-06-30 to 2026-08-17)

Key thread events

• v1 (2026-06-30): Egor Savelev cover letter with the COALESCE(a,b)=42 motivating example.
• 2026-07-10: Laurenz Albe review — idea good, worried about O(n*m) cost, v1 didn't improve common case, gave four EXPLAIN test cases.
• v2 (2026-07-10): fixes const/const and join-with-const-on-both-sides.
• 2026-07-16: Laurenz Albe review — "works fine", asks for more comments in try_coalesce_eq, notes no tests added.
• v3 (2026-08-07): addresses review, removes <2 args check, adds comments.
• 2026-08-07: Alexandre Felipe review — suggests leading-const check before loop, product threshold.
• v4 (2026-08-17): moved leading-Const check before loop, changed early-stop to product threshold. Frozen worktree is v3, not v4.
• 2026-08-17: Ilya Evdokimov live objection — COALESCE(a,1)<>5 estimate not recoverable (10260 vs 90057). Unreplied by author. Corroborates PR-1.

Finding dispositions

• PR-1 (neqsel <>): new (live objection in msg #9, unreplied)
• PR-4 (no tests): accepted (acknowledged by Laurenz Albe, msg #5)
• All other findings: new (not raised on -hackers; found by review)

Aggregated Test Plan

1. selectivity_coalesce.sql (proposal-only, 13 scenarios): controlled-distribution EXPLAIN row-count assertions. Apply-checked clean; loaded against cluster with deterministic output. Expected output requires correction for SEMI/ANTI scenarios (1000/1, not 1/999) and the COV-1 scenario (8, not 2).
2. COV-1 supplement: b = COALESCE(a,b) restriction + t1.b = COALESCE(t2.c,t2.d) join (validated, rows=8 and 1000).
3. COV-2 supplement: small-work_mem hash-join scenario (runs cleanly but does not cover line 4787; requires injection point).
4. PR-1 regression oracle: pr1_neq_estimate ~= 1000 - eq_rows - coalesce_nullfrac (after fix: ~998, was 2).
5. PR-3 regression oracle: nested-COALESCE-join with expr-index stats → ~1000 (was 5000 default).

Verified-Clean (explicitly cleared)

• Recursion termination: try_coalesce_eq recursion terminates for both the common case (Var/Const branches → inner bail) and the nested-COALESCE case (each level peels one layer; bounded by parse tree). No infinite recursion.
• **DirectFunctionCall*Coll flinfo=NULL:** the fcinfo->flinfo != NULL guard prevents an NPE. No NPE.
• ReleaseVariableStats pairing: verified on every early-return and error path. No missing release, no double-release.
• get_variable_mcv_freq histogram branch: flags=0 allocates nothing; not a leak. Faithfully reproduces HEAD behavior.
• palloc(0): list_length(left_args) is always ≥ 1. No zero-size palloc.
• Double-free / UAF: list_free(result) is shallow (shared nodes not freed). list_free(sub_args) is shallow (copies leaked, PR-9, not double-free).
• catversion/docs/pg_dump/OIDs: no catalog changes; no catversion bump needed.
• Security: no privilege escalation, no SQL injection, no catalog races, no new individual-value statistics leaks. The stanullfrac reads are consistent with established PG policy.

Provenance / Model Routes

Residual Risk

• The <> (neqsel) defect (PR-1) produces silently wrong estimates for COALESCE(...) <> const predicates — a realistic pattern over nullable fallback columns. Should be fixed before commit.
• The is_eqjoin dispatch defect (PR-3) is wrong-by-design but only triggers for nested COALESCE with expressional-index stats. Should be fixed before commit.
• The frozen worktree is v3, not the latest v4; the author's v4 changes (leading-Const check placement, product threshold) are not in the frozen diff.
• The <> live objection (msg #9) is unreplied; the thread is not marked Ready-for-Committer.

Deterministic Verdict

needs-review — verified correctness MAJORs (PR-1, PR-3) require author action. No BLOCKERs. The test MAJOR (PR-4) is addressed by the proposal-only test suite. The combined repair patch is validated but is a proposal (not applied to the read-only worktree).