From 92aa97c31db6028fe3e3fe79db511af3b8158e9c Mon Sep 17 00:00:00 2001 From: Sagar Shedge Date: Sun, 6 Sep 2026 07:09:55 +0530 Subject: [PATCH] postgres_fdw: Push down FETCH FIRST .. WITH TIES when server version allows FETCH FIRST .. WITH TIES was never pushed down, because doing so requires knowing whether the remote server is v13 or later (which added support for the clause), and checking that would mean opening a connection during planning. However, postgres_fdw already keeps a connection cache alive for the lifetime of the session. 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. Add GetCachedConnectionVersion(), a non-creating lookup into the connection cache, and use it in add_foreign_final_paths() to allow the pushdown when a cached connection reports version 13 or later. The relation's server and user mapping are read from RelOptInfo's own serverid/userid fields (set by grouping_planner() and propagated through every upper-relation stage), which are InvalidOid whenever the relation spans more than one foreign server (like a cross-server join or a sharded partitioned table), so the pushdown continues to be skipped, since the tie boundary for WITH TIES can only be determined after combining candidate rows from every contributing server. appendLimitClause() emits the SQL-standard FETCH FIRST clause (with OFFSET ahead of it, per the grammar) instead of plain LIMIT/OFFSET when the query uses WITH TIES. deparseExpr() normally emits ::type cast decoration for constants. The value in this position is parsed as c_expr, not a full a_expr, and c_expr does not accept that cast. Added parentheses around it, which c_expr explicitly allows via a parenthesized a_expr. Related: https://postgr.es/m/18467-7bb89084ff03a08d@postgresql.org --- contrib/postgres_fdw/connection.c | 30 +++++++ contrib/postgres_fdw/deparse.c | 39 +++++++-- .../postgres_fdw/expected/postgres_fdw.out | 85 +++++++++++++++++-- contrib/postgres_fdw/postgres_fdw.c | 32 ++++++- contrib/postgres_fdw/postgres_fdw.h | 1 + contrib/postgres_fdw/sql/postgres_fdw.sql | 32 ++++++- 6 files changed, 201 insertions(+), 18 deletions(-) diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c index b5d4cf3dccc..6552f708da2 100644 --- a/contrib/postgres_fdw/connection.c +++ b/contrib/postgres_fdw/connection.c @@ -1030,6 +1030,36 @@ ReleaseConnection(PGconn *conn) */ } +/* + * Return the server version number of the already-cached connection for + * "user", if one exists, or 0 if there is none (in which case the caller + * must not assume anything about the remote server's version). + * + * This never establishes a new connection and never does any network I/O: + * it only consults the connection cache and, if a live entry is found, + * reads the version number libpq already recorded during that connection's + * startup handshake. This makes it safe to call from planning code, which + * must not have the side effect of opening remote connections. + */ +int +GetCachedConnectionVersion(UserMapping *user) +{ + bool found; + ConnCacheKey key; + ConnCacheEntry *entry; + + if (ConnectionHash == NULL) + return 0; + + key = user->umid; + entry = (ConnCacheEntry *) hash_search(ConnectionHash, &key, HASH_FIND, + &found); + if (!found || entry->conn == NULL || entry->invalidated) + return 0; + + return PQserverVersion(entry->conn); +} + /* * Assign a "unique" number for a cursor. * diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index 673b678826c..aa9ab79a45d 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -4229,15 +4229,44 @@ appendLimitClause(deparse_expr_cxt *context) /* Make sure any constants in the exprs are printed portably */ nestlevel = set_transmission_modes(); - if (root->parse->limitCount) + if (root->parse->limitOption == LIMIT_OPTION_WITH_TIES) { - appendStringInfoString(buf, " LIMIT "); + /* + * Plain LIMIT has no way to express WITH TIES, so use the + * SQL-standard FETCH clause instead. Per the grammar, OFFSET (if + * any) must precede FETCH. + * + * Unlike LIMIT/OFFSET, the value in this position is restricted to + * "c_expr" rather than a full "a_expr" (see select_fetch_first_value + * in gram.y), which notably disallows the "::type" cast decoration + * deparseExpr() adds to constants for portability. Parenthesize + * the value to work around that; c_expr explicitly allows a + * parenthesized a_expr, so this is valid regardless of what kind of + * expression it turns out to be. + */ + if (root->parse->limitOffset) + { + appendStringInfoString(buf, " OFFSET ("); + deparseExpr((Expr *) root->parse->limitOffset, context); + appendStringInfoString(buf, ") ROWS"); + } + Assert(root->parse->limitCount); + appendStringInfoString(buf, " FETCH FIRST ("); deparseExpr((Expr *) root->parse->limitCount, context); + appendStringInfoString(buf, ") ROWS WITH TIES"); } - if (root->parse->limitOffset) + else { - appendStringInfoString(buf, " OFFSET "); - deparseExpr((Expr *) root->parse->limitOffset, context); + if (root->parse->limitCount) + { + appendStringInfoString(buf, " LIMIT "); + deparseExpr((Expr *) root->parse->limitCount, context); + } + if (root->parse->limitOffset) + { + appendStringInfoString(buf, " OFFSET "); + deparseExpr((Expr *) root->parse->limitOffset, context); + } } reset_transmission_modes(nestlevel); diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 517d15cf1fa..4b86e2723aa 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -1087,17 +1087,17 @@ SELECT * FROM ft1 t1 WHERE t1.c1 === t1.c2 order by t1.c2 limit 1; 1 | 1 | 00001 | Fri Jan 02 00:00:00 1970 PST | Fri Jan 02 00:00:00 1970 | 1 | 1 | foo (1 row) --- Ensure we don't ship FETCH FIRST .. WITH TIES +-- Ensure we ship FETCH FIRST .. WITH TIES once the remote server's version +-- is known (i.e., a connection to it is already cached in this session, as +-- is the case here due to preceding tests) EXPLAIN (VERBOSE, COSTS OFF) SELECT t1.c2 FROM ft1 t1 WHERE t1.c1 > 960 ORDER BY t1.c2 FETCH FIRST 2 ROWS WITH TIES; - QUERY PLAN -------------------------------------------------------------------------------------------------- - Limit + QUERY PLAN +---------------------------------------------------------------------------------------------------------------------------------- + Foreign Scan on public.ft1 t1 Output: c2 - -> Foreign Scan on public.ft1 t1 - Output: c2 - Remote SQL: SELECT c2 FROM "S 1"."T 1" WHERE (("C 1" > 960)) ORDER BY c2 ASC NULLS LAST -(5 rows) + Remote SQL: SELECT c2 FROM "S 1"."T 1" WHERE (("C 1" > 960)) ORDER BY c2 ASC NULLS LAST FETCH FIRST (2::bigint) ROWS WITH TIES +(3 rows) SELECT t1.c2 FROM ft1 t1 WHERE t1.c1 > 960 ORDER BY t1.c2 FETCH FIRST 2 ROWS WITH TIES; c2 @@ -1108,6 +1108,75 @@ SELECT t1.c2 FROM ft1 t1 WHERE t1.c1 > 960 ORDER BY t1.c2 FETCH FIRST 2 ROWS WIT 0 (4 rows) +-- Same, but combined with OFFSET; OFFSET must be emitted ahead of FETCH FIRST +-- per the grammar, and skipping into the middle of a tied group must not +-- drop any of the remaining ties +EXPLAIN (VERBOSE, COSTS OFF) +SELECT t1.c2 FROM ft1 t1 WHERE t1.c1 > 960 ORDER BY t1.c2 OFFSET 1 FETCH FIRST 2 ROWS WITH TIES; + QUERY PLAN +---------------------------------------------------------------------------------------------------------------------------------------------------------- + Foreign Scan on public.ft1 t1 + Output: c2 + Remote SQL: SELECT c2 FROM "S 1"."T 1" WHERE (("C 1" > 960)) ORDER BY c2 ASC NULLS LAST OFFSET (1::bigint) ROWS FETCH FIRST (2::bigint) ROWS WITH TIES +(3 rows) + +SELECT t1.c2 FROM ft1 t1 WHERE t1.c1 > 960 ORDER BY t1.c2 OFFSET 1 FETCH FIRST 2 ROWS WITH TIES; + c2 +---- + 0 + 0 + 0 +(3 rows) + +-- Ensure we never ship FETCH FIRST .. WITH TIES for a query whose result +-- combines rows from more than one foreign server (here, a join between +-- ft5 on "loopback" and ft6 on "loopback2"), regardless of whether either +-- server's version is known; there's no single remote query to push the +-- FETCH clause into, so it must stay local +EXPLAIN (VERBOSE, COSTS OFF) +SELECT ft5.c1, ft5.c2 FROM ft5 JOIN ft6 USING (c1) + ORDER BY ft5.c2 FETCH FIRST 2 ROWS WITH TIES; + QUERY PLAN +------------------------------------------------------------------------------------- + Limit + Output: ft5.c1, ft5.c2 + -> Nested Loop + Output: ft5.c1, ft5.c2 + Join Filter: (ft5.c1 = ft6.c1) + -> Foreign Scan on public.ft5 + Output: ft5.c1, ft5.c2, ft5.c3 + Remote SQL: SELECT c1, c2 FROM "S 1"."T 4" ORDER BY c2 ASC NULLS LAST + -> Materialize + Output: ft6.c1 + -> Foreign Scan on public.ft6 + Output: ft6.c1 + Remote SQL: SELECT c1 FROM "S 1"."T 4" +(13 rows) + +-- Two independently limited scans on different foreign servers, combined +-- locally via UNION ALL: each side's FETCH FIRST .. WITH TIES pushdown +-- decision is made independently based on its own server's cached +-- connection, with no coordination needed between them. ft5's server +-- (loopback) is already warmed up by many earlier tests, so that side +-- pushes the FETCH clause down; ft6's server (loopback2) has not been +-- connected to yet, so that side falls back to a local Limit. +EXPLAIN (VERBOSE, COSTS OFF) +(SELECT c1, c2 FROM ft6 ORDER BY c2 FETCH FIRST 2 ROWS WITH TIES) +UNION ALL +(SELECT c1, c2 FROM ft5 ORDER BY c2 FETCH FIRST 2 ROWS WITH TIES); + QUERY PLAN +---------------------------------------------------------------------------------------------------------------------- + Append + -> Limit + Output: ft6.c1, ft6.c2 + -> Foreign Scan on public.ft6 + Output: ft6.c1, ft6.c2 + Remote SQL: SELECT c1, c2 FROM "S 1"."T 4" ORDER BY c2 ASC NULLS LAST + -> Foreign Scan on public.ft5 + Output: ft5.c1, ft5.c2 + Remote SQL: SELECT c1, c2 FROM "S 1"."T 4" ORDER BY c2 ASC NULLS LAST FETCH FIRST (2::bigint) ROWS WITH TIES +(9 rows) + -- Test CASE pushdown EXPLAIN (VERBOSE, COSTS OFF) SELECT c1,c2,c3 FROM ft2 WHERE CASE WHEN c1 > 990 THEN c1 END < 1000 ORDER BY c1; diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index 9269418a074..ebc71fff077 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -8500,12 +8500,36 @@ add_foreign_final_paths(PlannerInfo *root, RelOptInfo *input_rel, * determined to be safe to push down before we get here. So in that case * the FETCH clause is safe to push down with ORDER BY if the remote * server is v13 or later, but if not, the remote query will fail entirely - * for lack of support for it. Since we do not currently have a way to do - * a remote-version check (without accessing the remote server), disable - * pushing the FETCH clause for now. + * for lack of support for it. We have no way to check the remote + * server's version without connecting to it, and plan-time code must not + * have the side effect of opening a new connection; but if a connection + * to this server already exists in the connection cache (from an earlier + * query in this session), its version is known for free. Push the FETCH + * clause down only in that case; otherwise play it safe and disable the + * pushdown, as before. */ if (parse->limitOption == LIMIT_OPTION_WITH_TIES) - return; + { + Oid pushdown_userid; + UserMapping *user; + + /* + * 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; + + pushdown_userid = OidIsValid(final_rel->userid) ? + final_rel->userid : GetUserId(); + user = GetUserMapping(pushdown_userid, final_rel->serverid); + + if (GetCachedConnectionVersion(user) < 130000) + return; + } /* * Also, the LIMIT/OFFSET cannot be pushed down, if their expressions are diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h index da7da1c2ea9..c2d03266607 100644 --- a/contrib/postgres_fdw/postgres_fdw.h +++ b/contrib/postgres_fdw/postgres_fdw.h @@ -170,6 +170,7 @@ extern void process_pending_request(AsyncRequest *areq); extern PGconn *GetConnection(UserMapping *user, bool will_prep_stmt, PgFdwConnState **state); extern void ReleaseConnection(PGconn *conn); +extern int GetCachedConnectionVersion(UserMapping *user); extern unsigned int GetCursorNumber(PGconn *conn); extern unsigned int GetPrepStmtNumber(PGconn *conn); extern void do_sql_command(PGconn *conn, const char *sql); diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index ec766e2b28a..dd369799ddc 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -442,11 +442,41 @@ EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE t1.c1 === t1.c2 order by t1.c2 limit 1; SELECT * FROM ft1 t1 WHERE t1.c1 === t1.c2 order by t1.c2 limit 1; --- Ensure we don't ship FETCH FIRST .. WITH TIES +-- Ensure we ship FETCH FIRST .. WITH TIES once the remote server's version +-- is known (i.e., a connection to it is already cached in this session, as +-- is the case here due to preceding tests) EXPLAIN (VERBOSE, COSTS OFF) SELECT t1.c2 FROM ft1 t1 WHERE t1.c1 > 960 ORDER BY t1.c2 FETCH FIRST 2 ROWS WITH TIES; SELECT t1.c2 FROM ft1 t1 WHERE t1.c1 > 960 ORDER BY t1.c2 FETCH FIRST 2 ROWS WITH TIES; +-- Same, but combined with OFFSET; OFFSET must be emitted ahead of FETCH FIRST +-- per the grammar, and skipping into the middle of a tied group must not +-- drop any of the remaining ties +EXPLAIN (VERBOSE, COSTS OFF) +SELECT t1.c2 FROM ft1 t1 WHERE t1.c1 > 960 ORDER BY t1.c2 OFFSET 1 FETCH FIRST 2 ROWS WITH TIES; +SELECT t1.c2 FROM ft1 t1 WHERE t1.c1 > 960 ORDER BY t1.c2 OFFSET 1 FETCH FIRST 2 ROWS WITH TIES; + +-- Ensure we never ship FETCH FIRST .. WITH TIES for a query whose result +-- combines rows from more than one foreign server (here, a join between +-- ft5 on "loopback" and ft6 on "loopback2"), regardless of whether either +-- server's version is known; there's no single remote query to push the +-- FETCH clause into, so it must stay local +EXPLAIN (VERBOSE, COSTS OFF) +SELECT ft5.c1, ft5.c2 FROM ft5 JOIN ft6 USING (c1) + ORDER BY ft5.c2 FETCH FIRST 2 ROWS WITH TIES; + +-- Two independently limited scans on different foreign servers, combined +-- locally via UNION ALL: each side's FETCH FIRST .. WITH TIES pushdown +-- decision is made independently based on its own server's cached +-- connection, with no coordination needed between them. ft5's server +-- (loopback) is already warmed up by many earlier tests, so that side +-- pushes the FETCH clause down; ft6's server (loopback2) has not been +-- connected to yet, so that side falls back to a local Limit. +EXPLAIN (VERBOSE, COSTS OFF) +(SELECT c1, c2 FROM ft6 ORDER BY c2 FETCH FIRST 2 ROWS WITH TIES) +UNION ALL +(SELECT c1, c2 FROM ft5 ORDER BY c2 FETCH FIRST 2 ROWS WITH TIES); + -- Test CASE pushdown EXPLAIN (VERBOSE, COSTS OFF) SELECT c1,c2,c3 FROM ft2 WHERE CASE WHEN c1 > 990 THEN c1 END < 1000 ORDER BY c1; -- 2.50.1 (Apple Git-155)