On Tue, Sep 22, 2026 at 9:10 PM Sagar Shedge <[email protected]> wrote:
> > > 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 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 <https://www.enterprisedb.com>
