Re: [HACKERS] Transactions involving multiple postgres foreignservers, take 2 - Mailing list pgsql-hackers
From | Masahiko Sawada |
---|---|
Subject | Re: [HACKERS] Transactions involving multiple postgres foreignservers, take 2 |
Date | |
Msg-id | CAD21AoBzLY8Ke-c6vkDL9mYOF=Vequ=UQOkMsf5L_hiwkR7Msg@mail.gmail.com Whole thread Raw |
In response to | Re: [HACKERS] Transactions involving multiple postgres foreignservers, take 2 (Masahiko Sawada <sawada.mshk@gmail.com>) |
Responses |
Re: [HACKERS] Transactions involving multiple postgres foreignservers, take 2
|
List | pgsql-hackers |
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. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Attachment
pgsql-hackers by date: