Hi, Alexander! The revised patch is attached.
On Tue, May 12, 2026 at 11:09 AM Alexander Pyhalov <[email protected]> wrote: > 1) deparseColumnRef() doesn't account for whole row vars. > In queries like > > UPDATE remote_tbl r SET b=5 FROM UNNEST(array[box '((2,3),(-2,-3))']) AS > t (bx) WHERE r.a = area(t.bx) > > it fails with assert that varattno should be > 0. When we lock > non-relation RTE, we select whole row var, and we have to deparse it for > function RTE. > > You've removed check for function return type. This seems to be > dangerous. Old example > > CREATE OR REPLACE FUNCTION f_ret_record() RETURNS record AS $$ SELECT > (1,2)::record $$ language SQL IMMUTABLE; > ALTER EXTENSION postgres_fdw ADD function f_ret_record(); > EXPLAIN (VERBOSE, COSTS OFF) > SELECT s FROM remote_tbl rt, f_ret_record() AS s(a int, b int) > WHERE s.a = rt.a; > > fails with > > ERROR: a column definition list is required for functions returning > "record" function_rte_pushdown_ok() now calls get_expr_result_type() and rejects anything that isn't TYPEFUNC_SCALAR (also RECORDOID/VOIDOID), so f_ret_record() no longer reaches the remote side. deparseColumnRef() now handles varattno == 0 for RTE_FUNCTION and emits ROW(f<rti>.c1, ..., f<rti>.c<N>) from rte->eref->colnames. > 2) postgresBeginForeignScan() can step on function RTE, and doesn't know > what to do with it: > SELECT * FROM unnest(array[2,3,4]) n, remote_tbl r WHERE r.a = n; > ERROR: cache lookup failed for foreign table 0 > > So, we need to look for the first RTE_RELATION, as in older patch > version. The scanrelid == 0 branch in postgresBeginForeignScan() now scans fs_base_relids until it finds an RTE_RELATION. An explicit elog(ERROR) guards the (theoretically impossible) case where no foreign RTE is found. > 3) A lot of complexity in the old patch version was in making it > possible to find out RTE_FUNCTION attribute types after planing, as it's > necessary to correctly handle joins. In this version > get_tupdesc_for_join_scan_tuples() doesn't handle function RTEs. This > means, when we try to find out type for attribute types for joins, we'll > get errors. This can be seen in queries like > > UPDATE remote_tbl r SET b=CASE WHEN random()>=0 THEN 5 ELSE 0 END FROM > UNNEST(array[box '((2,3),(-2,-3))']) AS t (bx) WHERE r.a = area(t.bx) > RETURNING a,b; > > Now it fails on earlier stages (with "column f2.c0 does not exist"), but > if we fix it, we'll get something like > "ERROR: input of anonymous composite types is not implemented" > > Overall, function_rte_pushdown_ok() now allows more strange > constructions. Could it skip Vars from outside of joinrel->relids? Can > we safely ship function with parameters in arguments? I'm not sure. Restored the per-function metadata you had in v2/v3. FdwScanPrivateFunctions (list of (funcid, funcrettype, funccollation) indexed by RTI-offset) and FdwScanPrivateMinRTIndex are now saved in fdw_private by postgresGetForeignPlan(). get_tupdesc_for_join_scan_tuples() now has an RTE_FUNCTION branch that rebuilds the tuple descriptor from this metadata, exactly as in your patch. > 4) Why do we restrict list_length(rte->functions) to 1? The main reason > for supporting several rte->functions was to allow pushdown of UNNEST() > with several arrays, which is used by HammerDB tproc-c test. function_rte_pushdown_ok() now loops over rte->functions and applies the same checks to every member. deparseFunctionRangeTblRef() emits ROWS FROM (f1(), f2(), ...) AS f<rti>(c1, c2, ...) with the column-name list covering the concatenation of every function's columns, in the order they appear. > 5) We can support pushing down joins of foreign tables and another RTE > types, for example, VALUES. But with new specific way of handling > RTE_FUNCTIONS in core, this requires both changes to > set_foreign_rel_properties() and postgres_fdw. Not sure, if this is a > big problem, but at least is worth mentioning . I've kept the planner-side change minimal: set_foreign_rel_properties() propagates fdwroutine onto a joinrel pairing a foreign rel with an RTE_FUNCTION rel, so the standard GetForeignJoinPaths callback gets invoked. No new FDW callback was needed. This is also the reason the planner-side fpinfo of the function side lives on the joinrel (outer_func_fpinfo/inner_func_fpinfo fields added to PgFdwRelationInfo) rather than on the function RelOptInfo->fdw_private: the same RTE_FUNCTION rel can be paired independently with foreign rels from several servers. Extending the same scheme to RTE_VALUES / RTE_CTE is, I agree, worth doing late. It would be a one branch addition to set_foreign_rel_properties() plus an FDW-side shippability check analogous to function_rte_pushdown_ok(). A few extra changes to the patch: - LATERAL function RTEs are explicitly rejected in function_rte_pushdown_ok() (returns false when rel->lateral_relids is non-empty). - Outer joins (LEFT/RIGHT/FULL) and SEMI joins fall through to the existing !fpinfo_o || !fpinfo_i rejection, since inner_is_function/outer_is_function are only set for JOIN_INNER. - Regression coverage in contrib/postgres_fdw/sql/postgres_fdw.sql now reuses your examples. ------ Regards, Alexander Korotkov Supabase
v5-0001-postgres_fdw-push-down-FUNCTION-RTE-into-foreign-.patch
Description: Binary data
