schema: review-report-v2
target: postgres_fdw: push down FETCH FIRST .. WITH TIES when server version allows (CommitFest 7269)
head: 4000ab1decb6138a11fb4423237589376cb9ff54
base: 4000ab1decb6138a11fb4423237589376cb9ff54
diff_identity: diff:80c811bed6fd700cfd81b34ff5e04c11f0123219
verdict: ready-for-committer
blocker_counts:
  verified: 0
  unverified: 0

CommitFest 7269 Review: postgres_fdw WITH TIES Pushdown

Summary

Verdict: ready-for-committer

The patch enables pushdown of FETCH FIRST .. WITH TIES to remote PostgreSQL servers in postgres_fdw when a cached connection already exists for the session and reports version >= 13. It adds GetCachedConnectionVersion() which consults the connection cache without performing network I/O, modifies add_foreign_final_paths() to check the cached version before allowing pushdown, and updates appendLimitClause() to emit the SQL-standard FETCH FIRST ... ROWS WITH TIES syntax with parenthesized values to work around the c_expr grammar restriction.

The approach is safe, well-designed, and addresses a known limitation documented in commit 2b461efc519 (which disabled WITH TIES pushdown due to bug #18467). The design avoids plan-time network I/O by only using already-cached connection versions.

Findings

Blocker counts: 0 verified, 0 unverified.

Verification Summary

• 7 findings verified, 1 cannot-verify (F5: theoretical race condition), 0 refuted.
• All verified findings have complete repair packets with concrete patches bound to HEAD, validated in disposable scratch builds.
• The finding verifier ran dynamic repros for F3 (confirmed plan-time ERROR with original patch, correct local Limit with fix) and verified all repair patches compile and pass the full postgres_fdw regression test suite (2 regression tests + 1 isolation test).
• Security review found no vulnerabilities: connection cache lookup is correctly scoped by user mapping OID, user identity matches execution-time logic, no SQL injection vectors, no plan-time network I/O.

Repair Validation Summary

All validated patches were built and tested in a disposable scratch build with --enable-debug --enable-cassert --without-readline --without-zlib --without-icu and coverage instrumentation. The postgres_fdw regression tests (postgres_fdw, query_cancel) and isolation test (eval_plan_qual) all passed.

Changed-Code Coverage

Coverage requirement: required (3 changed instrumentable implementation files)

Measurement: fresh disposable counter epoch, host-disposable lane, PostgreSQL 20devel, gcov.

Totals

Line coverage: 28/31 = 90.3% Branch coverage: 13/18 = 72.2%

Test commands

make -C contrib/postgres_fdw installcheck PGPORT=55432 PGHOST=/tmp
pg_isolation_regress --load-extension=postgres_fdw --dbname=isolation_regression eval_plan_qual

Uncovered regions (actionable)

1. connection.c:1052 (GetCachedConnectionVersion) — return 0 when ConnectionHash == NULL. Defensive guard; requires fresh session with no prior connections. Classification: requires-special-harness-or-injection-point. TAP test proposed.

1. postgres_fdw.c:8539 (add_foreign_final_paths) — return when !OidIsValid(final_rel->serverid). Cross-server WITH TIES early return not reached by existing tests. Classification: coverable-now. Regression test with partitioned table proposed (COV-2).

1. postgres_fdw.c:8541 (add_foreign_final_paths) — pushdown_userid = final_rel->userid (ternary true branch). Security definer view userid branch never taken. Classification: coverable-now. Regression test with security definer view proposed (COV-3).

Uncovered branches

• connection.c:1051 branch 0 (ConnectionHash == NULL): requires-special-harness-or-injection-point
• connection.c:1057 branch 3 (entry->conn == NULL): requires-special-harness-or-injection-point
• postgres_fdw.c:8538 branch 0 (!OidIsValid(serverid)): coverable-now
• postgres_fdw.c:8541 branch 0 (OidIsValid(userid) true): coverable-now
• postgres_fdw.c:8545 branch version_lt_130000: infeasible-with-reason (covered via cold-cache path; real <v13 server unavailable in test environment)

Non-instrumented changes

• contrib/postgres_fdw/postgres_fdw.h — header file, extern declaration only
• contrib/postgres_fdw/expected/postgres_fdw.out — test expected output
• contrib/postgres_fdw/sql/postgres_fdw.sql — test SQL file

pgsql-hackers Thread Ledger

Status: no-thread-found

The patch submission thread (Message-ID: CAPhYifHu_Nd+YoAg0iWfCCO+6eGo5nzbQNyOm=uxXTcvKatccw@mail.gmail.com, 2026-09-06) has only the original patch post by Sagar Shedge with no review replies. The referenced bug #18467 discussion is a separate prior thread. No pgsql-hackers review discussion exists for this patch.

All findings are disposition new (not previously raised in the thread).

Security Section

No security vulnerabilities found. The patch is safe to merge from a security standpoint:

• Connection cache lookup is correctly scoped by user mapping OID, preventing cross-user information leaks.
• User identity for version check matches execution-time logic.
• No new network I/O at plan time.
• No SQL injection vectors (deparse uses same safe expression-deparsing infrastructure).
• The invalidated flag is correctly checked, ensuring stale connections aren't trusted.

Aggregated Test Plan

1. Existing tests: WITH TIES pushdown (warm cache), WITH TIES + OFFSET pushdown, cross-server join (no pushdown), UNION ALL with mixed cache states — all pass.
2. F3 repro: Missing user mapping at plan time — verify no ERROR, local Limit plan.
3. COV-2 test: Cross-server partitioned table WITH TIES — verify local Limit, no pushdown.
4. COV-3 test: Security definer view WITH TIES — verify pushdown with view owner's connection.
5. F4 deterministic test: UNION ALL with both connections warmed up — verify both sides push down.

Provenance

• pg-patch-reviewer: code review, PG memory-context filter, readiness assessment
• pg-security-reviewer: security review, connection cache security, authorization, SQL injection
• pg-finding-verifier: finding verification, repair validation, dynamic repros
• Coverage: fresh-epoch gcov measurement in host-disposable lane
• Model: glm-5.2:cloud

Residual Risk

• F5 (cannot-verify): Theoretical plan-to-execution race for connection invalidation. Worst case is a remote query error, not incorrect results. Acceptable per existing postgres_fdw design patterns.
• Coverage gaps: 3 uncovered lines (90.3% line coverage). Two are coverable with additional test cases (COV-2, COV-3); one is a defensive guard requiring a TAP test with session freshness control.