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

Thanks Jinqing for handling regressions. I did one more round of testing
with
different flags and scenarios.

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

Good catch. I had considered it but hadn't written it down. While thinking
it through, Postgres already has similar behavior for custom vs. generic
plans which differ across executions where the optimizer's estimates lead
to different plans.
Thanks for pushing on that. It makes sense to highlight both in the code
comment and the commit message.

Attached updated patch.

On Thu, Sep 10, 2026 at 4:12 PM Jeevan Chalke <
jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:

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

--
Sagar Dilip Shedge,
Pune.

With Regards.