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

On Tue, Oct 20, 2020 at 4:32 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Fri, Oct 16, 2020 at 5:21 PM Peter Smith <smithpb2250@gmail.com> wrote:
> >
>
> Comments:
>
> src/backend/replication/logical/worker.c
> @@ -888,6 +888,319 @@ apply_handle_prepare(StringInfo s)
> + /*
> + * FIXME - Following condition was in apply_handle_prepare_txn except
> I found  it was ALWAYS IsTransactionState() == false
> + * The synchronization worker runs in single transaction. *
> + if (IsTransactionState() && !am_tablesync_worker())
> + */
> + if (!am_tablesync_worker())
>
> Comment: I dont think a tablesync worker will use streaming, none of
> the other stream APIs check this, this might not be relevant for
> stream_prepare either.
>

Yes, I think this is right. See pgoutput_startup where we are
disabling the streaming for init phase. But it is always good to once
test this and ensure the same.

>
> + /*
> + * ==================================================================================================
> + * The following chunk of code is largely cut/paste from the existing
> apply_handle_prepare_commit_txn
>
> Comment: Here, I think you meant apply_handle_stream_commit. Also
> rather than duplicating this chunk of code, you could put it in a new
> function.
>
> + /* open the spool file for the committed transaction */
> + changes_filename(path, MyLogicalRepWorker->subid, xid);
>
> Comment: Here the comment should read "committed/prepared" rather than
> "committed"
>
>
> + else
> + {
> + /* Process any invalidation messages that might have accumulated. */
> + AcceptInvalidationMessages();
> + maybe_reread_subscription();
> + }
>
> Comment: This else block might not be necessary as a tablesync worker
> will not initiate the streaming APIs.
>

I think it is better to have an Assert here for streaming-mode?

> + BeginTransactionBlock();
> + CommitTransactionCommand();
> + StartTransactionCommand();
>
> Comment: Rereading the code and the transaction state description in
> src/backend/access/transam/README. I am not entirely sure if the
> BeginTransactionBlock followed by CommitTransactionBlock is really
> needed here.
>

Yeah, I also find this strange. I guess the patch is doing so because
it needs to call PrepareTransactionBlock later but I am not sure. How
can we call CommitTransactionCommand(), won't it commit the on-going
transaction and make it visible before even it is visible on the
publisher. I think you can verify by having a breakpoint after
CommitTransactionCommand() and see if the changes for which we are
doing prepare become visible.

> I understand this code was copied over from apply_handle_prepare_txn,
> but now looking back I'm not so sure if it is correct. The transaction
> would have already begin as part of applying the changes, why begin it
> again?
> Maybe Amit could confirm this.
>

I hope the above suggestions will help to proceed here.

-- 
With Regards,
Amit Kapila.