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

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.

Regards,
Jinqing