Re: Asynchronous Append on postgres_fdw nodes.

On Mon, Oct 5, 2020 at 1:30 PM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)com> wrote:
> At Sun, 4 Oct 2020 18:36:05 +0900, Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote in
> > It’s the contract of the ExecProcNode() API: if the result is NULL or
> > an empty slot, there is nothing more to do. You changed it to
> > something like this: “even if the result is NULL or an empty slot,
> > there might be something more to do if AS_WAITING, so please wait in
> > that case”. That seems pretty invasive to me.
>
> Yeah, it's "invasive' as I intended. I thought that the async-aware
> and async-capable nodes should interact using a channel defined as a
> part of ExecProcNode API. It was aiming an increased affinity to
> push-up executor framework.
>
> Since the current direction is committing this feature as a
> intermediate or tentative implement, it sounds reasonable to avoid
> such a change.

OK (Actually, I'm wondering if we could probably extend this to the
case where an Append is indirectly on top of a foreign scan node
without changing the ExecProcNode() API.)

> > You made lots of changes to the original patch by Robert, but I don’t
> > think those changes are all good; 1) as for the core part, you changed
> > his patch so that FDWs can interact with the core at execution time,
> > only through the ForeignAsyncConfigureWait() API, but that resulted in
> > an invasive change to the ExecProcNode() API as mentioned above, and
> > 2) as for the postgres_fdw part, you changed it so that postgres_fdw
> > can handle concurrent data fetches from multiple foreign scan nodes
> > using the same connection, but that would cause a performance issue
> > that I mentioned in [1].

> Yeah, I noticed such a possibility of fetch cascading, however, I
> think that that situation that the feature is intended for is more
> common than the problem case.

I think a cleaner solution to that would be to support multiple
connections to the remote server...

> > So I think it would be better to use his patch rather as proposed
> > except for the postgres_fdw part and Thomas’ patch as a base patch for
> > that part. As for your patch, I think we could use some part of it as
> > improvements. One thing is the planner/executor changes that lead to
> > the improved efficiency discussed in [2][3]. Another would be to have
> > a separate ExecAppend() function for this feature like your patch to
> > avoid a performance penalty in the case of a plain old Append that
> > involves no FDWs with asynchronism optimization, if necessary. I also
> > think we could probably use the WaitEventSet-related changes in your
> > patch (i.e., the 0001 patch).
> >
> > Does that answer your question?
>
> Yes, thanks. Comments about the direction from me is as above. Are
> you going to continue working on this patch?

Yes, if there are no objections from you or Thomas or Robert or anyone
else, I'll update Robert's patch as such.

Best regards,
Etsuro Fujita