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

On Mon, Nov 9, 2020 at 3:23 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> > > 4.
> > > +static void
> > > +apply_handle_prepare_txn(LogicalRepPrepareData * prepare_data)
> > > +{
> > > + Assert(prepare_data->prepare_lsn == remote_final_lsn);
> > > +
> > > + /* The synchronization worker runs in single transaction. */
> > > + if (IsTransactionState() && !am_tablesync_worker())
> > > + {
> > > + /* End the earlier transaction and start a new one */
> > > + BeginTransactionBlock();
> > > + CommitTransactionCommand();
> > > + StartTransactionCommand();
> > >
> > > There is no explanation as to why you want to end the previous
> > > transaction and start a new one. Even if we have to do so, we first
> > > need to call BeginTransactionBlock before CommitTransactionCommand.
>
> Done
>
> ---
>
> Also...
>
> pgindent has been run for all patches now.
>
> The latest of all six patches are again reunited with a common v18
> version number.
>

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?

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

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

---
'git show --check' of v18-0002 reports some warnings.

Regards,

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