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

Hi Ajin.

I've addressed your review comments (details below) and bumped the
patch set to v12 attached.

I also added more test cases.

On Tue, Oct 20, 2020 at 10:02 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:

> Thanks for your patch. Some comments for your patch:
>
> 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.

Updated

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

Updated.

> Also
> rather than duplicating this chunk of code, you could put it in a new
> function.

Code is refactored to share a common function for the spool file processing.

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

Updated

~

Kind Regards,
Peter Smith
Fujitsu Australia