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

On Mon, Sep 14, 2026 at 7:12 AM Jinqing Kuang <kuangjinqingcn(at)gmail(dot)com>
wrote:

> On Sep 11, 2026, at 22:36, Jeevan Chalke <jeevan(dot)chalke(at)enterprisedb(dot)com>
> wrote:
> >
> > On Fri, Sep 11, 2026 at 9:17 AM Sagar Shedge <sagar(dot)shedge92(at)gmail(dot)com>
> wrote:
> > 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.
> >
> > Thanks for the changes. Looking deeper into the code, I noticed this:
> >
> > + /*
> > + * final_rel->serverid is set only if the whole relation
> belongs to a
> > + * single FDW (see grouping_planner()); this is InvalidOid
> for, e.g.,
> > + * a join or partitioned scan spanning more than one foreign
> server,
> > + * in which case there's no single remote query to push the
> FETCH
> > + * clause into.
> > + */
> > + if (!OidIsValid(final_rel->serverid))
> > + return;
> >
> > This check also guards a case beyond what the comment describes: a
> > partitioned/inherited relation whose partitions are all on the same
> foreign
> > server. There, final_rel->serverid is still InvalidOid (it's a
> multi-relation
> > Merge Append, not a single foreign relation), so this correctly forces
> > WITH TIES to stay local. That matters because pushing FETCH FIRST ...
> WITH TIES
> > independently into each partition's own scan would be an actual
> correctness bug.
> > Ties have to be evaluated against the globally merged ordering across all
> > partitions, not per-partition. This if already prevents that, but the
> comment
> > currently frames the check only in terms of "no single remote query to
> push
> > into," not the correctness hazard it happens to also rule out.
> >
> > Worth calling that out explicitly in the comment, and adding a
> regression test
> > for the same-server multi-partition case, so it's clear this isn't just
> a
> > missing-optimization corner but a case that would silently return wrong
> results
> > if this check were ever relaxed or bypassed.
> >
> > Rest all looks good to me.
> >
> > Thanks
> >
> >
> >
> > 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
> >
> >
> > --
> > Sagar Dilip Shedge,
> > Pune.
> > With Regards.
> >
> >
> > --
> > Jeevan Chalke
> > Senior Principal Engineer, Engineering Manager
> > Product Development
> >
> > enterprisedb.com
>
> Thanks for taking another look. I’ve added tests for the same-server
> partition case, covering ties across partitions and OFFSET into the
> tied group, with the connection already cached.
>
> While looking into this case, I noticed that the partitioned parent has
> no fdwroutine, so grouping_planner() doesn’t call GetForeignUpperPaths()
> for it. This means the global Limit stays local without reaching the
> server-id check. I’ve clarified that in the comment.
>
> Attached is v4 based on Sagar’s v3.
>

Thanks for the patch. The new comment above the serverid check is a good
improvement. It explains that a partitioned parent has no FDW routine at
all,
so grouping_planner() never calls us for it, even when all partitions are
on
the same server. This makes the reason clear. I am fine with this.

But there is one issue with the new with_ties test. It will not catch the
bug
if someone later weakens/removes the serverid check and allows pushdown for
each partition separately. I checked this by running the same
FETCH FIRST 2 ROWS WITH TIES on each partition's base table one by one, and
then combining the results by hand. with_ties_1 alone gives 1,2,2, and
with_ties_2 alone gives 2,2. When combined: *1,2,2,2,2* — same as what the
test
expects as the correct output. This happens because both partitions' own
tie
boundary lands on the same value (2) as the actual global boundary. So even
a
wrong, per-partition implementation would give the same result here, and
the
test would still pass.

If we use different data, the test can actually catch this bug. For
example,
with p1 = 1,2,2,2 and p2 = 1,3,3,3, the correct global answer is *1,1*
(only 2
rows, I checked this against the patch). But if each partition pushes the
FETCH FIRST WITH TIES on its own, we would wrongly get all 8 rows. So I
suggest
changing the test data to something like this — one partition with mostly
one
repeated value, other partition with mostly a different repeated value, and
just one row of the boundary value in each. This way the test will actually
fail if this check is ever broken, not just pass by chance.

This is not a bug in the code, just a suggestion to make the test stronger.
Rest all looks good to me.

Thanks

>
> Regards,
> Jinqing
>
>

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

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