Re: postgres_fdw: push down FETCH FIRST .. WITH TIES when server version allows

Hello,

On Thu, Sep 10, 2026 at 7:05 AM Jinqing Kuang <kuangjinqingcn(at)gmail(dot)com>
wrote:

>
> > 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.
>

I gave the patch a quick review. It applies cleanly, builds, and make check
in
contrib/postgres_fdw passes, including the new tests. The logic looks
correct
to me, and I couldn't find a case where the pushdown produces different
results
than the local fallback.

One thing worth discussing explicitly rather than leaving implicit is that
the
pushdown decision in add_foreign_final_paths() depends entirely on whatever
connection happens to already be cached for that user mapping at plan time:

if (user == NULL || GetCachedConnectionVersion(user) < 130000)
return;

That means the exact same query, planned twice in the same backend, can end
up
with two different plans purely because of unrelated activity in between:

- First time a given foreign server is touched in a session (no cached
connection yet) => WITH TIES stays local, *no pushdown*, the full result
set
for the ORDER BY gets fetched.
- Any later query against that server in the same backend, once anything has
opened a connection to it => *pushed down*.

So EXPLAIN on the same statement can show a Foreign Scan with FETCH FIRST
...
WITH TIES folded into the remote SQL on one run, and a local LIMIT node on
another, with nothing about the query itself having changed. Someone
diagnosing
a slow query by comparing EXPLAIN output across sessions could easily
mistake
this for a bug.

To be clear, I don't think this makes the patch wrong, but since it
introduces
a new source of connection-history-dependent plan shape in postgres_fdw, I
think it's worth either:

- a note in the code comment above the check (right now the comment explains
why we use the cache, but not that this makes the pushdown decision
session-history-dependent), and/or
- a line in the commit message/release notes calling it out explicitly, so
it
doesn't surprise someone debugging plan differences later.

Curious whether this tradeoff was already considered and just not written
down,
or whether there's a reason it's not worth documenting.

Thanks

>
> Regards,
> Jinqing
>
>

--
*Jeevan Chalke*
*Senior Principal Engineer, Engineering Manager*
*Product Development*

enterprisedb.com <https://www.enterprisedb.com>