Hi,
While testing pg_plan_advice together with postgres_fdw, I noticed that the
advice generated for a single-relation foreign scan is not round-trip safe,
which contradicts the "round-trip safe" guarantee documented in
contrib/pg_plan_advice/README.
When postgres_fdw pushes an aggregate down over a single foreign table, the
resulting ForeignScan has scanrelid == 0 but fs_relids names exactly one
relation. pg_plan_advice generates FOREIGN_JOIN(<rel>) advice for it.
However,
the advice parser requires a FOREIGN_JOIN target to name more than one
relation, so feeding the generated advice back in fails to parse.
*Reproducer* (uses a loopback postgres_fdw server pointing back at the same
cluster; adjust host/port/dbname/user to match your instance):
LOAD 'pg_plan_advice';
CREATE EXTENSION IF NOT EXISTS postgres_fdw;
CREATE SERVER loopback FOREIGN DATA WRAPPER postgres_fdw
OPTIONS (host 'localhost', port '5432', dbname 'postgres');
CREATE USER MAPPING FOR CURRENT_USER SERVER loopback;
CREATE TABLE base_tab (a int);
CREATE FOREIGN TABLE ftab (a int)
SERVER loopback OPTIONS (table_name 'base_tab');
-- aggregate pushed down: a single-relation ForeignScan (scanrelid 0)
EXPLAIN (COSTS OFF, PLAN_ADVICE) SELECT count(*) FROM ftab;
-- now feed the generated advice back in
SET pg_plan_advice.advice = 'FOREIGN_JOIN(ftab)';
EXPLAIN (COSTS OFF, PLAN_ADVICE) SELECT count(*) FROM ftab;
*On unpatched master this produces:*
Foreign Scan
Relations: Aggregate on (ftab)
Generated Plan Advice:
FOREIGN_JOIN(ftab) <- single-relation FOREIGN_JOIN
NO_GATHER(ftab)
ERROR: invalid value for parameter "pg_plan_advice.advice":
"FOREIGN_JOIN(ftab)"
DETAIL: Could not parse advice: FOREIGN_JOIN targets must contain more
than
one relation identifier at or near ")"
*Analysis*:
In pgpa_build_scan() (contrib/pg_plan_advice/pgpa_scan.c), a base-relation
foreign scan has scanrelid != 0 and is correctly classified as
PGPA_SCAN_ORDINARY. But an upper-rel foreign scan (such as an aggregate
pushdown) has scanrelid == 0, so it falls into the pgpa_relids() branch,
where
the T_ForeignScan case unconditionally chose PGPA_SCAN_FOREIGN -- even when
fs_relids contains a single relation. The generator then emits FOREIGN_JOIN,
which the parser (pgpa_parser.y) rejects for a single relation.
*Fix*:
The attached patch chooses PGPA_SCAN_FOREIGN only when more than one
relation
is involved (bms_membership(relids) == BMS_MULTIPLE), and otherwise treats
the
foreign scan as an ordinary scan, matching what we already do for
base-relation foreign scans:
--- a/contrib/pg_plan_advice/pgpa_scan.c
+++ b/contrib/pg_plan_advice/pgpa_scan.c
@@ -142,8 +142,19 @@ pgpa_build_scan(...)
* foreign scan, then the foreign join has been pushed
to the
* remote side, and we want that to be reflected in the
* generated advice.
+ *
+ * A foreign scan can also reach this branch while
targeting a
+ * single relation -- for example, when an aggregate
is pushed
+ * down over one relation, so that scanrelid is 0 but
fs_relids
+ * contains exactly one member. There is no foreign
join in
+ * that case, so emitting FOREIGN_JOIN advice would be
wrong
+ * (and not round-trip safe, since a FOREIGN_JOIN
target must
+ * name more than one relation). Treat it as an
ordinary scan.
*/
- strategy = PGPA_SCAN_FOREIGN;
+ if (bms_membership(relids) == BMS_MULTIPLE)
+ strategy = PGPA_SCAN_FOREIGN;
+ else
+ strategy = PGPA_SCAN_ORDINARY;
break;
The patch also adds a TAP test (contrib/pg_plan_advice/t/001_foreign_scan.pl
)
using a loopback postgres_fdw server, since exercising this requires both
pg_plan_advice and postgres_fdw. The test verifies that:
(1) no FOREIGN_JOIN advice is generated for a single-relation foreign
scan,
(2) the generated advice is round-trip safe, and
(3) a genuine, pushed-down multi-relation foreign join still gets
FOREIGN_JOIN advice.
The TAP test fails on unpatched master and passes with the fix; the existing
pg_plan_advice regression suite continues to pass.
Patch attached. Please review this patch and let me know feedback.
--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
From 73e9a67695c35915d2f298654077dd57dbbff295 Mon Sep 17 00:00:00 2001
From: Mahendra Singh Thalor <[email protected]>
Date: Tue, 30 Jun 2026 18:10:43 +0530
Subject: [PATCH] pg_plan_advice: Don't generate FOREIGN_JOIN advice for a
single relation.
A foreign scan can target a single relation while still reaching the
fs_relids branch of pgpa_build_scan() -- for example, when postgres_fdw
pushes an aggregate down over one foreign table, so that scanrelid is 0
but fs_relids contains exactly one member. In that case there is no
foreign join, but we nonetheless emitted FOREIGN_JOIN advice for the lone
relation. That advice is not round-trip safe: the parser rejects a
FOREIGN_JOIN target that does not name more than one relation, so feeding
the generated advice back in fails with "FOREIGN_JOIN targets must contain
more than one relation identifier".
Fix by only choosing the PGPA_SCAN_FOREIGN strategy when more than one
relation is involved; treat a single-relation foreign scan as an ordinary
scan, as we already do for base-relation foreign scans.
Add a TAP test using a loopback postgres_fdw server, since exercising this
requires both pg_plan_advice and postgres_fdw.
---
contrib/pg_plan_advice/Makefile | 4 +-
contrib/pg_plan_advice/meson.build | 5 ++
contrib/pg_plan_advice/pgpa_scan.c | 13 ++-
contrib/pg_plan_advice/t/001_foreign_scan.pl | 85 ++++++++++++++++++++
4 files changed, 105 insertions(+), 2 deletions(-)
create mode 100644 contrib/pg_plan_advice/t/001_foreign_scan.pl
diff --git a/contrib/pg_plan_advice/Makefile b/contrib/pg_plan_advice/Makefile
index d016723794d..c844846dd54 100644
--- a/contrib/pg_plan_advice/Makefile
+++ b/contrib/pg_plan_advice/Makefile
@@ -22,7 +22,9 @@ PGFILEDESC = "pg_plan_advice - help the planner get the right plan"
REGRESS = alternatives gather join_order join_strategy partitionwise \
prepared scan semijoin syntax
-EXTRA_INSTALL = contrib/tsm_system_time
+TAP_TESTS = 1
+
+EXTRA_INSTALL = contrib/tsm_system_time contrib/postgres_fdw
EXTRA_CLEAN = pgpa_parser.h pgpa_parser.c pgpa_scanner.c
diff --git a/contrib/pg_plan_advice/meson.build b/contrib/pg_plan_advice/meson.build
index f2098947b64..bbab676be31 100644
--- a/contrib/pg_plan_advice/meson.build
+++ b/contrib/pg_plan_advice/meson.build
@@ -64,4 +64,9 @@ tests += {
'syntax',
],
},
+ 'tap': {
+ 'tests': [
+ 't/001_foreign_scan.pl',
+ ],
+ },
}
diff --git a/contrib/pg_plan_advice/pgpa_scan.c b/contrib/pg_plan_advice/pgpa_scan.c
index 21b58a0ac42..0798b82350d 100644
--- a/contrib/pg_plan_advice/pgpa_scan.c
+++ b/contrib/pg_plan_advice/pgpa_scan.c
@@ -142,8 +142,19 @@ pgpa_build_scan(pgpa_plan_walker_context *walker, Plan *plan,
* foreign scan, then the foreign join has been pushed to the
* remote side, and we want that to be reflected in the
* generated advice.
+ *
+ * A foreign scan can also reach this branch while targeting a
+ * single relation -- for example, when an aggregate is pushed
+ * down over one relation, so that scanrelid is 0 but fs_relids
+ * contains exactly one member. There is no foreign join in
+ * that case, so emitting FOREIGN_JOIN advice would be wrong
+ * (and not round-trip safe, since a FOREIGN_JOIN target must
+ * name more than one relation). Treat it as an ordinary scan.
*/
- strategy = PGPA_SCAN_FOREIGN;
+ if (bms_membership(relids) == BMS_MULTIPLE)
+ strategy = PGPA_SCAN_FOREIGN;
+ else
+ strategy = PGPA_SCAN_ORDINARY;
break;
case T_Append:
diff --git a/contrib/pg_plan_advice/t/001_foreign_scan.pl b/contrib/pg_plan_advice/t/001_foreign_scan.pl
new file mode 100644
index 00000000000..950da747966
--- /dev/null
+++ b/contrib/pg_plan_advice/t/001_foreign_scan.pl
@@ -0,0 +1,85 @@
+# Copyright (c) 2021-2026, PostgreSQL Global Development Group
+
+# Verify that pg_plan_advice generates round-trip-safe advice for foreign
+# scans. This needs postgres_fdw, so it lives in a TAP test rather than in the
+# pg_regress suite.
+#
+# In particular, a foreign scan that targets a single relation (for example,
+# when postgres_fdw pushes an aggregate down over one foreign table) must not
+# get FOREIGN_JOIN advice: there is no foreign join, and a FOREIGN_JOIN target
+# is required to name more than one relation, so such advice would not be
+# round-trip safe.
+
+use strict;
+use warnings FATAL => 'all';
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# A loopback postgres_fdw server pointing back at this same cluster lets us
+# exercise foreign-scan plans without a second node. pg_plan_advice is loaded
+# into every session so that the PLAN_ADVICE EXPLAIN option is available.
+my $node = PostgreSQL::Test::Cluster->new('node');
+$node->init;
+$node->append_conf('postgresql.conf',
+ "session_preload_libraries = 'pg_plan_advice'");
+$node->start;
+
+my $host = $node->host;
+my $port = $node->port;
+
+$node->safe_psql(
+ 'postgres', qq{
+ CREATE EXTENSION postgres_fdw;
+ CREATE SERVER loopback FOREIGN DATA WRAPPER postgres_fdw
+ OPTIONS (host '$host', port '$port', dbname 'postgres');
+ CREATE USER MAPPING FOR CURRENT_USER SERVER loopback;
+ CREATE TABLE base_tab (a int);
+ CREATE TABLE base_tab2 (a int);
+ CREATE FOREIGN TABLE ftab (a int)
+ SERVER loopback OPTIONS (table_name 'base_tab');
+ CREATE FOREIGN TABLE ftab2 (a int)
+ SERVER loopback OPTIONS (table_name 'base_tab2');
+});
+
+# A pushed-down aggregate over a single foreign table yields a ForeignScan
+# that names exactly one relation. No FOREIGN_JOIN advice should be generated.
+my $advice = $node->safe_psql('postgres',
+ "EXPLAIN (COSTS OFF, PLAN_ADVICE) SELECT count(*) FROM ftab;");
+unlike($advice, qr/FOREIGN_JOIN/,
+ 'no FOREIGN_JOIN advice for a single-relation foreign scan');
+
+# The generated advice must be round-trip safe: feeding it back in as supplied
+# advice must not raise a parse error.
+my @items;
+my $collecting = 0;
+foreach my $line (split /\n/, $advice)
+{
+ $collecting = 1, next if $line =~ /Generated Plan Advice:/;
+ next unless $collecting;
+ push @items, $1 if $line =~ /^\s+(\S.*\S)\s*$/;
+}
+my $generated = join(' ', @items);
+my ($ret, $stdout, $stderr) = $node->psql('postgres',
+ "SET pg_plan_advice.advice = '$generated'; "
+ . "EXPLAIN (COSTS OFF) SELECT count(*) FROM ftab;");
+is($ret, 0, 'generated advice for a single-relation foreign scan is round-trip safe')
+ or diag("generated advice: <$generated>\nstderr: $stderr");
+
+# A genuine multi-relation foreign join (here forced by disabling local join
+# methods so the join must be pushed to the remote side) must still generate
+# FOREIGN_JOIN advice.
+my $join_advice = $node->safe_psql(
+ 'postgres', q{
+ SET enable_mergejoin = off;
+ SET enable_hashjoin = off;
+ SET enable_nestloop = off;
+ EXPLAIN (COSTS OFF, PLAN_ADVICE) SELECT * FROM ftab JOIN ftab2 USING (a);
+});
+like($join_advice, qr/FOREIGN_JOIN/,
+ 'FOREIGN_JOIN advice is still generated for a pushed-down foreign join');
+
+$node->stop;
+
+done_testing();
--
2.52.0
