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

On Mon, Nov 9, 2020 at 8:21 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Nov 9, 2020 at 1:38 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Mon, Nov 9, 2020 at 3:23 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > >
> >
> > I've looked at the patches and done some tests. Here is my comment and
> > question I realized during testing and reviewing.
> >
> > +static void
> > +DecodePrepare(LogicalDecodingContext *ctx, XLogRecordBuffer *buf,
> > + xl_xact_parsed_prepare *parsed)
> > +{
> > + XLogRecPtr origin_lsn = parsed->origin_lsn;
> > + TimestampTz commit_time = parsed->origin_timestamp;
> >
> > static void
> > DecodeAbort(LogicalDecodingContext *ctx, XLogRecordBuffer *buf,
> > - xl_xact_parsed_abort *parsed, TransactionId xid)
> > + xl_xact_parsed_abort *parsed, TransactionId xid, bool prepared)
> > {
> > int i;
> > + XLogRecPtr origin_lsn = InvalidXLogRecPtr;
> > + TimestampTz commit_time = 0;
> > + XLogRecPtr origin_id = XLogRecGetOrigin(buf->record);
> >
> > - for (i = 0; i < parsed->nsubxacts; i++)
> > + if (parsed->xinfo & XACT_XINFO_HAS_ORIGIN)
> > {
> > - ReorderBufferAbort(ctx->reorder, parsed->subxacts[i],
> > - buf->record->EndRecPtr);
> > + origin_lsn = parsed->origin_lsn;
> > + commit_time = parsed->origin_timestamp;
> > }
> >
> > In the above two changes, parsed->origin_timestamp is used as
> > commit_time. But in DecodeCommit() we use parsed->xact_time instead.
> > Therefore it a transaction didn't have replorigin_session_origin the
> > timestamp of logical decoding out generated by test_decoding with
> > 'include-timestamp' option is invalid. Is it intentional?
> >
>
> I think all three DecodePrepare/DecodeAbort/DecodeCommit should have
> same handling for this with the exception that at DecodePrepare time
> we can't rely on XACT_XINFO_HAS_ORIGIN but instead we need to check if
> origin_timestamp is non-zero then we will overwrite commit_time with
> it. Does that make sense to you?

Yeah, that makes sense to me.

> > 'git show --check' of v18-0002 reports some warnings.
> >
>
> I have also noticed this. Actually, I have already started making some
> changes to these patches apart from what you have reported so I'll
> take care of these things as well.

Ok.

Regards,

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