Re: [HACKERS] logical decoding of two-phase transactions - Mailing list pgsql-hackers

On Mon, Nov 9, 2020 at 1:38 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Nov 9, 2020 at 3:23 PM Peter Smith <smithpb2250@gmail.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?

> ---
> +   if (is_commit)
> +       txn->txn_flags |= RBTXN_COMMIT_PREPARED;
> +   else
> +       txn->txn_flags |= RBTXN_ROLLBACK_PREPARED;
> +
> +   if (rbtxn_commit_prepared(txn))
> +       rb->commit_prepared(rb, txn, commit_lsn);
> +   else if (rbtxn_rollback_prepared(txn))
> +       rb->rollback_prepared(rb, txn, commit_lsn);
>
> RBTXN_COMMIT_PREPARED and RBTXN_ROLLBACK_PREPARED are used only here
> and it seems to me that it's not necessarily necessary.
>

+1.

> ---
> +               /*
> +                * If this is COMMIT_PREPARED and the output plugin supports
> +                * two-phase commits then set the prepared flag to true.
> +                */
> +               prepared = ((info == XLOG_XACT_COMMIT_PREPARED) &&
> ctx->twophase) ? true : false;
>
> We can write instead:
>
> prepared = ((info == XLOG_XACT_COMMIT_PREPARED) && ctx->twophase);
>
>
> +               /*
> +                * If this is ABORT_PREPARED and the output plugin supports
> +                * two-phase commits then set the prepared flag to true.
> +                */
> +               prepared = ((info == XLOG_XACT_ABORT_PREPARED) &&
> ctx->twophase) ? true : false;
>
> The same is true here.
>

+1.

> ---
> '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.

-- 
With Regards,
Amit Kapila.