On Wed, 17 Jun 2020 at 14:07, Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> On Wed, 17 Jun 2020 at 09:01, Masahiro Ikeda <ikedamsh@oss.nttdata.com> wrote:
> >
> > > I've attached the new version patch set. 0006 is a separate patch
> > > which introduces 'prefer' mode to foreign_twophase_commit.
> >
> > I hope we can use this feature. Thank you for making patches and
> > discussions.
> > I'm currently understanding the logic and found some minor points to be
> > fixed.
> >
> > I'm sorry if my understanding is wrong.
> >
> > * The v22 patches need rebase as they can't apply to the current master.
> >
> > * FdwXactAtomicCommitParticipants said in
> > src/backend/access/fdwxact/README
> > is not implemented. Is FdwXactParticipants right?
>
> Right.
>
> >
> > * A following comment says that this code is for "One-phase",
> > but second argument of FdwXactParticipantEndTransaction() describes
> > this code is not "onephase".
> >
> > AtEOXact_FdwXact() in fdwxact.c
> > /* One-phase rollback foreign transaction */
> > FdwXactParticipantEndTransaction(fdw_part, false, false);
> >
> > static void
> > FdwXactParticipantEndTransaction(FdwXactParticipant *fdw_part, bool
> > onephase,
> > bool for_commit)
> >
> > * "two_phase_commit" option is mentioned in postgres-fdw.sgml,
> > but I can't find related code.
> >
> > * resolver.c comments have the sentence
> > containing two blanks.(Emergency Termination)
> >
> > * There are some inconsistency with PostgreSQL wiki.
> > https://wiki.postgresql.org/wiki/Atomic_Commit_of_Distributed_Transactions
> >
> > I understand it's difficult to keep consistency, I think it's ok to
> > fix later
> > when these patches almost be able to be committed.
> >
> > - I can't find "two_phase_commit" option in the source code.
> > But 2PC is work if the remote server's "max_prepared_transactions"
> > is set
> > to non zero value. It is correct work, isn't it?
>
> Yes. I had removed two_phase_commit option from postgres_fdw.
> Currently, postgres_fdw uses 2pc when 2pc is required. Therefore,
> max_prepared_transactions needs to be set to more than one, as you
> mentioned.
>
> >
> > - some parameters are renamed or added in latest patches.
> > max_prepared_foreign_transaction, max_prepared_transactions and so
> > on.
> >
> > - typo: froeign_transaction_resolver_timeout
> >
>
> Thank you for your review! I've incorporated your comments on the
> local branch. I'll share the latest version patch.
>
> Also, I've updated the wiki page. I'll try to keep the wiki page up-to-date.
>
I've attached the latest version patches. I've incorporated the review
comments I got so far and improved locking strategy.
Please review it.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services