Re: BUG #17355: Server crashes on ExecReScanForeignScan in postgres_fdw when accessing foreign partition

Fujita-san,

On Wed, Feb 2, 2022 at 7:55 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Wed, Feb 2, 2022 at 12:59 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > I was trying to understand in what way commit 86dc90056's is related
> > to this, because you mention it here in the thread and also in the
> > commit message.  I see that c3928b467, also mentioned in your commit
> > message, fixed a related bug [1], which in turn refers to 1375422c as
> > the cause [2].
>
> I think the diagnosis in commit c3928b467 isn't correct; I think the
> cause is commit 86dc90056, because before that commit, direct-modify
> ForeignScan nodes didn't get called during EvalPlanQual processing,
> even in the case of an inherited UPDATE/DELETE.  (In direct
> modification, all the work for it except the RETURNING computation is
> done on the remote side, so we don't need to invoke EvalPlanQual!)
> BUT as you know, commit 86dc90056 changed things so that such
> ForeignScan nodes can get called during EvalPlanQual processing, as
> they are contained as part of the EvalPlanQual subtree in the case of
> an inherited UPDATE/DELETE, which I think led to the issues in commit
> c3928b467 as well as this bug report.

You're right, thanks for the explanation.

What I embarrassingly forgot is that, before 86dc90056, each child
subplan would be set to be passed to EvalPlanQ ual() when in its turn
came in the ExecModifyTable()'s main loo, using the following code
that 86dc90056 removed:

        if (TupIsNull(planSlot))
-       {
-           /* advance to next subplan if any */
-           node->mt_whichplan++;
-           if (node->mt_whichplan < node->mt_nplans)
-           {
-               resultRelInfo++;
-               subplanstate = node->mt_plans[node->mt_whichplan];
-               junkfilter = resultRelInfo->ri_junkFilter;
-               EvalPlanQualSetPlan(&node->mt_epqstate, subplanstate->plan,
-                                   node->mt_arowmarks[node->mt_whichplan]);
-               continue;
-           }
-           else
-               break;
-       }

Direct-modify subplans, even if they would get set to be passed to
EvalPlanQual(), would never actually be processed for the reasons you
mention.  Now that there's only one subplan and all the child subplans
just its subnodes, any direct-modify nodes *would* get processed
because they're in the plan that gets passed to EvalPlanQual().

> > Before commit 86dc90056, EPQState.plan would be set to the 1st child
> > subplan, which if it happened to be a direct-modify ForeignScan node,
> > this bug would manifest.  After that commit though, a direct-modify
> > child subplan would *always* be present in EPQState.plan by way of its
> > parent Append node or some parent node thereof being set as
> > EPQState.plan, so the bug would now always manifest.  So my
> > understanding is that the commit 86dc90056 only changed *when* the bug
> > manifests, which is now "always" if direct-modify children are
> > present.  Is that understanding correct?
>
> As I mentioned above, I think we didn't have this bug before commit
> 86dc90056.  As for when this bug happens, I think it would depend on
> the partitioned-table definition; eg, for a partitioned table like the
> one discussed in the thread for commit c3928b467, it wouldn't happen.

Yeah, got it.

> > BTW, isn't the following code in ForeignNext() added by c3928b467
> > non-reachable after your patch:
> >
> >         /*
> >          * direct modifications cannot be re-evaluated, so shouldn't get here
> >          * during EvalPlanQual processing
> >          */
> >         if (estate->es_epq_active != NULL)
> >             elog(ERROR, "cannot re-evaluate a Foreign Update or Delete
> > during EvalPlanQual");
> >
> > Should that be converted to an Assert(estate->es_epq_active == NULL)?
>
> +1  I updated the patch as such.  Attached is a new version.  I also
> tweaked a comment a litttle bit further.
>
> Thanks!

Thanks, looks good to me.

-- 
Amit Langote
EDB: http://www.enterprisedb.com