On Mon, Mar 1, 2021 at 7:23 AM Ajin Cherian <itsa...@gmail.com> wrote: > > On Sat, Feb 27, 2021 at 11:06 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > Few comments on 0002 patch: > > ========================= > > 1. > > + > > + /* > > + * Disable two-phase here, it will be set in the core if it was > > + * enabled whole creating the slot. > > + */ > > + ctx->twophase = false; > > > > Typo, /whole/while. I think we don't need to initialize this variable > > here at all. > > > > 2. > > + /* If twophase is set on the slot at create time, then > > + * make sure the field in the context is also updated > > + */ > > + if (MyReplicationSlot->data.twophase) > > + { > > + ctx->twophase = true; > > + } > > + > > > > For multi-line comments, the first line of comment should be empty. > > Also, I think this is not the right place because the WALSender path > > needs to set it separately. I guess you can set it in > > CreateInitDecodingContext/CreateDecodingContext by doing something > > like > > > > ctx->twophase &= MyReplicationSlot->data.twophase > > Updated accordingly. > > > > > 3. I think we can support this option at the protocol level in a > > separate patch where we need to allow it via replication commands (say > > when we support it in CreateSubscription). Right now, there is nothing > > to test all the code you have added in repl_gram.y. > > > > Removed that. > > > > 4. I think we can expose this new option via pg_replication_slots. > > > > Done. Added, >
v7-0002-Add-option-to-enable-two-phase-commits-in-pg_crea.patch adds twophase to pg_create_logical_replication_slot, I feel this option should be documented in src/sgml/func.sgml. Regards, Vignesh