Re: Asynchronous Append on postgres_fdw nodes.

At Fri, 2 Oct 2020 09:00:53 +0900, Etsuro Fujita <etsuro.fujita@gmail.com> wrote in 
> 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

Could you explain about what the "change" you are mentioning is?

I have made many changes to reduce performance inpact on existing
paths (before the current PlanState.ExecProcNode was introduced.) So
large part of my changes could be actually reverted.

> 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]);
..
> 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).

I'm not sure what you have in mind from the description above.  Could
you please ellaborate?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center