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/
