On Tue, 18 Feb 2020 at 00:40, Muhammad Usama <m.us...@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
