Hi Amit-san,
On Wed, Feb 2, 2022 at 12:59 PM Amit Langote <amitlangote09@gmail.com> wrote:
> Your proposed fix makes sense.
Cool!
> 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.
> 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.
> 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!
Best regards,
Etsuro Fujita