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

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.

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.

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..)

0002:

The name FdwXactParticipantsForAC doesn't sound good for me. How
about FdwXactAtomicCommitPartitcipants?

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.

> 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.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center