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

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>