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

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)"

Best regards,
Etsuro Fujita