On Wed, Sep 23, 2026 at 1:24 PM Sagar Shedge <[email protected]> wrote:
> > > On Wed, Sep 23, 2026 at 11:20 AM Jeevan Chalke < > [email protected]> wrote: > >> >> >> 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> >> > > Hi Jeevan, > > Here is commitfest entry - https://commitfest.postgresql.org/patch/7269/ > Thanks, Sagar. I have added myself as a reviewer. The patch looks good to me now. Feel free to mark the status as "Ready for Committer," or you can wait to see if anyone else has follow-up reviews. Thanks -- *Jeevan Chalke* *Senior Principal Engineer, Engineering Manager* *Product Development* enterprisedb.com <https://www.enterprisedb.com>
