Re: Asynchronous Append on postgres_fdw nodes.

At Tue, 30 Mar 2021 20:40:35 +0900, Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote in
> On Mon, Mar 29, 2021 at 6:50 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
> > I think the patch would be committable.
>
> Here is a new version of the patch.
>
> * Rebased the patch against HEAD.
> * Tweaked docs/comments a bit further.
> * Added the commit message. Does that make sense?
>
> I'm happy with the patch, so I'll commit it if there are no objections.

Thanks for the patch.

May I ask some questions?

+ <term><literal>async_capable</literal></term>
+ <listitem>
+ <para>
+ This option controls whether <filename>postgres_fdw</filename> allows
+ foreign tables to be scanned concurrently for asynchronous execution.
+ It can be specified for a foreign table or a foreign server.

Isn't it strange that an option named "async_capable" *allows* async?

+ * We'll prefer to consider this join async-capable if any table from
+ * either side of the join is considered async-capable.
+ */
+ fpinfo->async_capable = fpinfo_o->async_capable ||
+ fpinfo_i->async_capable;

We need to explain this behavior in the documentation.

Regarding to the wording "async capable", if it literally represents
the capability to run asynchronously, when any one element of a
combined path doesn't have the capability, the whole path cannot be
async-capable. If it represents allowance for an element to run
asynchronously, then the whole path is inhibited to run asynchronously
unless all elements are allowed to do so. If it represents
enforcement or suggestion to run asynchronously, enforcing asynchrony
to an element would lead to running the whole path asynchronously
since all elements of postgres_fdw are capable to run asynchronously
as the nature.

It looks somewhat inconsistent to be inhibitive for the default value
of "async_capable", but agressive in merging?

If I'm wrong in the understanding, please feel free to go ahead.

regrds.

--
Kyotaro Horiguchi
NTT Open Source Software Center