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

On Tue, Dec 22, 2020 at 8:59 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Dec 22, 2020 at 2:51 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> >
> > On Sat, Dec 19, 2020 at 2:13 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.
regards,
Ajin Cherian
Fujitsu Australia