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

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. I tried to devise a consistent
> solution for this issue, but I couldn’t. So I feel inclined to
> disable async execution in cases where async-capable nodes access to
> subplans (or initplans), for now.

That do you think, if (in addition to the patch) we will give
ExecAsyncAppendResponse a chance to fill the result slot? Code:

--- a/src/backend/executor/nodeAppend.c
+++ b/src/backend/executor/nodeAppend.c
@@ -1108,6 +1108,9 @@ ExecAsyncAppendResponse(AsyncRequest *areq)
return;
}

+ if (areq->result == NULL)
+ areq->result =
areq->requestee->ExecProcNodeReal(areq->requestee);
+
/* If the result is NULL or an empty slot, there's nothing more
to do. */
if (TupIsNull(slot))
{

--
regards,
Andrey Lepikhov
Postgres Professional