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

On Wed, Sep 23, 2026 at 11:20 AM Jeevan Chalke <
jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:

>
>
> On Tue, Sep 22, 2026 at 9:10 PM Sagar Shedge <sagar(dot)shedge92(at)gmail(dot)com>
> wrote:
>
>>
>>
>> On Tue, Sep 22, 2026 at 6:54 PM Jeevan Chalke <
>> jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
>>
>>>
>>>
>>> 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>
>>>
>>
>> Good catch and thanks for working out the exact numbers. I've updated the
>> test data to one boundary-value row plus a distinct filler value per
>> partition. I confirmed by hand and by directly running the per-partition
>> queries
>> that a wrongly independent per-partition pushdown would now return all 8
>> rows.
>> While the correct combined result is just the two boundary ties. So the
>> test will
>> actually fail if the serverid check is ever weakened, not pass by
>> coincidence.
>>
>> I also added a test for a related but distinct case. Restriction that
>> makes only
>> one of several ORDER BY keys redundant (rather than all of them). That
>> exercises the pathkeys-non-empty path with a reduced remote sort key,
>> which wasn't covered by the existing all-keys-redundant tests.
>>
>> v5 attached, rebased on current master. Rest unchanged from v4.
>>
>
> v5 addresses all points — the with_ties test data now actually
> distinguishes
> correct global tie evaluation from wrong per-partition pushdown, and the
> code
> itself is unchanged from what I already reviewed and tested. LGTM.
>
> I didn't see any commitfest entry for this — can you point me to it, if
> there
> is one?
>
> Thanks,
>
> --
> *Jeevan Chalke*
> *Senior Principal Engineer, Engineering Manager*
> *Product Development*
>
> enterprisedb.com <https://www.enterprisedb.com>
>

Hi Jeevan,

Here is commitfest entry - https://commitfest.postgresql.org/patch/7269/