Pinner architecture is opt-out: silently undoes inlining choices made in src/

[coderdan]

Context

The post-install pinner (tasks/pin_search_path.sql, added in #190) is opt-out: every eql_v2.* function gets SET search_path = ... applied at install time, unless it's explicitly listed in the inline_critical_oids allowlist inside the pinner itself. PostgreSQL's planner refuses to inline any SQL function whose pg_proc.proconfig is non-NULL (see inline_function in src/backend/optimizer/util/clauses.c), so any function the pinner touches stops being inlinable.

This means the inlinability decision an author writes in src/ can be silently overwritten by the pinner. The author's source is correct; the pipeline mutates it after the fact.

Examples that have already bitten or come close:

• perf: flip eql_v2_encrypted infix operator implementations to inlinable SQL (#193) #196 (eq/neq/like operators flipped to inlinable SQL): would have shipped with zero perf benefit if the allowlist hadn't been extended manually. Caught during review.
• fix(ope): address CodeRabbit and reviewer feedback on PR #176 #197 (OPE helpers flipped to inlinable SQL): same problem, didn't catch it in author CI because the merge-commit CI passed splinter (pinning makes splinter happy) and the existing lint_phase_1_operators_are_clean test only watches the operators from perf: flip eql_v2_encrypted infix operator implementations to inlinable SQL (RFC Phase 1) #193, not OPE helpers. Flagged in review.

In both cases CI was green and the perf goal was silently undone.

Why the failure mode is bad

Property	Current model
Default behaviour	Pin everything → break inlining → ship slow code
Failure cost	Silent: same query plan shape, just orders of magnitude slower at scale
Detected by	Manual review of every inlinability-touching PR
Triggers when	Author flips a plpgsql wrapper to LANGUAGE sql for perf
Catches missed	Lint test only watches a hand-picked list of operators (lint_phase_1_operators_are_clean)

The opt-out model means the cost of forgetting is a perf regression in production. It should be the other way round — forgetting should mean a yellow warning in Supabase Security Advisor (loud, visible, easy to fix later), not a silent seq scan on customer data.

Possible directions

1. Flip the default — opt-in pinning

Authors mark "I want this pinned" rather than "I want this skipped." Concretely: an SQL comment marker at the function definition (e.g. -- @pinned) or COMMENT ON FUNCTION ... IS '@pinned', and the pinner only ALTERs functions tagged that way.

• Pro: Forgetting a tag → splinter warning (loud, fixable). Failure mode now flips compared to today.
• Pro: Author-time decision lives next to the code it affects.
• Con: Most existing functions need to be tagged in a one-time migration.

2. Pin in source, drop the pinner entirely

Each function declares its own SET search_path clause (or doesn't) at definition time. The CLAUDE.md "Function Language Choice" guidance grows a third axis: when pinning is wanted vs not.

• Pro: No build-time mutation. What you write is what runs.
• Pro: Diff for any function shows its full intended state.
• Con: More boilerplate per function. Easy to forget for plpgsql functions where pinning is desirable (this is what PR fix: pin search_path on every eql_v2 function #177 originally tried — was reverted for the inlining cases).
• Con: Splinter coverage drift is harder to spot in review than a missing allowlist line.

3. Plan-shape regression net (orthogonal to the above)

Add a CI test that runs EXPLAIN on representative queries (SELECT ... WHERE col = val with a hmac index, ORDER BY order_by_ope(col) with an OPE index, etc.) and asserts plan shape (Bitmap Index Scan / Index Scan rather than Seq Scan).

• Pro: Catches inlining regressions from any cause — pinner overreach, an author accidentally adding SET, a planner change in a future PG version.
• Pro: Self-documents which queries are perf-critical.
• Con: Doesn't fix the architecture, just the alarm system. Still need (1) or (2) for the underlying model.

Recommendation

Flip to opt-in (option 1) and add the plan-shape regression net (option 3). The combination means:

• Default failure mode is splinter warning, not seq scan.
• Even if someone introduces a new inlinability regression by some other path, EXPLAIN-based tests fail loudly in CI.

Option 2 is tempting for its simplicity, but reviewing splinter compliance one function at a time across hundreds of definitions seems strictly worse than one centralised tagged list.

Related

• fix(splinter): make EQL splinter-clean and flip the gate #190 — original pinner (opt-out by default, allowlist of 11 entries).
• perf: flip eql_v2_encrypted infix operator implementations to inlinable SQL (#193) #196 — first time someone hit this; allowlist grew from 11 to 22 entries.
• fix(ope): address CodeRabbit and reviewer feedback on PR #176 #197 — second time; would have grown to 27. Where the user-visible tension first surfaced and prompted this issue.
• Supabase security advisor will still flag EQL functions until packaged as an extension #198 — Supabase advisor downstream impact (independent of the architecture choice here).
• lint_phase_1_operators_are_clean (tooling: add an inlinability lint for EQL functions and operators #194) — narrow regression test that catches some but not all of these.

Definition of done

This issue closes when (a) the pinner default is inverted (no function is pinned without an explicit marker / list entry), and (b) at least one EXPLAIN-shape regression test is wired into the per-PR test job — at least covering the operators currently in the splinter allowlist (eq, neq, like, ilike, jsonb_contains/contained_by, @>, <@, order_by_ope).