Thread

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

Etsuro Fujita <etsuro.fujita@gmail.com> — 2025-10-02T11:01:07Z

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.

Best regards,
Etsuro Fujita