fix(splinter): flip pinner to opt-in, add inlinability regression net#200
Open
fix(splinter): flip pinner to opt-in, add inlinability regression net#200
Conversation
- Drop dead NULL-handling branches from compare_ope_cllw_* (STRICT
short-circuits NULL inputs before the body runs).
- Fix ciphertext-length wording in OPE type docs ("8 bytes per plaintext
byte", not "per plaintext bit", which contradicted the 65-byte result).
- Convert simple OPE helpers (extractors, has_*, order_by_ope) from
LANGUAGE plpgsql to LANGUAGE sql so the planner can inline them on the
sort/order_by hot path. Per project guideline.
- Spell the cipher consistently as CLLW (matching ope_cllw_* type names
and the existing ore_cllw_* docs); CLWW was a typo.
- Clarify sql-support.md MIN/MAX and COUNT(DISTINCT) rows so ope is no
longer implied to be a ste_vec-extracted node term — it only applies
to the outer column.
- Loosen brittle exact-list assertion in config_check_rejects_unknown_index
to mention 'ope' and the offending token.
- Add ore_wins_over_opf_when_both_present to lock in the
ORE-before-OPE dispatch precedence in eql_v2.compare.
- Add operator-level coverage for <=, >=, =, <> on both opf and opv
variants (previously only < and > were covered for opf, and opv was
exercised through compare_* but not via operators).
Adds 18 OPE tests covering behaviors that were exercised for ORE but had no OPE equivalent. The ORE suite duplicates each test across three ciphertext variants (block / cllw_u64_8 / cllw_var_8); for OPE we only need one representative per variant per behavior because both opf and opv use the same primitive (bytea lex compare). NULL term in payload (mirrors compare_hmac_with_null_ore_index): - has_opf_false_when_field_is_json_null - has_opv_false_when_field_is_json_null - compare_dispatches_through_null_opf_to_hmac - compare_dispatches_through_null_opv_to_hmac NULL operands at the comparator (codifies STRICT short-circuit): - compare_ope_cllw_u64_65_strict_returns_null_for_null_operand - compare_ope_cllw_var_8_strict_returns_null_for_null_operand ORDER BY NULLS FIRST/LAST × ASC/DESC on opf-encoded data: - order_by_asc_nulls_first_with_opf - order_by_asc_nulls_last_with_opf - order_by_desc_nulls_first_with_opf - order_by_desc_nulls_last_with_opf sort_compare with mixed NULL + opf rows: - sort_compare_asc_puts_nulls_first_with_opf - sort_compare_desc_puts_nulls_last_with_opf MIN / MAX aggregates over OPE-encoded values: - eql_v2_min_with_opf_finds_minimum - eql_v2_max_with_opf_finds_maximum - eql_v2_min_with_opf_null_only_returns_null - eql_v2_max_with_opf_null_only_returns_null BETWEEN range filtering: - between_with_opf_inclusive_bounds - between_with_opv_inclusive_bounds All 47 OPE tests pass; full suite (~430 tests) is green.
The earlier sql-support clarification dropped `ope` from the outer-column index list for COUNT(DISTINCT col). OPE ciphertexts are deterministic within a column key, so an outer-column `ope` index supports DISTINCT semantics just like `unique` and `ore`. Restoring it for parity with the MIN/MAX row.
Match the bidirectional NULL coverage of the u64_65 sibling test. Asserting only compare(NULL, x) leaves the right-NULL STRICT path unverified; both directions must short-circuit.
Two doc-only fixes flagged in PR #197 review: 1. compare.sql @note for both u64_65 and var_8: the term-NULL branches (a_term IS NULL / b_term IS NULL) are NOT redundant with STRICT. STRICT only short-circuits on NULL function inputs (a, b); the term branches handle a non-NULL eql_v2_encrypted whose payload simply lacks the opf/opv field. Mirrors the defensive pattern in compare_ore_block_u64_8_256. 2. functions.sql @param tags for ope_cllw_u64_65 / ope_cllw_var_8 / has_ope_cllw_u64_65 / has_ope_cllw_var_8 now include the parameter name "val" before the type, matching valid Doxygen syntax (and compare.sql which already names a/b correctly). No code behavior change; mise run docs:validate stays clean.
Closes #199. The previous pinner (introduced in #190) was opt-out: it ALTERed every eql_v2.* function at install time to add `SET search_path`, except for an explicit deny-list of "GIN-critical" functions. PostgreSQL refuses to inline any SQL function with `proconfig` set (see `inline_function` in src/backend/optimizer/util/clauses.c), so the pinner silently broke inlining for any newly-authored SQL function the deny-list missed. This bit #196 (operator inlining) and #197 (OPE helpers) the same way. Two fixes: **1. Pinner is now opt-in (`tasks/pin_search_path.sql`).** Authors explicitly tag a function with `@noinline` in `COMMENT ON FUNCTION ...` to request pinning. Default is to leave functions alone. Failure mode flips: forgetting to tag → splinter `function_search_path_mutable` warning (loud, fixable). Forgetting under the old model → silent perf regression. The 4 plpgsql functions that previously relied on pinning (`compare_ope_cllw_u64_65/var_8`, `ope_cllw_u64_65/var_8(jsonb)`) now declare `SET search_path = pg_catalog, extensions, public` inline, matching the convention used by ~143 other plpgsql functions across the codebase. **2. EXPLAIN-based regression net (`tests/sqlx/tests/inlinability_regression_tests.rs`).** Six tests run EXPLAIN against representative queries and assert the relevant functional index is in the plan: hmac_256-hash and bloom_filter-GIN expressions, plus operator-form variants for `=` / `<>` / `~~` / `~~*`. The four operator-form tests are `#[ignore]`'d pending PR #196 which flips those to inlinable SQL; the two direct-expression tests pass now and protect against pinner overreach independently of the splinter allowlist. Splinter allowlist (`tasks/test/splinter.sh`) grows from 7 to 30 entries to cover all the inlinable wrappers that the previous opt-out pinner was silently pinning. Splinter still passes: 47 raw findings, all 47 allowlisted, 0 unallowlisted. Verified locally: full sqlx test suite passes 403/0/50 against a fresh install on PG17.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on top of #197. Closes #199.
Problem
The pinner introduced in #190 (
tasks/pin_search_path.sql) is opt-out: it ALTERs every `eql_v2.*` function at install time to add `SET search_path`, except for a hand-maintained deny-list of "GIN-critical" functions. PostgreSQL refuses to inline any SQL function whose `pg_proc.proconfig` is non-NULL (see `inline_function` in `src/backend/optimizer/util/clauses.c`), so the pinner silently disabled inlining for any newly-authored SQL function the deny-list missed.This already happened twice in the last week:
The principle established in those reviews is performance wins over splinter compliance: when the two conflict on the same `proconfig` column, we keep the function inlinable and add an allowlist entry, never the other way round.
Fix
1. Opt-in pinner (issue #199, suggestion 1)
Authors explicitly tag a function with `@noinline` in `COMMENT ON FUNCTION ...` to request pinning. The pinner only ALTERs marked functions; default is to leave functions alone. Failure mode flips:
The 4 plpgsql functions that were previously relying on the pinner now declare `SET search_path = pg_catalog, extensions, public` inline, matching the convention already used by ~143 other plpgsql functions in the codebase:
The pinner's old `inline_critical_oids` query is gone; its replacement is a single `coalesce(d.description, '') LIKE '%@noinline%'` predicate against `pg_description`. Future authors with a corner case that wants pinning can use the marker without learning the allowlist machinery.
2. EXPLAIN regression net (issue #199, suggestion 4)
New `tests/sqlx/tests/inlinability_regression_tests.rs` runs EXPLAIN against representative queries that should hit functional indexes and asserts the index is in the plan. Failure means inlining broke — for any reason, not just pinner overreach. Six tests:
The four operator-form tests are `#[ignore]`'d with explicit messages pointing at #196. Un-ignore once that lands.
3. Splinter allowlist expansion
Allowlist grows from 7 to 30 entries to cover all the inlinable wrappers the old opt-out pinner was silently pinning. Each entry has a one-line justification referencing the index it needs to match. Splinter still passes: 47 raw findings, all 47 allowlisted, 0 unallowlisted.
The new entries fall into three groups:
Test plan
Future work captured by #199
The "definition of done" on #199 is met by this PR. If we ever decide to also move EQL toward being a real `CREATE EXTENSION` package (#198), the splinter allowlist becomes unnecessary entirely — splinter has a built-in carve-out for functions owned by extensions.