Re: Asynchronous Append on postgres_fdw nodes.

On Fri, Oct 2, 2020 at 3:39 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> At Fri, 2 Oct 2020 09:00:53 +0900, Etsuro Fujita <etsuro.fujita@gmail.com> wrote in
> > I think we should avoid changing the ExecProcNode() API.

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

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.

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

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?

Best regards,
Etsuro Fujita

[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