schema: review-report-v2
target: "CommitFest patch 6397 - examine_variable ignored CollateExpr"
base: 45817676632b46fc9d2b97c8d432348e0ee4f1f4
head: 45817676632b46fc9d2b97c8d432348e0ee4f1f4
diff_sha: 0b527f177011ccb090faaad303456d9f4248c41d
verdict: needs-review
blocker_counts: {verified: 0, unverified: 0}
source: postgresql-master
slug: commitfest-6397-review
model: glm-5.2:cloud

CommitFest 6397 Review — examine_variable ignored CollateExpr

Verdict

• Verdict: needs-review
• Blocker counts: verified=0, unverified=0
• Target: CommitFest patch 6397 - examine_variable ignored CollateExpr
• Base = HEAD: 45817676632b46fc9d2b97c8d432348e0ee4f1f4 (uncommitted/staged target)
• Diff SHA (blob): 0b527f177011ccb090faaad303456d9f4248c41d

Rationale: The core collation-aware RelabelType fix is correct and aligns with the pgsql-hackers thread direction (in-place normalization, not eval_const_expressions, per Tom Lane's feedback). However, H2 (a correctness MAJOR) identifies that the expressional-index matching loop at selfuncs.c:5828 was left on the old naive single-level RelabelType strip, making it asymmetric with the new collation-aware query-side normalization — this produces both false misses and false matches for expressional indexes built on COLLATE expressions. This is a new correctness issue not raised on the thread and warrants an on-list decision before commit. No blockers.

Scope

• Changed function: examine_variable (src/backend/utils/adt/selfuncs.c, lines 5652–6098)
• Changed files (staged):
• src/backend/utils/adt/selfuncs.c (+57/-5)
• src/test/regress/expected/collate.icu.utf8.out (+49)
• src/test/regress/sql/collate.icu.utf8.sql (+32)
• No catalog/version bump (planner-only; no stored/parse-tree format change); no pg_dump impact.

Patch summary

examine_variable() strips RelabelType nodes before statistics lookup. The old code did while (IsA(basenode, RelabelType)) basenode = arg; unconditionally, discarding all relabels including collation-changing ones — conflating stats for GROUP BY a COLLATE C and GROUP BY a COLLATE case_insensitive. The patch replaces this in two places with collation-aware logic: walk nested relabels, and if the deepest expr's collation equals the relabel's resultcollid, strip all relabels; otherwise rebuild a single RelabelType preserving the outermost type/typmod/resultcollid/format/location wrapping the deepest expr. Added tests verify selectivity estimates for extended statistics with collations.

Findings

H2 — MAJOR (correctness) — verified

Location: src/backend/utils/adt/selfuncs.c:5828

Mechanism: The expressional-index matching loop at line 5828 retained the old naive single-level RelabelType strip (if (indexkey && IsA(indexkey, RelabelType)) indexkey = (Node *)((RelabelType *)indexkey)->arg;) while the query-side basenode became collation-aware via the new top-of-function normalization. The two sides of the subsequent equal(node, indexkey) comparison are now normalized asymmetrically: the query side preserves a collation-carrying RelabelType, the index side strips all RelabelTypes. This produces a false miss when the query collation matches the index (index stats not applied; estimate 25 vs actual 192 in the repro) and a false match when the query collation differs (default-collation query consumes a case_insensitive index's pg_statistic row, applying wrong-collation MCV frequencies).

Failure: Wrong selectivity estimates for expressional indexes built on COLLATE expressions: matching-collation queries fail to use index expression statistics (estimate 25 vs actual 192) and mismatched-collation queries wrongly consume the index's stats (estimate 192 from case_insensitive stats for a default-collation query). Reproduced via EXPLAIN with a temp ICU cluster: CREATE INDEX t1_low_ci ON t1 ((lower(a) COLLATE case_insensitive)); three WHERE-clause variants showed the asymmetry in debug elog of equal(node, indexkey).

Verification: live-repro — Disposable scratch ICU cluster (initdb with icu provider) with the cassert+debug+coverage build at the frozen staged tree. Three queries with collation match/default/C were run through EXPLAIN with a temporary elog(LOG) instrumenting equal(node,indexkey) at the index path. Pre-repair: matching-collation estimated 25 (no stats, false miss); default-collation estimated 192 (false match consuming case_insensitive index stats). Post-repair (strip_relabel_for_stats on indexkey): matching collation estimates 192 (stats applied), mismatches estimate 25 (no stats). The defect pass_on_new=false (present on the patched tree).

Repair instruction:

• Files: src/backend/utils/adt/selfuncs.c, src/test/regress/sql/collate.icu.utf8.sql, src/test/regress/expected/collate.icu.utf8.out
• Locations: examine_variable index-expression loop, ~line 5828 (the if (indexkey && IsA(indexkey, RelabelType)) block)
• Exact edits: Replace the single-level if (indexkey && IsA(indexkey, RelabelType)) indexkey = (Node *)((RelabelType *)indexkey)->arg; with a collation-aware normalization indexkey = strip_relabel_for_stats(indexkey, index->indexcollations[pos]); using the index column's collation from IndexOptInfo.indexcollations, so the index side and query side are symmetric. Factor the shared normalization (with the top and extended-stats sites) into a static helper strip_relabel_for_stats() defined before strip_all_phvs_deep.
• Postcondition: equal(node, indexkey) at the index-expression path matches iff the query expression and the index expression are semantically equal including collation; a matching-collation query consumes the index expression stats, a mismatched-collation query does not.

Suggested code example (provided, illustrative):

/* helper replaces both duplicated blocks and the index path */
static Node *
strip_relabel_for_stats(Node *node, Oid targetcollid)
{
    RelabelType *relabel;
    Expr       *expr;
    if (IsA(node, RelabelType))
    {
        relabel = castNode(RelabelType, node);
        expr = (Expr *) relabel;
        while (expr && IsA(expr, RelabelType))
            expr = ((RelabelType *) expr)->arg;
    }
    else
    {
        relabel = NULL;
        expr = (Expr *) node;
    }
    if (exprCollation((Node *) expr) == targetcollid)
        return (Node *) expr;
    {
        RelabelType *newrelabel = makeNode(RelabelType);
        newrelabel->arg = (Expr *) expr;
        if (relabel)
        {
            newrelabel->resulttype = relabel->resulttype;
            newrelabel->resulttypmod = relabel->resulttypmod;
            newrelabel->relabelformat = relabel->relabelformat;
            newrelabel->location = relabel->location;
        }
        else
        {
            newrelabel->resulttype = exprType((Node *) expr);
            newrelabel->resulttypmod = exprTypmod((Node *) expr);
            newrelabel->relabelformat = COERCE_IMPLICIT_CAST;
            newrelabel->location = -1;
        }
        newrelabel->resultcollid = targetcollid;
        return (Node *) newrelabel;
    }
}

Consolidated repair patch: HEAD-bound unified diff (applies_to_head=45817676632b46fc9d2b97c8d432348e0ee4f1f4, sha256=3307d0c72e09a30b3df9bf1a2465c01859c7638644117c0318b1c8cc11eacb83).

Repair validation: passed via methods: apply-check, diff-check, compile, targeted-test, repro, static-check.

Evidence: apply-check (patch -p1 --dry-run) reported all three files checked with no errors; apply succeeded; diff -q of applied files vs intended repaired files all MATCH; make -C src/backend/utils/adt selfuncs.o compiled clean (no warnings); make -C src/backend postgres linked clean; repro in temp ICU cluster: three H2 queries produced corrected estimates (matching-collation 192 uses stats, mismatches 25 fall back); regression: new index-path test section produced zero diffs vs updated expected output. The only remaining collate.icu.utf8 diff is a pre-existing eager_agg plan-shape mismatch (HashAggregate->GroupAggregate at line ~3675) present in the staged tree before any repair.

Test example (vehicle: regress, status: provided):

CREATE INDEX test_stats_ext_coll_idx ON test_stats_ext_coll ((lower(a) COLLATE case_insensitive));
ANALYZE test_stats_ext_coll;
SELECT * FROM check_estimated_rows ($$ SELECT * FROM test_stats_ext_coll WHERE (lower(a) COLLATE case_insensitive) = 'a' $$);
SELECT * FROM check_estimated_rows ($$ SELECT * FROM test_stats_ext_coll WHERE lower(a) = 'a' $$);
SELECT * FROM check_estimated_rows ($$ SELECT * FROM test_stats_ext_coll WHERE (lower(a) COLLATE "C") = 'a' $$);

Oracle: matching-collation estimate 19 (uses index expression stats, actual 19); default-collation estimate 2 (no match, actual 19); C-collation estimate 2 (no match, actual 19). Pre-fix the matching case estimated 25 (false miss) and the default case estimated ~19 (false match).

Baseline: Pre-fix: matching collation rows=25 (false miss); default collation rows=~19 (false match); C collation rows=25.

Patched: Post-fix: matching collation rows=19 (stats applied); default collation rows=2 (correct fallback); C collation rows=2 (correct fallback).

M1 — MINOR (maintainability) — verified

Location: src/backend/utils/adt/selfuncs.c:5680

Mechanism: The two hunks at lines 5680-5706 (top-of-function basenode normalization) and 5954-5984 (extended-stats expr normalization) duplicate near-identical 27-line logic (variable renamed expr<->rexpr). A third divergent RelabelType strip exists at line 5828 (the index path) on the OLD single-level form. Duplicated subtle collation logic in the same function is a maintainability hazard: a future fix to one copy will likely miss the other (the index-path drift already demonstrates this class of bug).

Failure: Maintainability: two copies of subtle collation-aware normalization that must stay in sync; divergent index-path copy already caused H2.

Verification: code-comparison — git diff --cached shows two byte-for-byte identical blocks (only local var name differs); refactor into strip_relabel_for_stats() reproduced the exact logic; compile clean; extended-stats test output unchanged after refactor.

Repair instruction:

• Files: src/backend/utils/adt/selfuncs.c, src/test/regress/sql/collate.icu.utf8.sql, src/test/regress/expected/collate.icu.utf8.out
• Locations: top-of-function normalization (~5680), extended-stats normalization (~5954), new helper definition before strip_all_phvs_deep (~6042)
• Exact edits: Factor the duplicated normalization block into a static helper strip_relabel_for_stats(Node *node, Oid targetcollid) and replace both duplicated blocks with one-line calls: basenode = strip_relabel_for_stats(basenode, exprCollation(basenode)); and expr = strip_relabel_for_stats(expr, exprCollation(expr));. Add a forward declaration next to strip_all_phvs_deep's.
• Postcondition: Single canonical implementation of collation-aware RelabelType normalization; both call sites use the helper; behavior unchanged for the query and extended-stats paths.

Suggested code example (provided, illustrative):

static Node *
strip_relabel_for_stats(Node *node, Oid targetcollid)
{
    RelabelType *relabel;
    Expr       *expr;
    if (IsA(node, RelabelType))
    {
        relabel = castNode(RelabelType, node);
        expr = (Expr *) relabel;
        while (expr && IsA(expr, RelabelType))
            expr = ((RelabelType *) expr)->arg;
    }
    else
    {
        relabel = NULL;
        expr = (Expr *) node;
    }
    if (exprCollation((Node *) expr) == targetcollid)
        return (Node *) expr;
    {
        RelabelType *newrelabel = makeNode(RelabelType);
        newrelabel->arg = (Expr *) expr;
        if (relabel) { newrelabel->resulttype = relabel->resulttype; newrelabel->resulttypmod = relabel->resulttypmod; newrelabel->relabelformat = relabel->relabelformat; newrelabel->location = relabel->location; }
        else { newrelabel->resulttype = exprType((Node *) expr); newrelabel->resulttypmod = exprTypmod((Node *) expr); newrelabel->relabelformat = COERCE_IMPLICIT_CAST; newrelabel->location = -1; }
        newrelabel->resultcollid = targetcollid;
        return (Node *) newrelabel;
    }
}

Consolidated repair patch: HEAD-bound unified diff (applies_to_head=45817676632b46fc9d2b97c8d432348e0ee4f1f4, sha256=3307d0c72e09a30b3df9bf1a2465c01859c7638644117c0318b1c8cc11eacb83).

Repair validation: passed via methods: apply-check, diff-check, compile, static-check, targeted-test.

Evidence: Refactor compiled clean; static check confirms the helper reproduces the prior logic for the query and extended-stats paths (targetcollid == relabel->resultcollid case); extended-stats test output unchanged after refactor.

Test example (vehicle: inspection, status: provided):

Both the query-side (basenode) and extended-stats-side (expr) normalization blocks reduce to one-line calls to strip_relabel_for_stats(); the helper definition is the single source of truth.

Oracle: The existing extended-stats tests (GROUP BY a COLLATE C/POSIX/case_insensitive/default) continue to pass with estimates 52/52/32/52 after the refactor.

Baseline: Pre-refactor: two duplicated 27-line blocks produce estimates 52/52/32/52.

Patched: Post-refactor: helper-based calls produce identical estimates 52/52/32/52.

H1 — MAJOR (maintainability) — verified

Location: src/backend/utils/adt/selfuncs.c:5685

Mechanism: The while-loop discards intermediate RelabelTypes and the else-branch rebuilds only with the outermost resultcollid. This is semantically correct (RelabelType composition is right-associative, only the outermost resultcollid is visible) but the invariant is undocumented, risking a future maintainer incorrectly changing it. applyRelabelType in src/backend/nodes/nodeFuncs.c:644-650 relies on the same invariant ('If we find stacked RelabelTypes (eg, from foo::int::oid) we can discard all but the top one').

Failure: Maintainability: an undocumented semantic invariant that the outermost RelabelType's resultcollid is the only one that matters; without a comment a future maintainer may 'fix' the stack handling incorrectly.

Verification: source-inspection — Read applyRelabelType at src/backend/nodes/nodeFuncs.c:644-650 which confirms the right-associative invariant and discards stacked inner relabels; the patch's reconstruction preserves only the outermost type/typmod/resultcollid/format/location which is semantically correct.

Repair instruction:

• Files: src/backend/utils/adt/selfuncs.c, src/test/regress/sql/collate.icu.utf8.sql, src/test/regress/expected/collate.icu.utf8.out
• Locations: strip_relabel_for_stats helper header comment (after refactor)
• Exact edits: Add a documenting header comment to strip_relabel_for_stats() stating the right-associative RelabelType invariant (only the outermost resultcollid is semantically visible), referencing applyRelabelType() in nodeFuncs.c and the foo::int::oid example. No behavior change.
• Postcondition: The invariant that only the outermost RelabelType's resultcollid is preserved is documented, preventing future incorrect modifications.

Suggested code example (provided, illustrative):

/*
 * strip_relabel_for_stats
 * RelabelType composition is right-associative: only the outermost
 * resultcollid is semantically visible, and applyRelabelType() in
 * nodeFuncs.c relies on the same invariant, discarding all but the
 * topmost RelabelType when flattening stacked relabels (e.g. foo::int::oid).
 */

Consolidated repair patch: HEAD-bound unified diff (applies_to_head=45817676632b46fc9d2b97c8d432348e0ee4f1f4, sha256=3307d0c72e09a30b3df9bf1a2465c01859c7638644117c0318b1c8cc11eacb83).

Repair validation: passed via methods: apply-check, diff-check, compile, static-check, targeted-test.

Evidence: Comment-only change (part of the consolidated repair patch); compile clean; static check confirms the comment documents the invariant without behavior change.

Test example (vehicle: inspection, status: provided):

The header comment of strip_relabel_for_stats documents the right-associative invariant.

Oracle: Comment present; no behavior change; existing tests unaffected.

Baseline: Pre-fix: no invariant comment.

Patched: Post-fix: invariant documented in the helper header comment.

M3 — MINOR (tests) — verified

Location: src/test/regress/sql/collate.icu.utf8.sql:587

Mechanism: The added tests only exercise the extended-statistics path (CREATE STATISTICS ON (a COLLATE case_insensitive)). The first hunk's normalization also affects basenode used in the expressional-index match loop at line 5828, but no test creates an index on a COLLATE expression and checks estimates, leaving the H2 defect (and the index path generally) untested.

Failure: Test gap: the expressional-index path of examine_variable is untested; the H2 defect would be a latent regression invisible to the suite.

Verification: test-coverage-inspection — Inspection of the added test block (collate.icu.utf8.sql:585-596) shows only CREATE STATISTICS; no CREATE INDEX on a COLLATE expression. The H2 repro confirmed the index path is reachable and affected.

Repair instruction:

• Files: src/test/regress/sql/collate.icu.utf8.sql, src/test/regress/expected/collate.icu.utf8.out, src/backend/utils/adt/selfuncs.c
• Locations: after the existing extended-stats check_estimated_rows block (~line 596)
• Exact edits: Add an expressional-index test block: CREATE INDEX test_stats_ext_coll_idx ON test_stats_ext_coll ((lower(a) COLLATE case_insensitive)); ANALYZE; three check_estimated_rows calls for matching/default/C collations; DROP INDEX; DROP STATISTICS. Update the expected .out with the deterministic estimates (19/2/2).
• Postcondition: The expressional-index stats path is covered by a regression test that fails if the index path reverts to the old single-level strip.

Suggested code example (provided, illustrative):

CREATE INDEX test_stats_ext_coll_idx ON test_stats_ext_coll ((lower(a) COLLATE case_insensitive));
ANALYZE test_stats_ext_coll;
SELECT * FROM check_estimated_rows ($$ SELECT * FROM test_stats_ext_coll WHERE (lower(a) COLLATE case_insensitive) = 'a' $$);
SELECT * FROM check_estimated_rows ($$ SELECT * FROM test_stats_ext_coll WHERE lower(a) = 'a' $$);
SELECT * FROM check_estimated_rows ($$ SELECT * FROM test_stats_ext_coll WHERE (lower(a) COLLATE "C") = 'a' $$);

Consolidated repair patch: HEAD-bound unified diff (applies_to_head=45817676632b46fc9d2b97c8d432348e0ee4f1f4, sha256=3307d0c72e09a30b3df9bf1a2465c01859c7638644117c0318b1c8cc11eacb83).

Repair validation: passed via methods: apply-check, diff-check, targeted-test, compile, static-check.

Evidence: New index-path test block produced zero diffs vs the updated expected output (estimates 19/2/2 deterministically). The test fails on the pre-fix tree (matching case estimated 25, default case ~19).

Test example (vehicle: regress, status: provided):

CREATE INDEX test_stats_ext_coll_idx ON test_stats_ext_coll ((lower(a) COLLATE case_insensitive));
ANALYZE test_stats_ext_coll;
SELECT * FROM check_estimated_rows ($$ SELECT * FROM test_stats_ext_coll WHERE (lower(a) COLLATE case_insensitive) = 'a' $$);
SELECT * FROM check_estimated_rows ($$ SELECT * FROM test_stats_ext_coll WHERE lower(a) = 'a' $$);
SELECT * FROM check_estimated_rows ($$ SELECT * FROM test_stats_ext_coll WHERE (lower(a) COLLATE "C") = 'a' $$);

Oracle: estimates 19, 2, 2 (matching collation uses index stats; mismatches fall back to default selectivity).

Baseline: Pre-fix: 25, ~19, 25 (matching collation misses stats; default collation falsely consumes case_insensitive index stats).

Patched: Post-fix: 19, 2, 2 (matching collation uses stats; mismatches fall back).

N4 — NIT (tests) — verified

Location: src/test/regress/sql/collate.icu.utf8.sql:587

Mechanism: test_stats_ext_coll is created without WITH (autovacuum_enabled = off), violating the convention documented at src/test/regress/sql/stats_ext.sql:4-6 (tables for which estimated row counts are checked should disable autovacuum to avoid unstable results from auto-analyze).

Failure: Potential test instability: a concurrent autovacuum could re-analyze between the explicit ANALYZE and the check_estimated_rows calls, changing estimates in slow CI.

Verification: convention-comparison — src/test/regress/sql/stats_ext.sql:4-6 documents the convention; the patch's table omits the reloption.

Repair instruction:

• Files: src/test/regress/sql/collate.icu.utf8.sql, src/test/regress/expected/collate.icu.utf8.out, src/backend/utils/adt/selfuncs.c
• Locations: CREATE TABLE test_stats_ext_coll (~line 587)
• Exact edits: Add WITH (autovacuum_enabled = off) to the CREATE TABLE statement; update the expected .out for the changed CREATE TABLE output.
• Postcondition: test_stats_ext_coll disables autovacuum consistent with stats_ext.sql convention; estimates remain stable.

Suggested code example (provided, illustrative):

CREATE TABLE test_stats_ext_coll (a text, b text, c int) WITH (autovacuum_enabled = off);

Consolidated repair patch: HEAD-bound unified diff (applies_to_head=45817676632b46fc9d2b97c8d432348e0ee4f1f4, sha256=3307d0c72e09a30b3df9bf1a2465c01859c7638644117c0318b1c8cc11eacb83).

Repair validation: passed via methods: apply-check, diff-check, targeted-test, static-check, compile.

Evidence: Test still produces stable estimates (52/52/32/52 and 19/2/2) with the reloption added; static check confirms the reloption matches the stats_ext.sql convention.

Test example (vehicle: inspection, status: provided):

CREATE TABLE test_stats_ext_coll (a text, b text, c int) WITH (autovacuum_enabled = off);

Oracle: Table created with autovacuum_enabled=off; estimates stable across runs.

Baseline: Pre-fix: no autovacuum reloption.

Patched: Post-fix: WITH (autovacuum_enabled = off) added.

N2 — NIT (maintainability) — verified

Location: src/backend/utils/adt/selfuncs.c:5693

Mechanism: Comments at lines 5693 and 5968 use a non-ASCII apostrophe (U+2019 RIGHT SINGLE QUOTATION MARK) in 'expression's' instead of ASCII '. PG style (and pgindent/pgperlcritic) prefer ASCII apostrophes in C comments.

Failure: Style: non-ASCII character in C comment; inconsistent with tree conventions.

Verification: grep — grep for U+2019 in selfuncs.c found two occurrences at lines 5693 and 5968, both inside the duplicated normalization comments.

Repair instruction:

• Files: src/backend/utils/adt/selfuncs.c, src/test/regress/sql/collate.icu.utf8.sql, src/test/regress/expected/collate.icu.utf8.out
• Locations: lines 5693 and 5968 (the duplicated normalization comments)
• Exact edits: Remove both non-ASCII apostrophes. With the M1 refactor the duplicated comments are deleted and the replacement strip_relabel_for_stats() comment uses only ASCII.
• Postcondition: No U+2019 characters remain in selfuncs.c.

Suggested code example (provided, illustrative):

/* The refactored helper comment uses only ASCII apostrophes. */

Consolidated repair patch: HEAD-bound unified diff (applies_to_head=45817676632b46fc9d2b97c8d432348e0ee4f1f4, sha256=3307d0c72e09a30b3df9bf1a2465c01859c7638644117c0318b1c8cc11eacb83).

Repair validation: passed via methods: apply-check, diff-check, compile, static-check, targeted-test.

Evidence: After applying the consolidated repair patch, grep for U+2019 in selfuncs.c returns zero matches (the duplicated comments containing the curly apostrophes are deleted; the helper comment uses ASCII); compile clean.

Test example (vehicle: inspection, status: provided):

grep -P '\xe2\x80\x99' src/backend/utils/adt/selfuncs.c returns no matches after the refactor.

Oracle: Zero non-ASCII apostrophes in selfuncs.c.

Baseline: Pre-fix: 2 occurrences of U+2019 at lines 5693 and 5968.

Patched: Post-fix: 0 occurrences.

N3 — NIT (tests) — refuted

Location: src/test/regress/sql/collate.icu.utf8.sql:19

Mechanism: Claim: check_estimated_rows is defined/dropped mid-file inconsistently with stats_ext.sql.

Failure: Claimed inconsistency.

Verification: source-inspection — check_estimated_rows is defined at collate.icu.utf8.sql:19, immediately after CREATE SCHEMA/SET search_path (top of file, consistent with stats_ext.sql:10). It is dropped immediately after its only use. The 'mid-file' characterisation is inaccurate; the only genuine convention gap (autovacuum reloption) is covered by N4.

Repair: refuted — not-applicable-refuted envelope; no repair.

Verification Summary

• Verified: 6 (H2, M1, H1, M3, N4, N2)
• Refuted: 1 (N3 — check_estimated_rows placement is consistent with stats_ext.sql; the only real convention gap is covered by N4)
• Cannot-verify: 0
• Security: CLEAN — no PG-specific security findings; the change tightens (does not loosen) extended-stats/index matching and the whole-table SELECT ACL gate is unchanged.

Repair Validation Summary

All six verified findings are addressed by a single consolidated HEAD-bound unified diff (repair.patch, sha256 3307d0c72e09a30b3df9bf1a2465c01859c7638644117c0318b1c8cc11eacb83), validated in a disposable scratch cassert+debug+coverage build:

• apply-check: patch -p1 --dry-run reports all three files checked, no errors.
• apply: patch -p1 patches all three files cleanly.
• diff-check: applied files match the intended repaired files (diff -q all MATCH).
• compile: make -C src/backend/utils/adt selfuncs.o clean; make -C src/backend postgres linked clean (no warnings).
• repro (H2): temp ICU cluster; three expressional-index queries produced corrected estimates (matching-collation 192 uses stats, mismatches 25 fall back) where the pre-repair tree showed a false miss (25) and false match (192).
• regression (M3): new index-path test block produced zero diffs vs updated expected output (estimates 19/2/2); the test fails on the pre-fix tree.
• Pre-existing note: the only remaining collate.icu.utf8 diff is a pre-existing eager_agg plan-shape mismatch (HashAggregate→GroupAggregate at line ~3675) present in the staged tree before any repair, unrelated to this patch.

pgsql-hackers thread ledger

• Schema: pg-review-thread-ledger/v1
• Status: reconciled
• Thread: examine_variable ignored CollateExpr (thread-id 633598)
• Participants: jian.universality@gmail.com, 44973863@qq.com, tgl@sss.pgh.pa.us
• Reconstruction: Located the pgsql-hackers thread 'examine_variable ignored CollateExpr' (thread-id 633598) via BM25 search and retrieved all 4 messages by Message-ID. Reconstructed the chronology: jian he's original patch (2026-01-13) raising the bug and suggesting moving check_estimated_rows to a shared setup file; ZizhuanLiu's first proposal (2026-08-16) to re-run eval_const_expressions; Tom Lane's rejection (2026-08-16) of that approach as 'ridiculously expensive'; ZizhuanLiu's revision (2026-08-17) implementing an in-place normalization function, which is the lineage of the staged patch. The staged patch correctly avoids eval_const_expressions per Tom Lane's steer.

Message chronology

Finding dispositions

The staged patch correctly avoids eval_const_expressions (Tom Lane rejected that as 'ridiculously expensive') and uses an in-place normalization, consistent with the thread's agreed direction. H2 (index-path asymmetry) is a new finding not raised on-list.

Changed-Code Coverage

• Coverage requirement: required — changed instrumentable paths: src/backend/utils/adt/selfuncs.c
• Counter epoch: fresh (all .gcda deleted before the run; counters accumulated only during this test pass)
• Execution lane: host-disposable — Disposable scratch build (cassert+debug+--coverage) of the staged tree; binaries installed to a scratch prefix; temp ICU cluster initdb'd; tests run via psql and pg_regress; gcov/gcovr collected from selfuncs.gcda/.gcno. Fresh epoch: all .gcda deleted before the run; counters accumulated only during this test pass.

Test commands

make -C src/backend (cassert+debug+--coverage, --with-icu)
make install DESTDIR=<scratch-prefix>
initdb --no-sync -U regress <scratch-pgdata>
pg_ctl -o '-p 5966' start
psql -h /tmp -p 5966 -U regress -d regress -f sql/collate.icu.utf8.sql
psql -h /tmp -p 5966 -U regress -d regress -f sql/stats_ext.sql
psql -h /tmp -p 5966 -U regress -d regress -f sql/selectivity.sql
psql -h /tmp -p 5966 -U regress -d regress -f sql/create_index.sql
psql -h /tmp -p 5966 -U regress -d regress -f sql/indexes.sql
psql -h /tmp -p 5966 -U regress -d regress -f <targeted collation/cast/index test>
gcov -b src/backend/utils/adt/selfuncs.gcno
gcovr --gcov-executable gcov --filter selfuncs.c --txt

Metrics (changed lines/branches in examine_variable)

Line equation: covered(29) + compiler-uncovered(1) = changed executable(30) ✓

Branch equation: changed(16) − covered(13) = uncovered(3) ✓

Uncovered regions (actionable)

src/backend/utils/adt/selfuncs.c:5963 — examine_variable — infeasible-with-reason

5963: 							expr = (Node *) rexpr;

Reason: This branch (the collation-equal true-branch in the extended-statistics expr normalization) is reached only when a stored extended-statistics expression is a RelabelType whose deepest underlying expression's collation equals the relabel's resultcollid. Extended-statistics expressions are run through eval_const_expressions at src/backend/statistics/extended_stats.c:541 before being stored, and applyRelabelType (src/backend/nodes/nodeFuncs.c:644-650) collapses no-op RelabelTypes (where the underlying collation equals resultcollid) during that pass. Consequently a stored extended-stats expression that is a RelabelType with matching collation cannot arise from normal planner operation; the branch is defensive/dead. Verified by constructing multiple extended-statistics expressions on collation-preserving casts (a::varchar, a::char(10), a::varchar(10)) that would exercise this branch; none produced a stored RelabelType with matching collation (eval_const_expressions collapsed them first).

Waiver: kind=not-instrumentable — The branch is infeasible through SQL-reachable planner operation. extended_stats.c:541 runs eval_const_expressions on stored expressions, and applyRelabelType collapses no-op relabels, so a stored RelabelType with matching underlying collation cannot occur.

Risk: No runtime risk: the branch is unreachable through normal SQL because eval_const_expressions removes no-op RelabelTypes before they reach the extended-stats storage. A stored extended-stats expression that is a RelabelType with matching collation would require bypassing eval_const_expressions or directly constructing the catalog row.

Uncovered branches

src/backend/utils/adt/selfuncs.c:5685 branch 1 (count 0) — examine_variable — infeasible-with-reason

5685: 		while (expr && IsA(expr, RelabelType))

Reason: Branch 1 of the while loop condition is the short-circuit when expr is NULL on the first evaluation (the expr && clause). This requires a RelabelType whose arg is NULL, which cannot arise from the parser (makeRelabelType always sets arg to a non-NULL Expr). Defensive guard for malformed input.

Waiver: kind=not-instrumentable — The NULL-arg short-circuit guards against malformed RelabelType nodes that cannot be constructed via SQL.

src/backend/utils/adt/selfuncs.c:5959 branch 1 (count 0) — examine_variable — infeasible-with-reason

5959: 					while (rexpr && IsA(rexpr, RelabelType))

Reason: Branch 1 of the while loop condition is the short-circuit when rexpr is NULL on the first evaluation. Same as line 5685: requires a stored extended-stats RelabelType with arg==NULL, which cannot arise because eval_const_expressions (extended_stats.c:541) and the parser always produce non-NULL args.

Waiver: kind=not-instrumentable — The NULL-arg short-circuit guards against malformed nodes unreachable through SQL.

src/backend/utils/adt/selfuncs.c:5962 branch 1 (count 0) — examine_variable — infeasible-with-reason

5962: 					if (exprCollation((Node *) rexpr) == relabel->resultcollid)

Reason: Branch 1 (the true/collation-equal branch) leads to the uncovered line 5963. As documented for the uncovered region, a stored extended-stats RelabelType whose underlying collation equals resultcollid cannot arise because eval_const_expressions (extended_stats.c:541) collapses no-op RelabelTypes via applyRelabelType before storage.

Waiver: kind=not-instrumentable — applyRelabelType collapses no-op relabels during eval_const_expressions, so a stored RelabelType always carries a meaningful collation change (hitting the else branch).

Touched functions

• examine_variable (src/backend/utils/adt/selfuncs.c:5652-6041): entered=True, whole-function lines 85/124

Non-instrumented changes

• src/test/regress/expected/collate.icu.utf8.out — Test expected-output file; not C/C++ instrumentable implementation.
• src/test/regress/sql/collate.icu.utf8.sql — Test SQL file; not C/C++ instrumentable implementation.

Aggregated Test Plan

1. Existing src/test/regress/sql/collate.icu.utf8.sql extended-stats block (GROUP BY a COLLATE C/POSIX/case_insensitive/default) — covers region-1 query-side and region-2 extended-stats else-branch.
2. Proposed (M3) expressional-index block: CREATE INDEX ON ((lower(a) COLLATE case_insensitive)); three check_estimated_rows calls for matching/default/C collations — covers the index path (H2 fix).
3. Targeted ad-hoc collation/cast/index queries run during coverage (extended-stats on a::varchar, a::char(10), a::varchar(10)) — exercised region-1/region-2 paths; the collation-equal branch (line 5963) confirmed infeasible.

Provenance / model routes

Residual risk

• H2 is a latent estimation-quality regression for expressional indexes with COLLATE; without the repair, matching-collation queries miss index stats and mismatched-collation queries consume wrong stats. Should be raised on-list.
• The uncovered line 5963 and 3 uncovered branches are all infeasible defensive guards (NULL-arg short-circuits and the eval_const_expressions-collapsed collation-equal branch); no runtime risk.
• The pre-existing collate.icu.utf8 eager_agg plan-shape diff is unrelated to this patch and environment-specific.

Validator

The v2 artifact at review-artifact-v2.json passed validate_review_artifact.py (sha256 edbcb304ffbd062627196c1ee0ce3d9b60cae80dd7b201e420335fea0a31979e) with ok: pg-review-artifact-v2 is structurally complete.