Re: postgres_fdw: push down FETCH FIRST .. WITH TIES when server version allows - Mailing list pgsql-hackers

On Tue, Sep 22, 2026 at 9:10 PM Sagar Shedge <sagar.shedge92@gmail.com> wrote:

On Tue, Sep 22, 2026 at 6:54 PM Jeevan Chalke <jeevan.chalke@enterprisedb.com> wrote:

On Mon, Sep 14, 2026 at 7:12 AM Jinqing Kuang <kuangjinqingcn@gmail.com> wrote:

On Sep 11, 2026, at 22:36, Jeevan Chalke <jeevan.chalke@enterprisedb.com> wrote:
>
> On Fri, Sep 11, 2026 at 9:17 AM Sagar Shedge <sagar.shedge92@gmail.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.chalke@enterprisedb.com> wrote:
> Hello,
>
> On Thu, Sep 10, 2026 at 7:05 AM Jinqing Kuang <kuangjinqingcn@gmail.com> wrote:
>
> > On Sep 6, 2026, at 10:39, Sagar Shedge <sagar.shedge92@gmail.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

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