RE: Perform streaming logical transactions by background workers and parallel apply

On Fri, Jul 7, 2022 at 11:32 AM Shi, Yu/侍 雨 <shiy(dot)fnst(at)cn(dot)fujitsu(dot)com> wrote:
> Thanks for updating the patch.
>
> Here are some comments.

Thanks for your comments.

> 0001 patch
> ==============
> 1.
> + /* Check If there are free worker slot(s) */
> + LWLockAcquire(LogicalRepWorkerLock, LW_SHARED);
>
> I think "Check If" should be "Check if".

Fixed.

> 0003 patch
> ==============
> 1.
> Should we call apply_bgworker_relation_check() in apply_handle_truncate()?

Because TRUNCATE blocks all other operations on the table, I think that when
two transactions on the publisher-side operate on the same table, at least one
of them will be blocked. So I think for this case the blocking will happen on
the publisher-side.

> 0004 patch
> ==============
> 1.
> @@ -3932,6 +3958,9 @@ start_apply(XLogRecPtr origin_startpos)
> }
> PG_CATCH();
> {
> + /* Set the flag that we will retry later. */
> + set_subscription_retry(true);
> +
> if (MySubscription->disableonerr)
> DisableSubscriptionAndExit();
> Else
>
> I think we need to emit the error and recover from the error state before
> setting the retry flag, like what we do in DisableSubscriptionAndExit().
> Otherwise if an error is detected when setting the retry flag, we won't get the
> error message reported by the apply worker.

You are right.
I fixed this point as you suggested. (I moved the operations you mentioned from
the function DisableSubscriptionAndExit to before setting the retry flag.)
I also made a similar modification in the function start_table_sync.

Attach the news patches.

Regards,
Wang wei