On Thu, Dec 28, 2017 at 11:40 AM, Tsunakawa, Takayuki <tsunakawa.ta...@jp.fujitsu.com> wrote: > From: Masahiko Sawada [mailto:sawada.m...@gmail.com] >> I've updated documentation of patches, and fixed some bugs. I did some >> failure tests of this feature using a fault simulation tool[1] for >> PostgreSQL that I created. >> >> 0001 patch adds a mechanism to track of writes on local server. This is >> required to determine whether we should use 2pc at commit. 0002 patch is >> the main part. It adds a distributed transaction manager (currently only >> for atomic commit), APIs for 2pc and foreign transaction manager resolver >> process. 0003 patch makes postgres_fdw support atomic commit using 2pc. >> >> Please review patches. > > I'd like to join the review and testing of this functionality. First, some > comments after taking a quick look at 0001:
Thank you so much! > (1) > Why don't you use the existing global variable MyXactFlags instead of the new > TransactionDidWrite? Or, how about using XactLastRecEnd != 0 to determine > the transaction did any writes? When the transaction only modified temporary > tables on the local database and some data on one remote database, I think > 2pc is unnecessary. Perhaps we can use (XactLastRecEnd != 0 && markXidCommitted) to see if we did any writes on local node which requires the atomic commit. Will fix. > > (2) > If TransactionDidWrite is necessary, I don't think you need to provide setter > functions, because other transaction state variables are accessed globally > without getter/setters. And you didn't create getter function for > TransactionDidWrite. > > (3) > heap_multi_insert() doesn't modify TransactionDidWrite. Is it sufficient to > just remember heap modifications? Are other modifications on the coordinator > node covered such as TRUNCATEand and REINDEX? I think the using (XactLastRecEnd != 0 && markXidCommitted) to check if we did any writes on local node would be better. After changed I will be able to deal with the all above concerns. > > > Questions before looking at 0002 and 0003: > > Q1: Does this functionality work when combined with XA 2pc transactions? All transaction including local transaction and foreign transactions are prepared when PREPARE. And all transactions are committed/rollbacked by the foreign transaction resolver process when COMMIT/ROLLBACK PREPARED. > Q2: Does the atomic commit cascade across multiple remote databases? For > example: > * The local transaction modifies data on remote database 1 via a foreign > table. > * A trigger fires on the remote database 1, which modifies data on remote > database 2 via a foreign table. > * The local transaction commits. I've not tested yet more complex failure situations but as far as I tested on my environment, the cascading atomic commit works. I'll test these cases more deeply. Regards, Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
