Hi, Alexander! On Wed, May 20, 2026 at 1:17 PM Alexander Pyhalov <[email protected]> wrote: > I've found another issue. The fact that in the new versions of the patch > RTE RelOptInfo misses fdw_private seems to be unfortunate. For example, > in the last version we haven't thought about classifying > baserestrictinfo. And if we do, we should pass fdw_private down to > foreign_expr_walker. Perhaps, we could attach it to RTE_FUNCTION rel > prior to calling classifyConditions(), but should we later set it back > to NULL? Another problem comes if we try to handle joins, which can > crearte subqueries (like INNER/OUTER UNIQUE). In this case we should > somehow cook fpinfo for get_relation_column_alias_ids(). Attaching patch > which tries to handle baserestrictinfos by passing fpinfo down to > foreign_expr_walker().
I think passing fpinfo down to foreign_expr_walker() is the right approach, thank you. Attaching it to RTE_FUNCTION rel, and reseting it back to NULL would create excessive state and potential source of bugs. The only advantage is saving existing function signatures, but it doesn't worth it. Also, I don't think we need to care bout get_relation_column_alias_ids(). As we currently pushdown function only for inner joins, make_outerrel_subquery/make_innerrel_subquery are always false, lower_subquery_rels also wouldn't contain RTE_FUNCTION relid. Therefore: * In deparseFromExprForRel() for functional rel deparseRangeTblRef() is always called with make_subquery=false. * is_subquery_var() returns false. > One more interesting example (included in the patch) is > > EXPLAIN (VERBOSE, COSTS OFF) > WITH s AS MATERIALIZED (SELECT r1.* FROM remote_tbl r1 > JOIN LATERAL > (SELECT r2.a FROM remote_tbl r2, f(r1.a) LIMIT 1) s > ON true) > SELECT * FROM s ORDER BY 1; > > We get the following plan: > > Sort > Output: s.a, s.b > Sort Key: s.a > CTE s > -> Nested Loop > Output: r1.a, r1.b > -> Foreign Scan on public.remote_tbl r1 > Output: r1.a, r1.b > Remote SQL: SELECT a, b FROM public.base_tbl_fn > -> Foreign Scan > Output: NULL::integer > Relations: (public.remote_tbl r2) INNER JOIN (Function > f) > Remote SQL: SELECT NULL FROM (public.base_tbl_fn r1 > INNER JOIN public.f($1::integer) f2(c1) ON (TRUE)) LIMIT 1::bigint > -> CTE Scan on s > Output: s.a, s.b > > Here you can see that we use parameter in function argument. Don't know > if it's a real problem, but at least looks suspicious. In v3 patch used > contain_param_walker() in is_nonrel_relinfo_ok() (which mutated to > function_rte_pushdown_ok()) to avoid such plans. AFAICS, there is no correctness problem. * The bms_is_empty(rel->lateral_relids) check rejects a lateral reference to a relation at the same query level. * In the example, f(r1.a) references r1 from an *outer* level. When the subquery is planned, r1.a becomes a PARAM_EXEC, and the function rel's lateral_relids inside the subquery is empty -- so the check passes and funcexpr retains a Param. * foreign_expr_walker deliberately allows PARAM_EXEC (deparse.c:486–505), and postgres_fdw's normal machinery (params_list => fdw_exprs) re-sends the param on each rescan. So there's no need to bring back v3's contain_param_walker() — this is actually a useful capability (a parameterized foreign scan). I only add a comment in function_rte_pushdown_ok() noting that outer-level Params are allowed intentionally and handled as an ordinary parameterized foreign scan. > One minor issue I've noticed is in function_rte_pushdown_ok(): > + if (rel->rtekind != RTE_FUNCTION) > + return false; > + rte = planner_rt_fetch(rel->relid, root); > + if (rte->rtekind != RTE_FUNCTION) > + return false; > > Is the second rtekind check necessary? The second check is not necessary, changed to an assert. Other changes I made: 1. In foreign_join_ok(), the function side's baserestrictinfo was classified against the joinrel fpinfo, whose server/shippable_extensions aren't set until merge_fdw_options() runs later — so extension-shippable quals would be misclassified as local and silently kill the pushdown. Now it: (a) copies shippable_extensions from the foreign side onto the stub, and (b) classifies against the stub (fpinfo_i/fpinfo_o, which already has server). Built-in quals like n > 3 were unaffected; this fixes the extension case. I've added the test case for this. 2. Dropped unused funcid. The per-RTE metadata was a 3-element list (funcid, funcrettype, funccollation) but funcid was never read. Reduced to list_make2(funcrettype, funccollation); updated the consumer (linitial/lsecond) and both descriptive comments. 3. Changed redundant elog(ERROR) into Assert in get_functions_data() and get_tupdesc_for_join_scan_tuples(). The return-type validity is already guaranteed by function_rte_pushdown_ok(), so these are pure cross-checks. 4. Style consistency in find_em_for_rel_target(): introduced a named fpinfo local instead of passing rel->fdw_private inline, matching find_em_for_rel(). 5. Also added test for nullable-side whole-row Var, but with outer rows that both match and miss the (foreign x function) inner join. ------ Regards, Alexander Korotkov Supabase
v10-0001-postgres_fdw-push-down-FUNCTION-RTE-into-foreign.patch
Description: Binary data
