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

Hi Ajin,

On Wed, Dec 23, 2020 at 6:38 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Tue, Dec 22, 2020 at 8:59 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Dec 22, 2020 at 2:51 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > >
> > > On Sat, Dec 19, 2020 at 2:13 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > > Okay, I have changed the rollback_prepare API as discussed above and
> > > > accordingly handle the case where rollback is received without prepare
> > > > in apply_handle_rollback_prepared.
> > >
> > >
> > > I have reviewed and tested your new patchset, I agree with all the
> > > changes that you have made and have tested quite a few scenarios and
> > > they seem to be working as expected.
> > > No major comments but some minor observations:
> > >
> > > Patch 1:
> > > logical.c: 984
> > > Comment should be "rollback prepared" rather than "abort prepared".
> > >
> >
> > Agreed.
>
> Changed.
>
> >
> > > Patch 2:
> > > decode.c: 737: The comments in the header of DecodePrepare seem out of
> > > place, I think here it should describe what the function does rather
> > > than what it does not.
> > >
> >
> > Hmm, I have written it because it is important to explain the theory
> > of concurrent aborts as that is not quite obvious. Also, the
> > functionality is quite similar to DecodeCommit and the comments inside
> > the function explain clearly if there is any difference so not sure
> > what additional we can write, do you have any suggestions?
>
> I have slightly re-worded it. Have a look.
>
> >
> > > reorderbuffer.c: 2422: It looks like pg_indent has mangled the
> > > comments, the numbering is no longer aligned.
> > >
> >
> > Yeah, I had also noticed that but not sure if there is a better
> > alternative because we don't want to change it after each pgindent
> > run. We might want to use (a), (b) .. notation instead but otherwise,
> > there is no big problem with how it is.
>
> Leaving this as is.
>
> >
> > > Patch 5:
> > > worker.c: 753: Type: change "dont" to "don't"
> > >
> >
> > Okay.
>
> Changed.
>
> >
> > > Patch 6: logicaldecoding.sgml
> > > logicaldecoding example is no longer correct. This was true prior to
> > > the changes done to replay prepared transactions after a restart. Now
> > > the whole transaction will get decoded again after the commit
> > > prepared.
> > >
> > > postgres=# COMMIT PREPARED 'test_prepared1';
> > > COMMIT PREPARED
> > > postgres=# select * from
> > > pg_logical_slot_get_changes('regression_slot', NULL, NULL,
> > > 'two-phase-commit', '1');
> > >     lsn    | xid |                    data
> > > -----------+-----+--------------------------------------------
> > >  0/168A060 | 529 | COMMIT PREPARED 'test_prepared1', txid 529
> > > (1 row)
> > >
> >
> > Agreed.
>
> Changed.
>
> >
> > > Patch 8:
> > > worker.c: 2798 :
> > > worker.c: 3445 : disabling two-phase in tablesync worker.
> > >  considering new design of multiple commits in tablesync, do we need
> > > to disable two-phase in tablesync?
> > >
> >
> > No, but let Peter's patch get committed then we can change it.
>
> OK, leaving it.
>
> > Can you please update the patch for the points we agreed upon?
>
> Changed and attached.

Thank you for updating the patches!

I realized that this patch is not registered yet for the next
CommitFest[1] that starts in a couple of days. I found the old entry
of this patch[2] but it's marked as "Returned with feedback". Although
this patch is being reviewed actively, I suggest you adding it before
2021-01-01 AoE[2] so cfbot also can test your patch.

Regards,

[1] https://commitfest.postgresql.org/31/
[2] https://commitfest.postgresql.org/22/944/
[3] https://en.wikipedia.org/wiki/Anywhere_on_Earth

-- 
Masahiko Sawada
EnterpriseDB:  https://www.enterprisedb.com/