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

Hello,

The patch needs rebase as it doesn't apply to the current master. I applied
it
to the older commit to test it. It worked fine so far.

I found one bug though which would cause resolver to finish by timeout even
though there are unresolved foreign transactions in the list. The
`fdw_xact_exists()` function expects database id as the first argument and
xid
as the second. But everywhere it is called arguments specified in the
different
order (xid first, then dbid). Also function declaration in header doesn't
match its definition.

There are some other things I found.
* In `FdwXactResolveAllDanglingTransactions()` variable `n_resolved` is
declared as bool but used as integer.
* In fdwxact.c's module comment there are
`FdwXactRegisterForeignTransaction()`
and `FdwXactMarkForeignTransactionModified()` functions mentioned that are
not there anymore.
* In documentation (storage.sgml) there is no mention of `pg_fdw_xact`
directory.

Couple of stylistic notes.
* In `FdwXactCtlData struct` there are both camel case and snake case naming
used.
* In `get_fdw_xacts()` `xid != InvalidTransactionId` can be replaced with
`TransactionIdIsValid(xid)`.
* In `generate_fdw_xact_identifier()` the `fx` prefix could be a part of
format
string instead of being processed by `sprintf` as an extra argument.

I'll continue looking into the patch. Thanks!

On Tue, Nov 20, 2018 at 12:18 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
wrote:

> On Thu, Nov 15, 2018 at 7:36 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
> wrote:
> >
> > On Mon, Oct 29, 2018 at 6:03 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
> wrote:
> > >
> > > On Mon, Oct 29, 2018 at 10:16 AM Masahiko Sawada <
> sawada(dot)mshk(at)gmail(dot)com> wrote:
> > > >
> > > > 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.
> > > >
> > >
> > > 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
>
>