On Sat, Mar 6, 2021 at 7:19 AM Peter Smith <smithpb2...@gmail.com> wrote: > > Please find attached the latest patch set v50* >
Few comments on the latest patch series: ================================= 1. I think we can extract the changes to make streaming optional with 2PC and infact you can start a separate thread for it. 2. I think we can get rid of table-sync early exit patch (v50-0007-Tablesync-early-exit) as we have kept two_phase off from tablesync worker. I agree that has its own independent value but it is not required for this patch series. 3. Now, that we are not supporting streaming with two_pc option, do we really need the first patch (v50-0001-Refactor-spool-file-logic-in-worker.c)? I suggest to get rid of the same unless it is really required. If we decide to remove this patch, then remove the reference to apply_spooled_messages from 0008 patch. v50-0005-Support-2PC-txn-subscriber-tests 4. +############################### +# Test cases involving DDL. +############################### + +# TODO This can be added after we add functionality to replicate DDL changes to subscriber. We can remove this from the patch. v50-0006-Support-2PC-txn-Subscription-option 5. - /* Binary mode and streaming are only supported in v14 and higher */ + /* Binary mode and streaming and Two phase commit are only supported in v14 and higher */ It looks odd that only one of the option starts with capital letter /Two/two. I suggest to two_phase. v50-0008-Fix-apply-worker-empty-prepare 6. In 0008, the commit message lines are too long, it is difficult to read those. Try to keep them 75 char long, this is generally what I use but you can try something else if you want but not as long as you have kept in this patch. 7. + /* + * A Problem: + * .. Let's call this the "empty prepare" problem. + * + * The following code has a 2-part fix for that scenario. No need to describe it in terms of problem and fix. You can say something like: "This can lead to "empty prepare". We avoid this by ...." -- With Regards, Amit Kapila.
