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

On Mon, Nov 9, 2020 at 3:23 PM Peter Smith <smithpb2250@gmail.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/