Re: Transactions involving multiple postgres foreign servers, take 2 - Mailing list pgsql-hackers

On Wed, Jun 3, 2020 at 12:02 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> On Wed, 3 Jun 2020 at 14:50, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> >
> > If the intention is to keep the first version simple, then why do we
> > want to support any mode other than 'required'?  I think it will limit
> > its usage for the cases where 2PC can be used only when all FDWs
> > involved support Prepare API but if that helps to keep the design and
> > patch simpler then why not just do that for the first version and then
> > extend it later.  OTOH, if you think it will be really useful to keep
> > other modes, then also we could try to keep those in separate patches
> > to facilitate the review and discussion of the core feature.
>
> ‘disabled’ is the fundamental mode. We also need 'disabled' mode,
> otherwise existing FDW won't work.
>

IIUC, if foreign_twophase_commit is 'disabled', we don't use a
two-phase protocol to commit distributed transactions, right?  So, do
we check this at the time of Prepare or Commit whether we need to use
a two-phase protocol?  I think this should be checked at prepare time.

+        <para>
+         This parameter can be changed at any time; the behavior for any one
+         transaction is determined by the setting in effect when it commits.
+        </para>

This is written w.r.t foreign_twophase_commit.  If one changes this
between prepare and commit, will it have any impact?

>  I was concerned that many FDW
> plugins don't implement FDW transaction APIs yet when users start
> using this feature. But it seems to be a good idea to move 'prefer'
> mode to a separate patch while leaving 'required'. I'll do that in the
> next version patch.
>

Okay, thanks.  Please, see if you can separate out the documentation
for that as well.

Few other comments on v21-0003-Documentation-update:
----------------------------------------------------
1.
+      <entry></entry>
+      <entry>
+       Numeric transaction identifier with that this foreign transaction
+       associates
+      </entry>

/with that this/with which this

2.
+      <entry>
+       The OID of the foreign server on that the foreign transaction
is prepared
+      </entry>

/on that the/on which the

3.
+      <entry><structfield>status</structfield></entry>
+      <entry><type>text</type></entry>
+      <entry></entry>
+      <entry>
+       Status of foreign transaction. Possible values are:
+       <itemizedlist>
+        <listitem>
+         <para>
+          <literal>initial</literal> : Initial status.
+         </para>

What exactly "Initial status" means?

4.
+      <entry><structfield>in_doubt</structfield></entry>
+      <entry><type>boolean</type></entry>
+      <entry></entry>
+      <entry>
+       If <literal>true</literal> this foreign transaction is
in-doubt status and
+       needs to be resolved by calling <function>pg_resolve_fdwxact</function>
+       function.
+      </entry>

It would be better if you can add an additional sentence to say when
and or how can foreign transactions reach in-doubt state.

5.
If <literal>N</literal> local transactions each
+         across <literal>K</literal> foreign server this value need to be set

This part of the sentence can be improved by saying something like:
"If a user expects N local transactions and each of those involves K
foreign servers, this value..".

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com