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