From 73e9a67695c35915d2f298654077dd57dbbff295 Mon Sep 17 00:00:00 2001 From: Mahendra Singh Thalor 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