| From: | Jinqing Kuang <kuangjinqingcn(at)gmail(dot)com> |
|---|---|
| To: | Sagar Shedge <sagar(dot)shedge92(at)gmail(dot)com> |
| Cc: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: postgres_fdw: push down FETCH FIRST .. WITH TIES when server version allows |
| Date: | 2026-09-10 01:35:27 |
| Message-ID: | 5B3F035B-4025-45E5-8C38-BF1EA2F13BD8@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> On Sep 6, 2026, at 10:39, Sagar Shedge <sagar(dot)shedge92(at)gmail(dot)com> wrote:
>
> Hi Hackers,
>
> add_foreign_final_paths() currently disables pushing down FETCH FIRST
> .. WITH TIES entirely, because doing so requires knowing whether the
> remote server is v13+ (which added support for the clause), and
> checking that would mean opening a connection during planning (see
> the discussion at https://postgr.es/m/18467-7bb89084ff03a08d@postgresql.org
> which led to the current behavior).
>
> Attached patch fills in that one remaining gap. postgres_fdw already
> keeps a connection cache alive for the session's lifetime; if a
> connection to the relevant foreign server already exists in that cache
> (from an earlier query in the same session), its version is known for
> free, with no additional network access. GetCachedConnectionVersion()
> lookup into that cache and retun cached version. This information used in
> add_foreign_final_paths() to allow the pushdown only when a cached
> connection reports version 13 or later. The relation's server/user
> mapping are read from RelOptInfo's own serverid/userid fields, which
> are InvalidOid whenever the relation spans more than one foreign server
> (a cross-server join, or a sharded partitioned table). So the pushdown
> correctly stays disabled in those cases.
>
> appendLimitClause() is updated to emit the SQL-standard FETCH FIRST
> clause (with OFFSET ahead of it, per the grammar) instead of plain
> LIMIT/OFFSET when WITH TIES is in use. The value in that position is
> parsed as c_expr rather than a_expr, which does not accept the
> "::type" cast decoration deparseExpr() normally emits for constants;
> the patch parenthesizes it, which c_expr explicitly allows.
>
> Regarding the collation/tie-semantics concern raised in the original
> thread: by the time add_foreign_final_paths() runs, ORDER BY has
> already been determined safe to push down by an earlier check. Ties are
> just rows that compare equal under that same, already-vetted comparison.
> So no new risk is introduced by additionallyfetching the tied rows.
>
> Tested against a loopback foreign server, including: 1/ cold-cache
> sessions correctly falling back to local evaluation; 2/ warm-cache
> sessions pushing the FETCH clause down with results matching the
> non-FDW reference, both with and without OFFSET 3/ cross-server
> joins/unions correctly never attempting the pushdown. New regression
> tests added to postgres_fdw.sql/expected covering all of the above.
> make check passes.
>
> Regards,
> Sagar Shedge
> Multigres Engineer, Supabase
>
> <0001-postgres_fdw-fetch-first-with-ties.patch>
Hi Sagar,
I found two regressions in the patch.
With use_remote_estimate=true, this fails during planning:
SELECT a, count(*) FROM ft
WHERE b = 1 GROUP BY a, b
ORDER BY b FETCH FIRST 2 ROWS WITH TIES;
The planner removes b from the sort keys because WHERE fixes its value.
The remote query then has WITH TIES without ORDER BY:
ERROR: WITH TIES cannot be specified without ORDER BY clause
ORDER BY (1+1) has the same problem on grouped queries. I’ve kept
WITH TIES local when pathkeys is empty.
Ordinary EXPLAIN also fails with local estimates when the server has
neither a user-specific nor a PUBLIC mapping:
CREATE SERVER no_mapping FOREIGN DATA WRAPPER postgres_fdw;
CREATE FOREIGN TABLE ft_no_mapping (a int) SERVER no_mapping;
EXPLAIN (VERBOSE, COST OFF)
SELECT a FROM ft_no_mapping ORDER BY a
FETCH FIRST 2 ROWS WITH TIES;
GetUserMapping() errors before the cache lookup can fall back. I used
GetUserMappingExtended(..., DEBUG1) so a missing mapping keeps the limit
local. Existing mapping checks for remote estimates and execution still
apply.
I’ve attached v2 with fixes for both cases on top of your original patch,
along with regression tests.
Regards,
Jinqing
| Attachment | Content-Type | Size |
|---|---|---|
| v2-0001-postgres_fdw-fetch-first-with-ties.patch | application/octet-stream | 19.6 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Richard Guo | 2026-09-10 01:36:46 | Re: Assert failure in try_nestloop_path() |
| Previous Message | Zhijie Hou (Fujitsu) | 2026-09-10 01:30:00 | RE: Follow-up review items for update_deleted |