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

On Fri, Jul 7, 2022 at 11:32 AM Shi, Yu/侍 雨 <shiy.fnst@cn.fujitsu.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