Re: [HACKERS] Transactions involving multiple postgres foreignservers, take 2 - Mailing list pgsql-hackers

On Thu, Nov 15, 2018 at 7:36 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> 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 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.
> > >
> >
> > Since the previous patch conflicts with current HEAD attached updated
> > set of patches.
> >
>
> Rebased and fixed a few bugs.
>

I got feedbacks regarding transaciton management FDW APIs at Japan
PostgreSQL Developer Meetup[1] and am considering to change these APIs
to make them consistent with XA interface[2] (xa_prepare(),
xa_commit() and xa_rollback()) as follows[3].

* FdwXactResult PrepareForeignTransaction(FdwXactState *state, inf flags)
* FdwXactResult CommitForeignTransaction(FdwXactState *state, inf flags)
* FdwXactResult RollbackForeignTransaction(FdwXactState *state, inf flags)
* char *GetPrepareId(TransactionId xid, Oid serverid, Oid userid, int
*prep_id_len)

Where flags set variaous setttings, currently it would contain only
FDW_XACT_FLAG_ONEPHASE that requires FDW to commit in one-phase (i.e.
without preparation). And where *state would contains information
necessary for specifying transaction: serverid, userid, usermappingid
and prepared id. GetPrepareId API is optional. Also I've removed the
two_phase_commit parameter from postgres_fdw options because we can
disable to use two-phase commit protocol for distributed transactions
using by distributed_atomic_commit GUC parameter.

Foreign transactions whose FDW provides both CommitForeignTransaction
API and RollbackForeignTransaction API will be managed by the global
transaction manager automatically. In addition, if the FDW also
provide PrepareForeignTransaction API it will participate to two-phase
commit protocol as a participant. So the existing FDWs that don't
provide transaction management FDW APIs can continue to work as before
even though this patch get committed.

The one point I'm concerned about this API design would be that since
both CommitForeignTransaction API and RollbackForeignTransaction API
will be used by two different kinds of process (backend and
transaction resolver processes), it might be hard to understand them
correctly for FDW developers.

I'd like to define new APIs so that FDW developers don't get confused.
Feedback is very welcome.

[1] https://wiki.postgresql.org/wiki/Japan_PostgreSQL_Developer_Meetup
[2] https://en.wikipedia.org/wiki/X/Open_XA
[3] The current API design I'm proposing has 6 APIs:  Prepare, Commit,
Rollback, Resolve, IsTwoPhaseEnabled and GetPrepareId. And these APIs
are devided based on who executes it.

Regards,

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