Conversation
Co-authored-by: Jubilee <workingjubilee@gmail.com> Signed-off-by: Philippe Noël <21990816+philippemnoel@users.noreply.github.com>
3b1c26c to
471129f
Compare
PostgreSQL 18 introduces self-join elimination optimization that breaks ParadeDB's context-sensitive score() functions by eliminating duplicate table scans, causing score() to return the same value for both instances. This implementation adds self-join detection and score() detection to the existing planner hook (which handles window function replacement) and uses a RAII guard to temporarily disable enable_self_join_elimination when both conditions are met. Changes: - Add BoolGucGuard RAII pattern for safe GUC management - Add has_self_join() to detect same table appearing multiple times with recursive subquery support - Add has_score_function() to detect score() usage in queries - Integrate PG18 self-join protection into existing paradedb_planner_hook - Preserve all existing window function replacement functionality The guard automatically restores the original GUC value when it goes out of scope, ensuring configuration changes don't leak beyond their intended scope.
This commit fixes two issues caused by rebase conflicts: 1. Restored pgvector upgrade to v0.8.1 for system Postgres installations - Line 127 in test-pg_search.yml was reverted to v0.8.0 during rebase - pgvector v0.8.1 is required for PostgreSQL 18 compatibility 2. Added missing pgrx CI configuration for PostgreSQL 18 - Previously only had pgrx tests for PG 17 - PG 18 tests were only running with system Postgres - Now includes pgrx-managed Postgres tests for PG 18
… hooks
PostgreSQL 18 introduced automatic rewriting of OR clauses to ScalarArrayOpExpr
(ANY) during match_clause_to_indexcol. This caused crashes in pg_search's planner
hooks which expected only scalar datums.
Changes:
- Enhanced from_datum_resilient() to detect and handle array datums
- Arrays are converted to Boolean OR queries: Boolean{should: [q1, q2, ...]}
- Added coerce_scalar_array() to safely detect array vs scalar datums
- Updated planner hooks (query_input_restrict, query_input_support) to use
universal datum decoder
- Enhanced custom scan path with explicit scalar_array_use_or handling
- Maintains backward compatibility with PG14-17 scalar queries
This fix allows both user-written ANY() and planner-generated ANY() from
OR rewrites to work correctly on PostgreSQL 18.
PostgreSQL 18 added self-join elimination code that calls score_funcoids() on every query through the planner hook, even before CREATE EXTENSION runs. The old implementation used regprocedurein which panics when functions don't exist yet. This fix: - Creates try_score_funcoids() using LookupFuncName with missing_ok=true - Returns None gracefully when functions don't exist (before CREATE EXTENSION) - Follows the ParadeDB pattern from utils.rs:lookup_pdb_function() - Fixes test_logical_replication test failure on PG18 PG17 and below don't have this issue because the self-join elimination check code is wrapped in #[cfg(feature = "pg18")]. Fixes #3375
PostgreSQL 18 added self-join elimination code that calls score_funcoids() on every query through the planner hook, even before CREATE EXTENSION runs. The old implementation used regprocedurein which panics when functions don't exist yet. This fix: - Creates try_score_funcoids() using LookupFuncName with missing_ok=true - Returns None gracefully when functions don't exist (before CREATE EXTENSION) - Follows the ParadeDB pattern from utils.rs:lookup_pdb_function() - Fixes test_logical_replication test failure on PG18 PG17 and below don't have this issue because the self-join elimination check code is wrapped in #[cfg(feature = "pg18")]. Fixes #3375
mdashti
left a comment
There was a problem hiding this comment.
@workingjubilee Thanks for the comments.
|
Thanks for the weekend merge ❤️ |
Thank you for your patience on this issue! :) |
| // Check if this is a SearchQueryInput array (ScalarArrayOpExpr case). | ||
| // Without this check, FromDatum would panic with "cache lookup failed" on non-array datums. | ||
| let array = datum.cast_mut_ptr::<pg_sys::ArrayType>(); | ||
| let is_sqi_array = (*array).ndim >= 1 | ||
| && (*array).ndim <= pg_sys::MAXDIM as i32 | ||
| && (*array).elemtype == searchqueryinput_typoid(); |
There was a problem hiding this comment.
Aha, that makes sense.
| if is_array { | ||
| // ScalarArrayOpExpr: decode as array of SearchQueryInput | ||
| let should = unsafe { | ||
| <Vec<SearchQueryInput> as FromDatum>::from_polymorphic_datum( | ||
| key.sk_argument, | ||
| false, | ||
| searchqueryinput_typoid(), | ||
| ) | ||
| .expect("SearchQueryInput array should not be NULL") | ||
| }; |
There was a problem hiding this comment.
That also works, yep.
| } | ||
|
|
||
| impl SearchQueryInput { | ||
| pub unsafe fn from_datum(datum: pg_sys::Datum, is_null: bool) -> Option<SearchQueryInput> { |
There was a problem hiding this comment.
This has one interesting little quirk in that this function can now shadow certain calls that might seem like they would hit FromDatum::from_datum, even if that trait is in scope: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=dec11478802dbf79879c77e69bc9de64
This can be desirable, so I am not saying this to warn you off it, just noting that. It's especially probably-what-you-want as this function does exactly what you want it to when from_datum is called.
| } | ||
| } | ||
|
|
||
| // Detoast if needed (PostgreSQL memory context handles cleanup) |
There was a problem hiding this comment.
Also valid, as long as this isn't called a ton of times in a single transaction. But if it does turn out that's the case, you can fix this later easily.
# Ticket(s) Closed - Closes #2723 ## What Add support for Postgres 18 to `pg_search`. I don't yet default to PG18 for our builds, though. That can come later. We'll also want to add PG18 to the prebuilt CI binaries. This can come in a separate PR I also upgraded pgvector to 0.8.1, which is necessary for PG18 support. ## Why Lots of community users are waiting for this, and I had an hour to spare. ## How Mostly reading Postgres source and compare REL_17_STABLE and REL_18_STABLE. ## Tests I can compile and run the full quickstart locally, and CI should pass for the full test suite. --------- Signed-off-by: Philippe Noël <21990816+philippemnoel@users.noreply.github.com> Co-authored-by: Stu Hood <stuhood@gmail.com> Co-authored-by: Jubilee <workingjubilee@gmail.com> Co-authored-by: Mithun Chicklore Yogendra <mithun.cy@gmail.com> Co-authored-by: Mohammad Dashti <mdashti@gmail.com>
Ticket(s) Closed
What
Add support for Postgres 18 to
pg_search. I don't yet default to PG18 for our builds, though. That can come later. We'll also want to add PG18 to the prebuilt CI binaries. This can come in a separate PRI also upgraded pgvector to 0.8.1, which is necessary for PG18 support.
Why
Lots of community users are waiting for this, and I had an hour to spare.
How
Mostly reading Postgres source and compare REL_17_STABLE and REL_18_STABLE.
Tests
I can compile and run the full quickstart locally, and CI should pass for the full test suite.