Hi Ajin. I have re-checked the v13 patches for how my remaining review comments have been addressed.
On Tue, Oct 27, 2020 at 8:55 PM Ajin Cherian <itsa...@gmail.com> wrote: > > > ==================== > > v12-0002. File: src/backend/replication/logical/reorderbuffer.c > > ==================== > > > > COMMENT > > Line 2401 > > /* > > * We are here due to one of the 3 scenarios: > > * 1. As part of streaming in-progress transactions > > * 2. Prepare of a two-phase commit > > * 3. Commit of a transaction. > > * > > * If we are streaming the in-progress transaction then discard the > > * changes that we just streamed, and mark the transactions as > > * streamed (if they contained changes), set prepared flag as false. > > * If part of a prepare of a two-phase commit set the prepared flag > > * as true so that we can discard changes and cleanup tuplecids. > > * Otherwise, remove all the > > * changes and deallocate the ReorderBufferTXN. > > */ > > ~ > > The above comment is beyond my understanding. Anything you could do to > > simplify it would be good. > > > > For example, when viewing this function in isolation I have never > > understood why the streaming flag and rbtxn_prepared(txn) flag are not > > possible to be set at the same time? > > > > Perhaps the code is relying on just internal knowledge of how this > > helper function gets called? And if it is just that, then IMO there > > really should be some Asserts in the code to give more assurance about > > that. (Or maybe use completely different flags to represent those 3 > > scenarios instead of bending the meanings of the existing flags) > > > > Left this for now, probably re-look at this at a later review. > But just to explain; this function is what does the main decoding of > changes of a transaction. > At what point this decoding happens is what this feature and the > streaming in-progress feature is about. As of PG13, this decoding only > happens at commit time. With the streaming of in-progress txn feature, > this began to happen (if streaming enabled) at the time when the > memory limit for decoding transactions was crossed. This 2PC feature > is supporting decoding at the time of a PREPARE transaction. > Now, if streaming is enabled and streaming has started as a result of > crossing the memory threshold, then there is no need to > again begin streaming at a PREPARE transaction as the transaction that > is being prepared has already been streamed. Which is why this > function will not be called when a streaming transaction is prepared > as part of a two-phase commit. AFAIK the last remaining issue now is only about the complexity of the aforementioned code/comment. If you want to defer changing that until we can come up with something better, then that is OK by me. Apart from that I have no other pending review comments at this time. Kind Regards, Peter Smith. Fujitsu Australia