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

On Mon, Oct 29, 2018 at 6:03 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Oct 29, 2018 at 10:16 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Wed, Oct 24, 2018 at 9:06 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > On Tue, Oct 23, 2018 at 12:54 PM Kyotaro HORIGUCHI
> > > <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> > > >
> > > > Hello.
> > > >
> > > > # It took a long time to come here..
> > > >
> > > > At Fri, 19 Oct 2018 21:38:35 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
<CAD21AoCBf-AJup-_ARfpqR42gJQ_XjNsvv-XE0rCOCLEkT=HCg@mail.gmail.com>
> > > > > On Wed, Oct 10, 2018 at 1:34 PM Masahiko Sawada <sawada.mshk@gmail.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
serverthat 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.
> >
>
> Since the previous patch conflicts with current HEAD attached updated
> set of patches.
>

Rebased and fixed a few bugs.

Regards,

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