Re: [HACKERS] Transactions involving multiple postgres foreign servers, take 2

On Wed, Oct 24, 2018 at 9:06 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Tue, Oct 23, 2018 at 12:54 PM Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> >
> > Hello.
> >
> > # It took a long time to come here..
> >
> > At Fri, 19 Oct 2018 21:38:35 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoCBf-AJup-_ARfpqR42gJQ_XjNsvv-XE0rCOCLEkT=HCg(at)mail(dot)gmail(dot)com>
> > > On Wed, Oct 10, 2018 at 1:34 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > ...
> > > * Updated docs, added the new section "Distributed Transaction" at
> > > Chapter 33 to explain the concept to users
> > >
> > > * Moved atomic commit codes into src/backend/access/fdwxact directory.
> > >
> > > * Some bug fixes.
> > >
> > > Please reivew them.
> >
> > I have some comments, with apologize in advance for possible
> > duplicate or conflict with others' comments so far.
>
> Thank youf so much for reviewing this patch!
>
> >
> > 0001:
> >
> > This sets XACT_FLAG_WROTENONTEMPREL when RELPERSISTENT_PERMANENT
> > relation is modified. Isn't it needed when UNLOGGED tables are
> > modified? It may be better that we have dedicated classification
> > macro or function.
>
> I think even if we do atomic commit for modifying the an UNLOGGED
> table and a remote table the data will get inconsistent if the local
> server crashes. For example, if the local server crashes after
> prepared the transaction on foreign server but before the local commit
> and, we will lose the all data of the local UNLOGGED table whereas the
> modification of remote table is rollbacked. In case of persistent
> tables, the data consistency is left. So I think the keeping data
> consistency between remote data and local unlogged table is difficult
> and want to leave it as a restriction for now. Am I missing something?
>
> >
> > The flag is handled in heapam.c. I suppose that it should be done
> > in the upper layer considering coming pluggable storage.
> > (X_F_ACCESSEDTEMPREL is set in heapam, but..)
> >
>
> Yeah, or we can set the flag after heap_insert in ExecInsert.
>
> >
> > 0002:
> >
> > The name FdwXactParticipantsForAC doesn't sound good for me. How
> > about FdwXactAtomicCommitPartitcipants?
>
> +1, will fix it.
>
> >
> > Well, as the file comment of fdwxact.c,
> > FdwXactRegisterTransaction is called from FDW driver and
> > F_X_MarkForeignTransactionModified is called from executor. I
> > think that we should clarify who is responsible to the whole
> > sequence. Since the state of local tables affects, I suppose
> > executor is that. Couldn't we do the whole thing within executor
> > side? I'm not sure but I feel that
> > F_X_RegisterForeignTransaction can be a part of
> > F_X_MarkForeignTransactionModified. The callers of
> > MarkForeignTransactionModified can find whether the table is
> > involved in 2pc by IsTwoPhaseCommitEnabled interface.
>
> Indeed. We can register foreign servers by executor while FDWs don't
> need to register anything. I will remove the registration function so
> that FDW developers don't need to call the register function but only
> need to provide atomic commit APIs.
>
> >
> >
> > > if (foreign_twophase_commit == true &&
> > > ((MyXactFlags & XACT_FLAGS_FDWNOPREPARE) != 0) )
> > > ereport(ERROR,
> > > (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> > > errmsg("cannot COMMIT a distributed transaction that has operated on foreign server that doesn't support atomic commit")));
> >
> > The error is emitted when a the GUC is turned off in the
> > trasaction where MarkTransactionModify'ed. I think that the
> > number of the variables' possible states should be reduced for
> > simplicity. For example in the case, once foreign_twopase_commit
> > is checked in a transaction, subsequent changes in the
> > transaction should be ignored during the transaction.
> >
>
> I might have not gotten your comment correctly but since the
> foreign_twophase_commit is a PGC_USERSET parameter I think we need to
> check it at commit time. Also we need to keep participant servers even
> when foreign_twophase_commit is off if both max_prepared_foreign_xacts
> and max_foreign_xact_resolvers are > 0.
>
> I will post the updated patch in this week.
>

Attached the updated version patches.

Based on the review comment from Horiguchi-san, I've changed the
atomic commit API so that the FDW developer who wish to support atomic
commit don't need to call the register function. The atomic commit
APIs are following:

* GetPrepareId
* PrepareForeignTransaction
* CommitForeignTransaction
* RollbackForeignTransaction
* ResolveForeignTransaction
* IsTwophaseCommitEnabled

The all APIs except for GetPreapreId is required for atomic commit.

Also, I've changed the foreign_twophase_commit parameter to an enum
parameter based on the suggestion from Robert[1]. Valid values are
'required', 'prefer' and 'disabled' (default). When set to either
'required' or 'prefer' the atomic commit will be used. The difference
between 'required' and 'prefer' is that when set to 'requried' we
require for *all* modified server to be able to use 2pc whereas when
'prefer' we require 2pc where available. So if any of written
participants disables 2pc or doesn't support atomic comit API the
transaction fails. IOW, when 'required' we can commit only when data
consistency among all participant can be left.

Please review the patches.

[1] https://www.postgresql.org/message-id/CA%2BTgmob4EqxbaMp0e--jUKYT44RL4xBXkPMxF9EEAD%2ByBGAdxw%40mail.gmail.com

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center