CommitFest 7365 Review: Partial Indexes on System Catalogs

code-review · generated from canonical Markdown

Type
code-review

Source
postgresql-master

Slug
commitfest-7365-review

Tags

Updated
2026-10-05T18:54:19.117008+00:00

Verdict / Summaryneeds-review

Front matter

schema: review-report-v2
slug: commitfest-7365-review
source: postgresql-master
target: "CommitFest 7365: Partial indexes on system catalogs (uncommitted staged diff on master dfcf6fa7)"
base: dfcf6fa7749d1fc4660d03590f14dca7c855048c
head: dfcf6fa7749d1fc4660d03590f14dca7c855048c
diff_sha: 08e15b568004b821e8694150cf0662975324f350c3fe097b7c7e5315a5cc3a97
verdict: needs-review
blocker_counts:
  verified: 0
  unverified: 0
coverage_requirement: required
coverage_status: measured

CommitFest 7365 Review: Partial Indexes on System Catalogs

Verdict: needs-review

Blocker split: verified 0 / unverified 0. No blockers. One correctness MAJOR (misc_sanity test breakage, FIND-001) and one live-thread design objection MAJOR (FIND-002) drive the needs-review verdict per the deterministic rubric (correctness MAJOR + live objection => needs-review).

Patch Summary

This patch adds support for partial indexes on system catalogs with a restricted whitelist of predicate shapes that can be evaluated during catalog maintenance without building an EState:

oidcol <> k (OID inequality) — evaluated by direct slot_getattr comparison
col IS NOT NULL (NullTest) — evaluated by checking slot nullness

The patch modifies:

src/backend/bootstrap/bootparse.y — adds WHERE grammar production and makeBootCatalogPredicate() to hand-build the OpExpr predicate during bootstrap
src/backend/bootstrap/bootscanner.l — adds <> (NEQ) and WHERE tokens
src/backend/catalog/indexing.c — adds catalog_pred_clause_supported(), CheckCatalogIndexPredicate(), and CatalogIndexPredSatisfied(); integrates predicate evaluation into CatalogIndexInsert()
src/backend/commands/indexcmds.c — adds creation-time CheckCatalogIndexPredicate() call for catalog indexes
src/backend/catalog/genbki.pl — suppresses constraint generation for partial unique indexes
src/include/catalog/pg_constraint.h — declares 3 new partial indexes on pg_constraint (OIDs 580, 581, 582)
src/test/regress/ — adds catalog_partial_index regression test

Findings

FIND-001

— MAJOR (correctness): misc_sanity regression test breaks

Location: src/test/regress/expected/misc_sanity.out:97

Mechanism: The patch adds two new unique partial indexes on pg_constraint (pg_constraint_conrelid_conname_index OID 581, pg_constraint_contypid_conname_index OID 582). genbki.pl correctly suppresses pg_constraint constraint generation for partial unique indexes (the WHERE regex check at line 155), so these indexes have no backing pg_constraint entry. However, the existing misc_sanity regression test asserts that no system catalog unique index lacks a backing constraint ("There should be none"). The test's expected output was not updated.

Verification: verified by reproduction. Full make installcheck with an instrumented build: "1 of 240 tests failed" — misc_sanity.out.diff shows the 2 new unique partial indexes. Confirmed via SQL that both indexes have indisunique=true and no pg_constraint row with matching conindid. Applied the repair (updating expected output) to a scratch copy and re-ran misc_sanity: "All 1 tests passed."

Repair: Update src/test/regress/expected/misc_sanity.out to list the 2 new unique partial indexes as expected results. The test comment "(There should be none.)" should be updated to reflect that partial unique indexes intentionally lack backing constraints.

Repair validation: passed (apply-check + diff-check + targeted-test). The repair patch applies cleanly to HEAD; re-running misc_sanity with the updated expected output passes.

FIND-002

— MAJOR (correctness, cannot-verify): Live thread design objection

Location: src/backend/catalog/indexing.c:77 (overall design)

Mechanism: The patch's design approach is contested by two senior reviewers on pgsql-hackers thread 657827:

Tom Lane (2026-10-01): skeptical of the restricted whitelist — "we don't have partial indexes, just a hack with a small number of potential use-cases." Suggests pg_constraint is misdesigned and should be refactored via breakout catalogs instead.
Andres Freund (2026-10-01): questions whether the gain warrants the complexity, noting catcache negative-lookup caching may negate the benefit. Requests an actual benefiting index + syscache.

