On Sep 11, 2026, at 22:36, Jeevan Chalke <[email protected]> wrote: > > On Fri, Sep 11, 2026 at 9:17 AM Sagar Shedge <[email protected]> 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 > <[email protected]> wrote: > Hello, > > On Thu, Sep 10, 2026 at 7:05 AM Jinqing Kuang <[email protected]> > wrote: > > > On Sep 6, 2026, at 10:39, Sagar Shedge <[email protected]> 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/[email protected] > > 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
v4-0001-postgres_fdw-fetch-first-with-ties.patch
Description: Binary data
