Re: Asynchronous Append on postgres_fdw nodes.

At Sun, 4 Oct 2020 18:36:05 +0900, Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote in
> On Fri, Oct 2, 2020 at 3:39 PM Kyotaro Horiguchi
> <horikyota(dot)ntt(at)gmail(dot)com> wrote:
> > At Fri, 2 Oct 2020 09:00:53 +0900, Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote in
> > > I think we should avoid changing the ExecProcNode() API.
>
> > Could you explain about what the "change" you are mentioning is?

Thank you for the explanation.

> 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.

> > > 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?
>
> Sorry, my explanation was not enough.
>
> 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].

(Putting aside the bug itself..)

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.

Being said, I agree that it is a candidate to rip out when we are
thinking to reduce the footprint of this patch.

> 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?

> [1] https://www.postgresql.org/message-id/CAPmGK16E1erFV9STg8yokoewY6E-zEJtLzHUJcQx%2B3dyivCT%3DA%40mail.gmail.com
> [2] https://www.postgresql.org/message-id/CAPmGK16%2By8mEX9AT1LXVLksbTyDnYWZXm0uDxZ8bza153Wey9A%40mail.gmail.com
> [3] https://www.postgresql.org/message-id/CAPmGK14AjvCd9QuoRQ-ATyExA_SiVmGFGstuqAKSzZ7JDJTBVg%40mail.gmail.com

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center