The patch author labels it "meant for experimentation, not for commit."

Verification: cannot-verify (design-acceptability objection, not a code defect). The thread is live with no resolution.

Repair: No code repair — this requires author response on pgsql-hackers addressing both reviewers' concerns, or a revised approach.

FIND-003

— MINOR (maintainability): Hardcoded operator OID

Location: src/backend/bootstrap/bootparse.y:128

Mechanism: op->opno = 608; uses a hardcoded operator OID literal for the oid <> operator instead of a named macro. The patch author explicitly acknowledges this: "dropping a hardcoded operator OID is still pending."

Verification: verified by static inspection. The cover letter lists this as pending work.

Repair: Replace literal 608 with a named constant (e.g., OID_NOT_EQUAL_OPERATOR). The exact macro name depends on project conventions; the illustrative example shows the intent.

Repair validation: partial (apply-check + diff-check + static-check). Full compile pending author confirmation of the canonical macro name.

COV-001

— MINOR (tests): Uncovered NullTest IS NOT NULL false-return path

Location: src/backend/catalog/indexing.c:164

Mechanism: Changed-code coverage identified an uncovered executable line: the NullTest IS NOT NULL false-return path in CatalogIndexPredSatisfied. When a partial catalog index has a column IS NOT NULL predicate and a catalog row is inserted where that column is NULL, line 164 (return false) should execute. The existing test only creates a contypid IS NOT NULL index on a NOT NULL column, so isnull is always false.

Verification: verified by coverage. gcov branch data for line 163 shows branch 0 (isnull true) taken 0%. Confirmed that pg_constraint.conbin is nullable and PK/UNIQUE/NOT NULL constraints have conbin=NULL. A test creating a conbin IS NOT NULL partial index + a PK table insert exercises this path.

Repair: Add a test section to catalog_partial_index.sql that creates a conbin IS NOT NULL partial index and creates a table with a PRIMARY KEY (inserting a pg_constraint row with conbin=NULL).

Repair validation: partial. Manual SQL execution confirmed the path is exercised; full pg_regress expected-output integration pending.

Verification Summary

Finding	Severity	Class	Status	Method
FIND-001	MAJOR	correctness	verified	repro
FIND-002	MAJOR	correctness	cannot-verify	static-check
FIND-003	MINOR	maintainability	verified	static-check
COV-001	MINOR	tests	verified	verified-by-coverage

Cannot-verify findings: FIND-002 is a design-acceptability objection (UNVERIFIED), not a code defect. It requires mailing-list resolution, not code verification.

Repair Validation Summary

Finding	Patch	Validation	Methods
FIND-001	provided (unified-diff)	passed	apply-check, diff-check, targeted-test
FIND-002	waived (design-choice)	waived	n/a
FIND-003	provided (unified-diff)	partial	apply-check, diff-check, static-check
COV-001	provided (unified-diff)	partial	apply-check, diff-check, targeted-test

FIND-001 repair is fully validated: the expected-output update applies to HEAD and the test passes with the updated output.

FIND-003 repair is partial: the structural change is correct but the exact macro name requires project-convention confirmation before compile validation.

COV-001 repair is partial: the test logic is verified by manual SQL execution but the full pg_regress expected-output integration is pending.

Changed-Code Coverage

Coverage requirement: required (4 changed instrumentable C/Y/L files)

Execution lane: host-disposable (passed)

Counter epoch: fresh (gcda reset before initdb; initdb captures bootstrap parser coverage; full installcheck captures runtime coverage)

Metrics

Metric	Count
Changed executable lines	83
Covered changed lines	78
Compiler-uncovered changed lines	5
Uncovered changed lines (semantic)	5
Structural uncovered lines	0
Changed branches	36
Covered changed branches	27
Uncovered branches	9

Invariant: covered (78) + compiler-uncovered (5) = changed executable lines (83) ✓

Uncovered Lines

File	Line	Function	Coverability	Reason
indexing.c	96	catalog_pred_clause_supported	infeasible	Defensive guard (neither OpExpr nor NullTest); enforced at creation
indexing.c	144	CatalogIndexPredSatisfied	infeasible	Defensive elog(ERROR); enforced at creation
indexing.c	164	CatalogIndexPredSatisfied	coverable-now	NullTest IS NOT NULL with NULL value; needs nullable-column test (COV-001)
bootparse.y	118	makeBootCatalogPredicate	infeasible	Unknown column error; requires malformed BKI
bootparse.y	121	makeBootCatalogPredicate	infeasible	Non-OID column error; requires malformed BKI

