Re: Asynchronous Append on postgres_fdw nodes.

On Tue, Sep 29, 2020 at 4:45 AM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
> BTW: I noticed that you changed the ExecProcNode() API so that an
> Append calling FDWs can know wether they return tuples immediately or
> not:

> That is, 1) in postgresIterateForeignScan() postgres_fdw sets the new
> PlanState’s flag asyncstate to AS_AVAILABLE/AS_WAITING depending on
> whether it returns a tuple immediately or not, and then 2) the Append
> knows that from the new flag when the callback routine returns. I’m
> not sure this is a good idea, because it seems likely that the
> ExecProcNode() change would affect many other places in the executor,
> making maintenance and/or future development difficult. I think the
> FDW callback routines proposed in the original patch by Robert would
> provide a cleaner way to do asynchronous execution of FDWs without
> changing the ExecProcNode() API, IIUC:
>
> +On the other hand, nodes that wish to produce tuples asynchronously
> +generally need to implement three methods:
> +
> +1. When an asynchronous request is made, the node's ExecAsyncRequest callback
> +will be invoked; it should use ExecAsyncSetRequiredEvents to indicate the
> +number of file descriptor events for which it wishes to wait and whether it
> +wishes to receive a callback when the process latch is set. Alternatively,
> +it can instead use ExecAsyncRequestDone if a result is available immediately.
> +
> +2. When the event loop wishes to wait or poll for file descriptor events and
> +the process latch, the ExecAsyncConfigureWait callback is invoked to configure
> +the file descriptor wait events for which the node wishes to wait. This
> +callback isn't needed if the node only cares about the process latch.
> +
> +3. When file descriptors or the process latch become ready, the node's
> +ExecAsyncNotify callback is invoked.
>
> What is the reason for not doing like this in your patch?

I think we should avoid changing the ExecProcNode() API.

Thomas’ patch also provides a clean FDW API that doesn’t change the
ExecProcNode() API, but I think the FDW API provided in Robert’ patch
would be better designed, because I think it would support more
different types of asynchronous interaction between the core and FDWs.
Consider this bit from Thomas’ patch, which produces a tuple when a
file descriptor becomes ready:

+ if (event.events & WL_SOCKET_READABLE)
+ {
+ /* Linear search for the node that told us to wait for this fd. */
+ for (i = 0; i < node->nasyncplans; ++i)
+ {
+ if (event.fd == node->asyncfds[i])
+ {
+ TupleTableSlot *result;
+
+ /*
+ --> * We assume that because the fd is ready, it can produce
+ --> * a tuple now, which is not perfect. An improvement
+ --> * would be if it could say 'not yet, I'm still not
+ --> * ready', so eg postgres_fdw could PQconsumeInput and
+ --> * then say 'I need more input'.
+ */
+ result = ExecProcNode(node->asyncplans[i]);
+ if (!TupIsNull(result))
+ {
+ /*
+ * Remember this plan so that append_next_async will
+ * keep trying this subplan first until it stops
+ * feeding us buffered tuples.
+ */
+ node->lastreadyplan = i;
+ /* We can stop waiting for this fd. */
+ node->asyncfds[i] = 0;
+ return result;
+ }
+ else
+ {
+ /*
+ * This subplan has reached EOF. We'll go back and
+ * wait for another one.
+ */
+ forget_async_subplan(node, i);
+ break;
+ }
+ }
+ }
+ }

As commented above, his patch doesn’t allow an FDW to do another data
fetch from the remote side before returning a tuple when the file
descriptor becomes available, but Robert’s patch would, using his FDW
API ForeignAsyncNotify(), which is called when the file descriptor
becomes available, IIUC.

I might be missing something, but I feel inclined to vote for Robert’s
patch (more precisely, Robert’s patch as a base patch with (1) some
planner/executor changes from Horiguchi-san’s patch and (2)
postgres_fdw changes from Thomas’ patch adjusted to match Robert’s FDW
API).

Best regards,
Etsuro Fujita