On Tue, Sep 22, 2026 at 6:54 PM Jeevan Chalke < [email protected]> wrote:
> > > On Mon, Sep 14, 2026 at 7:12 AM Jinqing Kuang <[email protected]> > wrote: > >> 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. >> > > 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-0001-postgres_fdw-Push-down-WITH-TIES-for-known-remote.patch
Description: Binary data
