-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add eql_v2.lints() inlinability lint (#194) #195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,211 @@ | ||
| -- REQUIRE: src/schema.sql | ||
|
|
||
| --! @brief EQL lint: detect non-inlinable operator implementation functions | ||
| --! | ||
| --! Returns one row per violation found in the installed EQL surface. The | ||
| --! Postgres planner can only inline a function during index matching when: | ||
| --! | ||
| --! * `LANGUAGE sql` (plpgsql / C / etc. cannot be inlined) | ||
| --! * `IMMUTABLE` or `STABLE` volatility (VOLATILE cannot be inlined into | ||
| --! index expressions) | ||
| --! * No `SET` clauses (e.g. `SET search_path = ...`) | ||
| --! * Not `SECURITY DEFINER` | ||
| --! * Single-statement SELECT body | ||
| --! | ||
| --! @note The single-statement SELECT body condition is **not yet checked** by | ||
| --! this lint. A `LANGUAGE sql` function with a multi-statement body, a CTE, | ||
| --! or any pre-SELECT statement will pass all four implemented checks while | ||
| --! remaining non-inlinable. Implementing the check requires walking `prosrc` | ||
| --! (or `pg_get_functiondef`); tracked as a follow-up to #194. | ||
| --! | ||
| --! Operators on encrypted types (`eql_v2_encrypted`, `eql_v2.bloom_filter`, | ||
| --! `eql_v2.ore_*`, etc.) whose implementation functions fail any of these | ||
| --! rules silently fall back to seq scan when the documented functional | ||
| --! indexes (`eql_v2.hmac_256(col)`, `eql_v2.bloom_filter(col)`, | ||
| --! `eql_v2.ste_vec(col)`) are in place. This lint surfaces every such case. | ||
| --! | ||
| --! Severity: | ||
| --! `error` — fixable, blocks index matching, ship-blocking. | ||
| --! `warning` — likely-fixable, may not block matching but signals intent. | ||
| --! `info` — observational; useful for review, not a defect on its own. | ||
| --! | ||
| --! Categories: | ||
| --! `inlinability_language` — implementation function isn't `LANGUAGE sql`. | ||
| --! `inlinability_volatility` — implementation function is VOLATILE. | ||
| --! `inlinability_set_clause` — implementation function has a `SET` clause. | ||
| --! `inlinability_secdef` — implementation function is `SECURITY DEFINER`. | ||
| --! `inlinability_transitive` — implementation function is itself inlinable | ||
| --! but its body invokes a non-inlinable function | ||
| --! (depth 1; the planner can't peek through | ||
| --! that boundary). | ||
| --! | ||
| --! @example | ||
| --! ``` | ||
| --! SELECT severity, category, object_name, message | ||
| --! FROM eql_v2.lints() | ||
| --! WHERE severity = 'error' | ||
| --! ORDER BY category, object_name; | ||
| --! ``` | ||
| --! | ||
| --! @return SETOF record (severity text, category text, object_name text, message text) | ||
| CREATE OR REPLACE FUNCTION eql_v2.lints() | ||
| RETURNS TABLE ( | ||
| severity text, | ||
| category text, | ||
| object_name text, | ||
| message text | ||
| ) | ||
| LANGUAGE sql STABLE | ||
| AS $$ | ||
| WITH | ||
| -- All operators where at least one operand involves an EQL type. Limits | ||
| -- the scope of the lint to the operator surface customers actually hit | ||
| -- via SQL (`col = val`, `col LIKE '...'`, `col @> '...'` and friends). | ||
| eql_operators AS ( | ||
| SELECT | ||
| op.oid AS oprid, | ||
| op.oprname AS opname, | ||
| op.oprcode AS implfunc, | ||
| op.oprleft::regtype AS lhs, | ||
| op.oprright::regtype AS rhs, | ||
| op.oprcode::regprocedure AS impl_signature | ||
| FROM pg_operator op | ||
| WHERE EXISTS ( | ||
| SELECT 1 FROM pg_type t | ||
| WHERE t.oid IN (op.oprleft, op.oprright) | ||
| AND (t.typname LIKE 'eql_v2%' | ||
| OR t.typnamespace = 'eql_v2'::regnamespace) | ||
| ) | ||
| ), | ||
|
|
||
| -- Cross-join with each operator's implementation function metadata. | ||
| -- One row per operator; columns describe the inlinability of the impl. | ||
| op_impl AS ( | ||
| SELECT | ||
| eo.opname, | ||
| eo.lhs, | ||
| eo.rhs, | ||
| eo.impl_signature::text AS impl_signature, | ||
| lang_l.lanname AS lang, | ||
| p.provolatile AS volatility, | ||
| p.proconfig AS config, | ||
| p.prosecdef AS secdef, | ||
| p.prosrc AS body | ||
| FROM eql_operators eo | ||
| JOIN pg_proc p ON p.oid = eo.implfunc | ||
| JOIN pg_language lang_l ON lang_l.oid = p.prolang | ||
| ) | ||
|
|
||
| -- ┌─────────────────────────────────────────────────────────────────┐ | ||
| -- │ Direct inlinability checks: each row examines one operator's │ | ||
| -- │ implementation function and emits a violation if any rule is │ | ||
| -- │ broken. Multiple violations on the same function become │ | ||
| -- │ multiple rows (developers see every reason it doesn't inline). │ | ||
| -- └─────────────────────────────────────────────────────────────────┘ | ||
|
|
||
| SELECT | ||
| 'error' AS severity, | ||
| 'inlinability_language' AS category, | ||
| format('operator %s(%s, %s) -> %s', | ||
| opname, lhs, rhs, impl_signature) AS object_name, | ||
| format( | ||
| 'Operator implementation function is `LANGUAGE %s`; only `LANGUAGE sql` functions can be inlined by the planner. Bare `col %s val` queries fall back to seq scan even when a matching functional index exists.', | ||
| lang, opname) AS message | ||
| FROM op_impl | ||
| WHERE lang <> 'sql' | ||
|
|
||
| UNION ALL | ||
|
|
||
| SELECT | ||
| 'error', | ||
| 'inlinability_volatility', | ||
| format('operator %s(%s, %s) -> %s', opname, lhs, rhs, impl_signature), | ||
| format( | ||
| 'Operator implementation function is `VOLATILE`. The Postgres planner refuses to inline volatile functions into index expressions, so functional indexes never engage. Mark the function `IMMUTABLE` (or `STABLE` if it depends on session state).', | ||
| opname) | ||
| FROM op_impl | ||
| WHERE volatility = 'v' | ||
|
|
||
| UNION ALL | ||
|
|
||
| SELECT | ||
| 'error', | ||
| 'inlinability_set_clause', | ||
| format('operator %s(%s, %s) -> %s', opname, lhs, rhs, impl_signature), | ||
| format( | ||
| 'Operator implementation function has a `SET` clause (e.g. `SET search_path = ...`). Per Postgres function-inlining rules, any `SET` clause blocks inlining. Use schema-qualified identifiers in the body and remove the `SET` clause to allow the planner to inline.') | ||
| FROM op_impl | ||
| WHERE config IS NOT NULL | ||
|
|
||
| UNION ALL | ||
|
|
||
| SELECT | ||
| 'error', | ||
| 'inlinability_secdef', | ||
| format('operator %s(%s, %s) -> %s', opname, lhs, rhs, impl_signature), | ||
| 'Operator implementation function is `SECURITY DEFINER`. Such functions cannot be inlined; remove `SECURITY DEFINER` or use a non-inlinable wrapper layer.' | ||
| FROM op_impl | ||
| WHERE secdef | ||
|
|
||
| -- ┌─────────────────────────────────────────────────────────────────┐ | ||
| -- │ Transitive inlinability: an operator implementation function │ | ||
| -- │ that's itself inlinable can still fail to inline if its body │ | ||
| -- │ calls a non-inlinable function. Walk one level via pg_depend. │ | ||
| -- │ │ | ||
| -- │ Postgres records function-to-function dependencies in │ | ||
| -- │ pg_depend with deptype 'n' (normal) when one function references│ | ||
| -- │ another in its body — but only at CREATE time and only for │ | ||
| -- │ direct calls. This is good enough for v1; deeper transitive │ | ||
| -- │ analysis is a follow-up. │ | ||
| -- └─────────────────────────────────────────────────────────────────┘ | ||
|
|
||
| UNION ALL | ||
|
|
||
| SELECT | ||
| 'error', | ||
| 'inlinability_transitive', | ||
| format('operator %s(%s, %s) -> %s', oi.opname, oi.lhs, oi.rhs, | ||
| oi.impl_signature), | ||
| format( | ||
| 'Operator implementation function is inlinable but invokes non-inlinable function `%s` (lang=%s, volatility=%s%s). The chain blocks at depth 1: the planner inlines the outer call but cannot reduce the inner call into an index expression.', | ||
| called.proname, | ||
| called_lang.lanname, | ||
| CASE called.provolatile | ||
| WHEN 'i' THEN 'IMMUTABLE' | ||
| WHEN 's' THEN 'STABLE' | ||
| WHEN 'v' THEN 'VOLATILE' | ||
| END, | ||
| CASE WHEN called.proconfig IS NOT NULL | ||
| THEN ', has SET clause' | ||
| ELSE '' END) | ||
| FROM op_impl oi | ||
| -- Only worth the transitive check if the outer function is otherwise | ||
| -- inlinable — otherwise the direct lints above already report it. | ||
| JOIN pg_proc outer_p ON outer_p.oid = oi.impl_signature::regprocedure | ||
| JOIN pg_depend d | ||
| ON d.classid = 'pg_proc'::regclass | ||
| AND d.objid = outer_p.oid | ||
| AND d.refclassid = 'pg_proc'::regclass | ||
| AND d.deptype = 'n' | ||
| JOIN pg_proc called ON called.oid = d.refobjid | ||
| JOIN pg_language called_lang ON called_lang.oid = called.prolang | ||
| WHERE oi.lang = 'sql' | ||
| AND oi.volatility IN ('i', 's') | ||
| AND oi.config IS NULL | ||
| AND NOT oi.secdef | ||
| AND called.oid <> outer_p.oid | ||
| AND ( | ||
| called_lang.lanname <> 'sql' | ||
| OR called.provolatile = 'v' | ||
| OR called.proconfig IS NOT NULL | ||
| OR called.prosecdef | ||
| ) | ||
|
|
||
| ORDER BY 1, 2, 3; | ||
| $$; | ||
|
|
||
| COMMENT ON FUNCTION eql_v2.lints() IS | ||
| 'EQL lint: returns one row per non-inlinable operator implementation. ' | ||
| 'Run `SELECT * FROM eql_v2.lints() WHERE severity = ''error''` for a ' | ||
| 'CI-gateable check that all operator implementations on EQL types are ' | ||
| 'eligible for planner inlining.'; | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| //! EQL lint runtime tests | ||
| //! | ||
| //! These tests run `eql_v2.lints()` against the installed EQL surface and | ||
| //! assert on the shape of the result. | ||
| //! | ||
| //! The lint is intentionally noisy on the current state of EQL — every | ||
| //! plpgsql / VOLATILE / SET-clause-bearing operator implementation is | ||
| //! reported. The tests here validate that the lint *runs* and that its | ||
| //! schema is sensible. A separate stacked PR (#193, the Phase 1 operator | ||
| //! inlining work) reduces the violation count, and at that point a | ||
| //! tighter test asserting `count = 0` for specific operators becomes | ||
| //! appropriate. | ||
|
|
||
| use anyhow::Result; | ||
| use sqlx::PgPool; | ||
|
|
||
| #[derive(Debug, sqlx::FromRow)] | ||
| struct LintRow { | ||
| severity: String, | ||
| category: String, | ||
| object_name: String, | ||
| #[allow(dead_code)] | ||
| message: String, | ||
| } | ||
|
|
||
| async fn fetch_lints(pool: &PgPool) -> Result<Vec<LintRow>> { | ||
| let rows = sqlx::query_as::<_, LintRow>( | ||
| "SELECT severity, category, object_name, message FROM eql_v2.lints() ORDER BY category, object_name", | ||
| ) | ||
| .fetch_all(pool) | ||
| .await?; | ||
| Ok(rows) | ||
| } | ||
|
|
||
| #[sqlx::test] | ||
| async fn lint_function_exists_and_returns_rows(pool: PgPool) -> Result<()> { | ||
| let rows = fetch_lints(&pool).await?; | ||
| // The current state of EQL has a non-trivial number of inlinability | ||
| // violations on the operator surface. Confirm the lint produces output | ||
| // and the columns parse correctly. | ||
| assert!( | ||
| !rows.is_empty(), | ||
| "Expected lint to surface at least one inlinability violation \ | ||
| against the current EQL surface; got 0 rows" | ||
| ); | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[sqlx::test] | ||
| async fn lint_severity_values_are_well_known(pool: PgPool) -> Result<()> { | ||
| let rows = fetch_lints(&pool).await?; | ||
| for row in rows { | ||
| assert!( | ||
| matches!(row.severity.as_str(), "error" | "warning" | "info"), | ||
| "Unexpected severity {:?} for {} ({})", | ||
| row.severity, | ||
| row.object_name, | ||
| row.category | ||
| ); | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[sqlx::test] | ||
| async fn lint_categories_are_well_known(pool: PgPool) -> Result<()> { | ||
| let rows = fetch_lints(&pool).await?; | ||
| let allowed = [ | ||
| "inlinability_language", | ||
| "inlinability_volatility", | ||
| "inlinability_set_clause", | ||
| "inlinability_secdef", | ||
| "inlinability_transitive", | ||
| ]; | ||
| for row in rows { | ||
| assert!( | ||
| allowed.contains(&row.category.as_str()), | ||
| "Unexpected lint category {:?} for {}", | ||
| row.category, | ||
| row.object_name | ||
| ); | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Smoke test: at least one operator equality / pattern operator on | ||
| /// `eql_v2_encrypted` is reported pre-#193. Once #193 lands and reduces | ||
| /// violations on those specific operators, this test should be updated | ||
| /// or removed. | ||
| #[sqlx::test] | ||
| async fn lint_reports_eql_v2_encrypted_operators(pool: PgPool) -> Result<()> { | ||
| let rows = fetch_lints(&pool).await?; | ||
| let names: Vec<&str> = rows.iter().map(|r| r.object_name.as_str()).collect(); | ||
| assert!( | ||
| names | ||
| .iter() | ||
| .any(|n| n.starts_with("operator =(eql_v2_encrypted") | ||
| || n.starts_with("operator <>(eql_v2_encrypted") | ||
| || n.starts_with("operator ~~(eql_v2_encrypted") | ||
| || n.starts_with("operator @>(eql_v2_encrypted")), | ||
| "Expected at least one violation on a core eql_v2_encrypted \ | ||
| operator; got: {:?}", | ||
| names | ||
| ); | ||
| Ok(()) | ||
| } |
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.
Uh oh!
There was an error while loading. Please reload this page.