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

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?

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