Uncovered Branches (9)

All 9 uncovered branches are defensive/infeasible except 1 coverable-now branch:

indexing.c:163 (isnull-true-NullTest) — coverable-now (COV-001 test covers it)
8 infeasible branches: defensive guards for unsupported clause shapes, NULL OID values, malformed BKI columns/types — all enforced at creation time or by NOT NULL constraints

Non-instrumented Changes

src/backend/catalog/genbki.pl — Perl script, validated functionally
src/include/catalog/indexing.h — header (declaration only)
src/include/catalog/pg_constraint.h — catalog header (data definition)
src/test/regress/ files — test files (exercised by pg_regress, not C-instrumented)

Test Commands

configure --enable-debug --enable-cassert CFLAGS='-O0 -g'
make -j4 && make install
# Rebuild changed objects with coverage:
make -C src/backend/catalog indexing.o CFLAGS='-O0 -g --coverage -fprofile-arcs -ftest-coverage'
make -C src/backend/commands indexcmds.o CFLAGS='-O0 -g --coverage -fprofile-arcs -ftest-coverage'
make -C src/backend/bootstrap bootparse.o bootscanner.o CFLAGS='-O0 -g --coverage -fprofile-arcs -ftest-coverage'
# Relink backend with coverage:
make -C src/backend postgres CFLAGS='-O0 -g --coverage -fprofile-arcs -ftest-coverage'
# Fresh epoch: initdb captures bootstrap, installcheck captures runtime
initdb -N --no-locale -E UTF8
pg_ctl start
make -C src/test/regress installcheck

Coverage Evidence

catalog_pred_clause_supported: called 12,549 times, 100% return, 86% blocks
CheckCatalogIndexPredicate: called 6 times, 83% return (1 ERROR for bad predicate), 94% blocks
CatalogIndexPredSatisfied: called 12,543 times, 100% return, 79% blocks
CatalogIndexInsert: called 182,597 times, 100% return, 72% blocks
makeBootCatalogPredicate: called 3 times (3 new partial indexes), 100% return, 67% blocks
DefineIndex: called 2,212 times, 93% return, 78% blocks
bootscanner <> token: 3 hits; WHERE token: 38 hits

Thread Reconciliation

Thread: pgsql-hackers #657827 "Partial indexes on system catalogs" (2026-10-01)

Status: reconciled (3 messages)

Message	Sender	Kind	Summary
179089136170...@gmail.com	Manuel Reyes Bravo	patch	Initial proof-of-concept; whitelist approach; acknowledges pending items
mvpdr22hi77...@7zh2ki5oeecm	Andres Freund	review	Skeptical: benefit/complexity tradeoff; needs real benefiting syscache
1298334.1790896057@sss.pgh.pa.us	Tom Lane	review	Skeptical: whitelist is a hack; suggests pg_constraint refactoring instead

Finding dispositions:

FIND-001 (misc_sanity break): new (not raised in thread)
FIND-002 (design objection): already-raised (Tom Lane + Andres Freund)
FIND-003 (hardcoded OID): already-raised (author acknowledges in cover letter)
COV-001 (coverage gap): new (from this review's coverage measurement)

Aggregated Test Plan

FIND-001 repair: Update misc_sanity.out expected output (verified passing)
COV-001 repair: Add conbin IS NOT NULL + PK insert test to catalog_partial_index.sql
Existing tests: catalog_partial_index (new test, passes), full installcheck (239/240 pass, 1 expected misc_sanity failure)

Residual Risk

The design approach is under active debate on pgsql-hackers with unresolved senior-reviewer objections (FIND-002). The patch may require significant rework or a different approach before acceptance.
The hardcoded OID (FIND-003) is a known pending item acknowledged by the author.
The misc_sanity test comment "(There should be none.)" becomes outdated with this design; the expected-output update should include a comment update explaining why partial unique indexes are exceptions.

Provenance

Model: glm-5.2:cloud (all seats)
Review seats: pg-patch-reviewer (generic review), pg-security-reviewer (security), pg-finding-verifier (finding/repair verification), pg-change-coverage-reviewer (coverage analysis)
Execution lane: host-disposable (VPATH build with gcov instrumentation, fresh initdb + full installcheck)
Coverage tools: gcc --coverage, gcov -b, gcovr 8.6