Re: Function scan FDW pushdown - Mailing list pgsql-hackers

Alexander Korotkov писал(а) 2026-07-31 00:19:
> On Tue, Jul 7, 2026 at 12:25 PM Alexander Pyhalov
> <a.pyhalov@postgrespro.ru> wrote:
>> 
>> Alexander Korotkov писал(а) 2026-07-06 17:11:
>> > Hi, Alexander!
>> >
>> > On Wed, May 20, 2026 at 1:17 PM Alexander Pyhalov
>> > <a.pyhalov@postgrespro.ru> 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.
>> 
>> Well, at least current patch version classifies conditions correctly.
>> 
>> > 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.
>> 
>> With funcid we could print actual function name in explain.
>> Now we print, for example,
>> 
>> Relations: ((public.distr1_part_3 d1_3) INNER JOIN 
>> (public.distr2_part_3
>> d2_3)) INNER JOIN (Function n)
>> 
>> If funcid is preserved, we could output actual function name instead 
>> of
>> alias. This is more similar to
>> how we show relation names. Attaching updated patch, which preserves
>> function names in explain.
>> 
>> After looking at it once more, I'm not convinced when we can get
>> not-FuncExpr in rtfunc->funcexpr.
>> So added more checks to function_rte_pushdown_ok() to avoid dealing 
>> with
>> such case.
> 
> I agree on your changes.  I've put some additional.
> 
> 1) Extracted initialization of stub fpinfo for function RTE into
> init_func_stub_fpinfo().  The stub now inherits fdw_startup_cost,
> fdw_tuple_cost, use_remote_estimate, fetch_size and async_capable from
> the foreign side, in addition to server and shippable_extensions.
> Without this, merge_fdw_options() and the cost estimator would pick up
> zeroed values of fdw_startup_cost and fdw_tuple_cost when the function
> RTEis on the outer side of the join, so the pushdown path appears to
> be artificially cheap.
> 2) Added a comment on the whole-row Var CASE WHEN wrapper in
> deparseColumnRef() explaining that it is a no-op under the current
> INNER-only policy but kept to stay correct for future outer-join and
> null-supply scenarios.  Please, check if you agree with this.
> 3) Documented the new pushdown case in postgres-fdw.sgml, listing the
> conditions under which a foreign-table x set-returning-function INNER
> join is absorbed into the remote query.
> 4) Added a regression test verifying that a STABLE (not IMMUTABLE)
> set- returning function is rejected for pushdown, alongside the
> existing VOLATILE-rejection test.
> 


Hi. I've updated comment a bit, as we currently have to handle NULL 
records correctly,
this is not a nice-to-have feature which we need for future changes to 
be more safe.
-- 
Best regards,
Alexander Pyhalov,
Postgres Professional