Re: The case when AsyncAppend exists also in the qual of Async ForeignScan

On Fri, Jul 23, 2021 at 3:09 PM Andrey V. Lepikhov
<a.lepikhov@postgrespro.ru> wrote:
> On 7/22/21 4:14 PM, Etsuro Fujita wrote:
> > On Fri, Jul 2, 2021 at 10:24 PM Andrey Lepikhov
> > @@ -7015,6 +7015,21 @@ process_pending_request(AsyncRequest *areq)
> >
> >      fetch_more_data(node);
> >
> > +   /*
> > +    * If the request are made by another append we will only prepare connection
> > +    * for the next query and don't take a tuple immediately. It is needed to
> > +    * prevent possible recursion into a qual subplan.
> > +    */
> > +   if (!fetch)
> > +   {
> > +       AppendState *node = (AppendState *) areq->requestor;
> > +
> > +       ExecAsyncRequestDone(areq, NULL);
> > +       node->as_needrequest = bms_add_member(node->as_needrequest,
> > +                                             areq->request_index);
> > +       return;
> > +   }
> >
> > I don’t think this is a good idea, because it is pretty inconsistent,
> > as doing ExecAsyncRequestDone(areq, NULL) means that there are no more
> > tuples while changing as_needrequest like that means that there is at
> > least one tuple to return.  This would happen to work, but for
> > example, if we add to the core more sanity checks on AsyncRequests,
> > this would not work well anymore.

> > So I feel inclined to
> > disable async execution in cases where async-capable nodes access to
> > subplans (or initplans), for now.

> I think it can be done, but only as a temporary solution.

My concern about that is that such an inconsistency would make the
code complicated, and thus make the maintenance hard.

> Maybe we can split async logic into:
> - receiving stage, when we only fetch and store tuples,
> - evaluating stage, when we form resulting tuple and return by a scan node.
> I will think about such solution more.

One simple solution along this line I came up with, which is not the
rewrite, is to 1) split process_pending_request() into the two steps,
and 2) postpone the second step until we are called from
postgresForeignAsyncConfigureWait(), like the attached, which I think
would be much consistent with the existing logic.

> Also, may be you tell your opinion about an additional optimization of
> Async Append [1]?

Is the optimization related to this issue?  (Sorry, I didn’t have time
for reviewing the patch in [1] than expected.  I plan to do so next
month.)

Best regards,
Etsuro Fujita