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

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.