Re: TRAP: failed Assert("outerPlan != NULL") in postgres_fdw.c

On Thu, Oct 2, 2025 at 4:01 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>
> On Wed, Oct 1, 2025 at 7:53 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > On Wed, Sep 24, 2025 at 6:42 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > I changed the regression tests and used the fix proposed by
> > > Fujita-san. Please review the attached new version patch.
> >
> > I reviewed the test part.  Here are my comments about it.
> >
> > You renamed the spec file to concurrent_update.spec.  It's a broader
> > name to cover the test case discussed here, but seems a bit vague to
> > me.  How about eval_plan_qual.spec like
> > src/test/isolation/specs/eval-plan-qual.spec, instead?  Which I think
> > is more specific.
> >
> > In the setup block in the test, you created schemas:
> >
> > +    CREATE SCHEMA sch1;
> > +    CREATE TABLE sch1.a (i int);
> > +    CREATE FOREIGN TABLE sch1.b (i int) SERVER loopback OPTIONS
> > (schema_name 'sch2', table_name 'b');
> > +    CREATE FOREIGN TABLE sch1.c (i int) SERVER loopback OPTIONS
> > (schema_name 'sch2', table_name 'c');
> > +
> > +    CREATE SCHEMA sch2;
> > +    CREATE TABLE sch2.b (i int);
> > +    CREATE TABLE sch2.c (i int);
> >
> > But actually, we don't need these schemas.  As I'm planning to add
> > more permutations using the same setup, and setup is executed once per
> > permutation, I'd like to propose to save cycles by creating all these
> > tables in the current schema like this:
> >
> >     CREATE TABLE a (i int);
> >     CREATE TABLE b (i int);
> >     CREATE TABLE c (i int);
> >     CREATE FOREIGN TABLE fb (i int) SERVER loopback OPTIONS (table_name 'b');
> >     CREATE FOREIGN TABLE fc (i int) SERVER loopback OPTIONS (table_name 'c');
> >
> > +step "s1_lock" {
> > +    SELECT a.i,
> > +       (SELECT 1 FROM sch1.b AS b, sch1.c AS c WHERE a.i = b.i AND b.i = c.i)
> > +    FROM sch1.a as a
> > +    FOR UPDATE;
> > +}
> >
> > I think the important point in the test case discussed here is that
> > the sub-select has a foreign-join plan (without an alternative local
> > join plan).  So I'd like to propose to add an EXPLAIN command to this
> > step, to confirm that it has such a plan.
> >
> > Also, how about s/s1_lock/s1_tuplock/?  This is just my taste, though.
> >
> > +# "s1_lock" waits for concurrent update on the tuple on sch1.a table by
> > +# "s0_update", and once s0 transaction is committed it resumes and does EPQ
> > +# recheck for the locked tuple, which should not use postgresRecheckForeignScan
> > +# as the remote join is not a descendant join in the EPQ plan tree.
> > +permutation "s0_begin" "s0_update" "s1_begin" "s1_lock" "s0_commit"
> >
> > Let's add to the permutation s1_commit as well, which I think is a
> > good practice.
> >
> > Considering the permutation is a typical sequence to exercise an
> > EvalPlanQual recheck, I don't feel the need for the first part of the
> > comments, sorry.  Also, IMO I think the second part is a bit too
> > detailed.  How about simplifying the comments to something like this
> > (I used a comment for a similar test in
> > src/test/isolation/specs/eval-plan-qual.spec.):
> >
> > "This test exercises EvalPlanQual with a SubLink sub-select (which
> > should be unaffected by any EPQ recheck behavior in the outer query)"
>
> Some more comments:
>
> +teardown
> +{
> +}
>
> I think it's good to add cleanup statements here; otherwise let's
> remove this for now, to suppress a "teardown failed:" error.
>
> --- a/contrib/postgres_fdw/Makefile
> +++ b/contrib/postgres_fdw/Makefile
> @@ -16,6 +16,8 @@ SHLIB_LINK_INTERNAL = $(libpq)
>  EXTENSION = postgres_fdw
>  DATA = postgres_fdw--1.0.sql postgres_fdw--1.0--1.1.sql
> postgres_fdw--1.1--1.2.sql
>
> +ISOLATION = concurrent_update
> +
>
> I'd like to propose to add "ISOLATION_OPTS =
> --load-extension=postgres_fdw", by which we don't need to do CREATE
> EXTENSION in the setup block.
>
> That's it.
>

Thank you for reviewing the patch!

I've updated the patch based on your comments. Please find the attached patch.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com