pg_plan_advice: FOREIGN_JOIN advice generated for a single-relation foreign scan is not round-trip safe

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