Hi Ajin, Some review comments for patch v12-0001.
====== Commit message 1. Track transactions which have snapshot changes with a new flag RBTXN_HAS_SNAPSHOT_CHANGES ~ The commit message only says *what* it does, but not *why* this patch even exists. TBH, I don't understand why this patch needs to be separated from your patch 0002, because 0001 makes no independent use of the flag, nor is it separately tested. Anyway, if it is going to remain separated then IMO at least the the message should explain the intended purpose e.g. why the subsequent patches require this flagged info and how they will use it. ====== src/include/replication/reorderbuffer.h 2. +/* Does this transaction have snapshot changes? */ +#define rbtxn_has_snapshot_changes(txn) \ +( \ + ((txn)->txn_flags & RBTXN_HAS_SNAPSHOT_CHANGES) != 0 \ +) + Is the below wording maybe a more plain way to say that: /* Does this transaction make changes to the current snapshot? */ ====== Kind Regards, Peter Smith. Fujitsu Australia
