On Tue, 18 Feb 2020 at 00:40, Muhammad Usama <m.usama@gmail.com> wrote:
>
> Hi Sawada San,
>
> I have a couple of comments on "v27-0002-Support-atomic-commit-among-multiple-foreign-ser.patch"
>
> 1- As part of the XLogReadRecord refactoring commit the signature of XLogReadRecord was changed,
> so the function call to XLogReadRecord() needs a small adjustment.
>
> i.e. In function XlogReadFdwXactData(XLogRecPtr lsn, char **buf, int *len)
> ...
> - record = XLogReadRecord(xlogreader, lsn, &errormsg);
> + XLogBeginRead(xlogreader, lsn)
> + record = XLogReadRecord(xlogreader, &errormsg);
>
> 2- In register_fdwxact(..) function you are setting the XACT_FLAGS_FDWNOPREPARE transaction flag
> when the register request comes in for foreign server that does not support two-phase commit regardless
> of the value of 'bool modified' argument. And later in the PreCommit_FdwXacts() you just error out when
> "foreign_twophase_commit" is set to 'required' only by looking at the XACT_FLAGS_FDWNOPREPARE flag.
> which I think is not correct.
> Since there is a possibility that the transaction might have only read from the foreign servers (not capable of
> handling transactions or two-phase commit) and all other servers where we require to do atomic commit
> are capable enough of doing so.
> If I am not missing something obvious here, then IMHO the XACT_FLAGS_FDWNOPREPARE flag should only
> be set when the transaction management/two-phase functionality is not available and "modified" argument is
> true in register_fdwxact()
>
Thank you for reviewing this patch!
Your comments are incorporated in the latest patch set I recently sent[1].
[1] https://www.postgresql.org/message-id/CA%2Bfd4k5ZcDvoiY_5c-mF1oDACS5nUWS7ppoiOwjCOnM%2BgrJO-Q%40mail.gmail.com
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
