Re: repeated decoding of prepared transactions

On Sat, Feb 27, 2021 at 11:38 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, Feb 26, 2021 at 4:13 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> >
> > On Fri, Feb 26, 2021 at 7:47 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> >
> > > I've updated snapshot_was_exported_at_ member to pg_replication_slots as well.
> > > Do have a look and let me know if there are any comments.
> >
> > Update with both patches.
> >
>
> Thanks, I have made some minor changes to the first patch and now it
> looks good to me. The changes are as below:
> 1. Removed the changes related to exposing this new parameter via view
> as mentioned in my previous email.
> 2. Changed the variable name initial_consistent_point.
> 3. Ran pgindent, minor changes in comments, and modified the commit message.
>
> Let me know what you think about these changes.
>

In the attached, I have just bumped SNAPBUILD_VERSION as we are
adding a new member in the SnapBuild structure.

> Next, I'll review your second patch which allows the 2PC option to be
> enabled only at slot creation time.
>

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

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.

4. I think we can expose this new option via pg_replication_slots.

--
With Regards,
Amit Kapila.