MIME-Version: 1.0
References: <CAPmGK17dAZCXvwnfpr1eTfknTGdt=hYTV9405Gt5SqPOX8K84w@mail.gmail.com>
 <65af8fb4-4c45-9cc3-c6ec-80f3ac488e07@oss.nttdata.com> <CAPmGK17XczZmzU=0mhk2_oG9Omo1p-uZCNdZAh_3vuC6+YboiA@mail.gmail.com>
 <95066202-c7e6-e4be-05cc-bdceb45b68c7@oss.nttdata.com> <CAPmGK14rr1R_t4xxWyJVng04nDjGnxi=J5fz-TBNBH9Xn0OFYQ@mail.gmail.com>
 <2d7c480a-63c8-a244-0f2a-361e5097764f@oss.nttdata.com> <CAPmGK15FuPVGx3TGHKShsbPKKtF1y58-ZLcKoxfN-nqLj1dZ=g@mail.gmail.com>
 <d888307d-c5b2-88e7-e2c0-f2228ad0756b@oss.nttdata.com> <CAPmGK158hrd=ZfXmgkmNFHivgh18e4oE2Gz151C2Q4OBDjZ08A@mail.gmail.com>
 <3e57ebd7-b536-333e-c877-49a3ebb57079@oss.nttdata.com> <CAPmGK17nfEZkKhqoP8fAPKbnfSpf2JVgV5+_Xjv2mn8P-4X0hQ@mail.gmail.com>
 <2a2519b5-0c1f-87c8-5461-a4d8be0728e0@oss.nttdata.com>
In-Reply-To: <2a2519b5-0c1f-87c8-5461-a4d8be0728e0@oss.nttdata.com>
From: Etsuro Fujita <etsuro.fujita@gmail.com>
Date: Mon, 21 Feb 2022 14:45:10 +0900
Message-ID: <CAPmGK152dsQHAzZC-g55M08Z-ur-h8m4J7FD0L3eh5uazCmvAQ@mail.gmail.com>
Subject: Re: postgres_fdw: commit remote (sub)transactions in parallel during pre-commit
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAPmGK152dsQHAzZC-g55M08Z-ur-h8m4J7FD0L3eh5uazCmvAQ%40mail.gmail.com>
Precedence: bulk

On Fri, Feb 18, 2022 at 1:46 AM Fujii Masao <masao.fujii@oss.nttdata.com> w=
rote:
> I reviewed 0001 patch. It looks good to me except the following minor thi=
ngs. If these are addressed, I think that the 001 patch can be marked as re=
ady for committer.

OK

> +        * Also determine to commit (sub)transactions opened on the remot=
e server
> +        * in parallel at (sub)transaction end.
>
> Like the comment "Determine whether to keep the connection ...", "determi=
ne to commit" should be "determine whether to commit"?

Agreed.  I=E2=80=99ll change it as such.

> "remote server" should be "remote servers"?

Maybe I=E2=80=99m missing something, but we determine this for the given
remote server, so it seems to me correct to say =E2=80=9Cthe remote server=
=E2=80=9D,
not =E2=80=9Cthe remote servers=E2=80=9C.

> +       curlevel =3D GetCurrentTransactionNestLevel();
> +       snprintf(sql, sizeof(sql), "RELEASE SAVEPOINT s%d", curlevel);
>
> Why does pgfdw_finish_pre_subcommit_cleanup() need to call GetCurrentTran=
sactionNestLevel() and construct the "RELEASE SAVEPOINT" query string again=
? pgfdw_subxact_callback() already does them and probably we can make it pa=
ss either of them to pgfdw_finish_pre_subcommit_cleanup() as its argument.

Yeah, that would save cycles, but I think that that makes code a bit
unclean IMO.  (To save cycles, I think we could also modify
pgfdw_subxact_callback() to reuse the query in the while loop in that
function when processing multiple open remote subtransactions there,
but that would make code a bit complicated, so I don=E2=80=99t think it=E2=
=80=99s a
good idea to do so, either.)  So I=E2=80=99d vote for reconstructing the qu=
ery
in pgfdw_finish_pre_subcommit_cleanup() as we do in
pgfdw_subxact_callback().

To avoid calling GetCurrentTransactionNestLevel() again, I think we
could pass the curlevel variable to that function.

> +       This option controls whether <filename>postgres_fdw</filename> co=
mmits
> +       remote (sub)transactions opened on a foreign server in a local
> +       (sub)transaction in parallel when the local (sub)transaction comm=
its.
>
> "a foreign server" should be "foreign servers"?

I thought it would be good to say =E2=80=9Ca foreign server=E2=80=9D, not =
=E2=80=9Cforeign
servers=E2=80=9D, because it makes clear that even remote transactions open=
ed
on a single foreign server are committed in parallel.  (To say that
this option is not for a specific foreign server, I added to the
documentation =E2=80=9CThis option can only be specified for foreign
servers=E2=80=9D.)

> "in a local (sub)transaction" part seems not to be necessary.

And I thought adding it would make clear which remote transactions are
committed in parallel.  But maybe I=E2=80=99m missing something, so could y=
ou
elaborate a bit more on these?

Thanks for reviewing!

Best regards,
Etsuro Fujita