Horton

Thread

Commits

  1. pg_plan_advice: Don't generate FOREIGN_JOIN advice for a single relation.

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

    Mahendra Singh Thalor <mahi6run@gmail.com> — 2026-06-30T14:36:40Z

    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

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

    Robert Haas <robertmhaas@gmail.com> — 2026-06-30T14:52:07Z

    On Tue, Jun 30, 2026 at 10:36 AM Mahendra Singh Thalor
<mahi6run@gmail.com> wrote:
> 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.

Thanks for the report. I think this patch looks correct, but the
comments are a bit overly verbose. Barring objections or other
comments, I'll tighten this up, commit and back-patch.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

    Robert Haas <robertmhaas@gmail.com> — 2026-07-02T20:20:28Z

    On Tue, Jun 30, 2026 at 10:52 AM Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Jun 30, 2026 at 10:36 AM Mahendra Singh Thalor
> <mahi6run@gmail.com> wrote:
> > 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.
>
> Thanks for the report. I think this patch looks correct, but the
> comments are a bit overly verbose. Barring objections or other
> comments, I'll tighten this up, commit and back-patch.

Done. I ended up making some changes to the test case as well,
basically to make it less likely that it could accidentally pass.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

    Richard Guo <guofenglinux@gmail.com> — 2026-07-03T02:27:22Z

    On Fri, Jul 3, 2026 at 5:20 AM Robert Haas <robertmhaas@gmail.com> wrote:
> Done. I ended up making some changes to the test case as well,
> basically to make it less likely that it could accidentally pass.

Seems pgindent is needed for this commit.

- Richard

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

    Richard Guo <guofenglinux@gmail.com> — 2026-07-03T03:36:09Z

    On Fri, Jul 3, 2026 at 11:27 AM Richard Guo <guofenglinux@gmail.com> wrote:
> On Fri, Jul 3, 2026 at 5:20 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > Done. I ended up making some changes to the test case as well,
> > basically to make it less likely that it could accidentally pass.

> Seems pgindent is needed for this commit.

I've just pushed a trivial patch for the pgindent fix.

- Richard

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

    Robert Haas <robertmhaas@gmail.com> — 2026-07-03T11:06:22Z

    On Thu, Jul 2, 2026 at 11:36 PM Richard Guo <guofenglinux@gmail.com> wrote:
> > Seems pgindent is needed for this commit.
>
> I've just pushed a trivial patch for the pgindent fix.

Thanks, sorry about that.

--
Robert Haas
EDB: http://www.enterprisedb.com