Re: Asynchronous Append on postgres_fdw nodes.

On Tue, Sep 29, 2020 at 4:45 AM Etsuro Fujita <etsuro.fujita@gmail.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