Re: [HACKERS] logical decoding of two-phase transactions

On Fri, Mar 19, 2021 at 5:03 AM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> Missed the patch - 0001, resending.
>

I have made miscellaneous changes in the patch which includes
improving comments, error messages, and miscellaneous coding
improvements. The most notable one is that we don't need an additional
parameter in walrcv_startstreaming, if the two_phase option is set
properly. My changes are in v63-0002-Misc-changes-by-Amit, if you are
fine with those, then please merge them in the next version. I have
omitted the dev-logs patch but feel free to submit it. I have one
question:
@@ -538,10 +550,21 @@ CreateDecodingContext(XLogRecPtr start_lsn,
..
+ /* Set two_phase_at LSN only if it hasn't already been set. */
+ if (ctx->twophase && !MyReplicationSlot->data.two_phase_at)
+ {
+ MyReplicationSlot->data.two_phase_at = start_lsn;
+ slot->data.two_phase = true;
+ ReplicationSlotMarkDirty();
+ ReplicationSlotSave();
+ SnapBuildSetTwoPhaseAt(ctx->snapshot_builder, start_lsn);
+ }

What if the walsender or apply worker restarts after setting
two_phase_at/two_phase here and updating the two_phase state in
pg_subscription? Won't we need to set SnapBuildSetTwoPhaseAt after
restart as well? If so, we probably need a else if (ctx->twophase)
{Assert (slot->data.two_phase_at);
SnapBuildSetTwoPhaseAt(ctx->snapshot_builder,
slot->data.two_phase_at);}. Am, I missing something?

--
With Regards,
Amit Kapila.