Horton

Thread

Commits

  1. Avoid creating parallel apply state hash table unless required.

  1. releasing ParallelApplyTxnHash when pa_launch_parallel_worker returns NULL

    Ted Yu <yuzhihong@gmail.com> — 2023-01-10T17:25:29Z

    Hi,
I was reading src/backend/replication/logical/applyparallelworker.c .
In `pa_allocate_worker`, when pa_launch_parallel_worker returns NULL, I
think the `ParallelApplyTxnHash` should be released.

Please see the patch.

Thanks

  2. Re: releasing ParallelApplyTxnHash when pa_launch_parallel_worker returns NULL

    Ted Yu <yuzhihong@gmail.com> — 2023-01-10T17:26:17Z

    On Tue, Jan 10, 2023 at 9:25 AM Ted Yu <yuzhihong@gmail.com> wrote:

> Hi,
> I was reading src/backend/replication/logical/applyparallelworker.c .
> In `pa_allocate_worker`, when pa_launch_parallel_worker returns NULL, I
> think the `ParallelApplyTxnHash` should be released.
>
> Please see the patch.
>
> Thanks
>
Here is the patch :-)

  3. Re: releasing ParallelApplyTxnHash when pa_launch_parallel_worker returns NULL

    Ted Yu <yuzhihong@gmail.com> — 2023-01-10T17:43:25Z

    On Tue, Jan 10, 2023 at 9:26 AM Ted Yu <yuzhihong@gmail.com> wrote:

>
>
> On Tue, Jan 10, 2023 at 9:25 AM Ted Yu <yuzhihong@gmail.com> wrote:
>
>> Hi,
>> I was reading src/backend/replication/logical/applyparallelworker.c .
>> In `pa_allocate_worker`, when pa_launch_parallel_worker returns NULL, I
>> think the `ParallelApplyTxnHash` should be released.
>>
>> Please see the patch.
>>
>> Thanks
>>
> Here is the patch :-)
>

In `pa_process_spooled_messages_if_required`, the `pa_unlock_stream` call
immediately follows `pa_lock_stream`.
I assume the following is the intended sequence of calls. If this is the
case, I can add it to the patch.

Cheers

diff --git a/src/backend/replication/logical/applyparallelworker.c
b/src/backend/replication/logical/applyparallelworker.c
index 2e5914d5d9..9879b3fff2 100644
--- a/src/backend/replication/logical/applyparallelworker.c
+++ b/src/backend/replication/logical/applyparallelworker.c
@@ -684,9 +684,9 @@ pa_process_spooled_messages_if_required(void)
     if (fileset_state == FS_SERIALIZE_IN_PROGRESS)
     {
         pa_lock_stream(MyParallelShared->xid, AccessShareLock);
-        pa_unlock_stream(MyParallelShared->xid, AccessShareLock);

         fileset_state = pa_get_fileset_state();
+        pa_unlock_stream(MyParallelShared->xid, AccessShareLock);
     }

     /*

  4. Re: releasing ParallelApplyTxnHash when pa_launch_parallel_worker returns NULL

    Ted Yu <yuzhihong@gmail.com> — 2023-01-10T18:37:45Z

    On Tue, Jan 10, 2023 at 9:43 AM Ted Yu <yuzhihong@gmail.com> wrote:

>
>
> On Tue, Jan 10, 2023 at 9:26 AM Ted Yu <yuzhihong@gmail.com> wrote:
>
>>
>>
>> On Tue, Jan 10, 2023 at 9:25 AM Ted Yu <yuzhihong@gmail.com> wrote:
>>
>>> Hi,
>>> I was reading src/backend/replication/logical/applyparallelworker.c .
>>> In `pa_allocate_worker`, when pa_launch_parallel_worker returns NULL, I
>>> think the `ParallelApplyTxnHash` should be released.
>>>
>>> Please see the patch.
>>>
>>> Thanks
>>>
>> Here is the patch :-)
>>
>
> In `pa_process_spooled_messages_if_required`, the `pa_unlock_stream` call
> immediately follows `pa_lock_stream`.
> I assume the following is the intended sequence of calls. If this is the
> case, I can add it to the patch.
>
> Cheers
>
> diff --git a/src/backend/replication/logical/applyparallelworker.c
> b/src/backend/replication/logical/applyparallelworker.c
> index 2e5914d5d9..9879b3fff2 100644
> --- a/src/backend/replication/logical/applyparallelworker.c
> +++ b/src/backend/replication/logical/applyparallelworker.c
> @@ -684,9 +684,9 @@ pa_process_spooled_messages_if_required(void)
>      if (fileset_state == FS_SERIALIZE_IN_PROGRESS)
>      {
>          pa_lock_stream(MyParallelShared->xid, AccessShareLock);
> -        pa_unlock_stream(MyParallelShared->xid, AccessShareLock);
>
>          fileset_state = pa_get_fileset_state();
> +        pa_unlock_stream(MyParallelShared->xid, AccessShareLock);
>      }
>
>      /*
>
Looking closer at the comment above this code and other part of the file,
it seems the order is intentional.

Please disregard my email about `pa_process_spooled_messages_if_required`.

  5. RE: releasing ParallelApplyTxnHash when pa_launch_parallel_worker returns NULL

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2023-01-11T02:12:40Z

    On Wednesday, January 11, 2023 1:25 AM Ted Yu <yuzhihong@gmail.com> wrote:

> I was reading src/backend/replication/logical/applyparallelworker.c .
> In `pa_allocate_worker`, when pa_launch_parallel_worker returns NULL, I think the `ParallelApplyTxnHash` should be released.

Thanks for reporting.

ParallelApplyTxnHash is used to cache the state of streaming transactions being
applied. There could be multiple streaming transactions being applied in
parallel and their information were already saved in ParallelApplyTxnHash, so
we should not release them just because we don't have a worker available to
handle a new transaction here.

Best Regards,
Hou zj

  6. Re: releasing ParallelApplyTxnHash when pa_launch_parallel_worker returns NULL

    Ted Yu <yuzhihong@gmail.com> — 2023-01-11T02:20:54Z

    On Tue, Jan 10, 2023 at 6:13 PM houzj.fnst@fujitsu.com <
houzj.fnst@fujitsu.com> wrote:

> On Wednesday, January 11, 2023 1:25 AM Ted Yu <yuzhihong@gmail.com> wrote:
>
> > I was reading src/backend/replication/logical/applyparallelworker.c .
> > In `pa_allocate_worker`, when pa_launch_parallel_worker returns NULL, I
> think the `ParallelApplyTxnHash` should be released.
>
> Thanks for reporting.
>
> ParallelApplyTxnHash is used to cache the state of streaming transactions
> being
> applied. There could be multiple streaming transactions being applied in
> parallel and their information were already saved in ParallelApplyTxnHash,
> so
> we should not release them just because we don't have a worker available to
> handle a new transaction here.
>
> Best Regards,
> Hou zj
>
Hi,

        /* First time through, initialize parallel apply worker state
hashtable. */
        if (!ParallelApplyTxnHash)

I think it would be better if `ParallelApplyTxnHash` is created by the
first successful parallel apply worker.

Please take a look at the new patch and see if it makes sense.

Cheers

  7. RE: releasing ParallelApplyTxnHash when pa_launch_parallel_worker returns NULL

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2023-01-11T03:55:25Z

    On Wednesday, January 11, 2023 10:21 AM Ted Yu <yuzhihong@gmail.com> wrote:
>         /* First time through, initialize parallel apply worker state hashtable. */
>         if (!ParallelApplyTxnHash)
> 
> I think it would be better if `ParallelApplyTxnHash` is created by the first
> successful parallel apply worker.

Thanks for the suggestion. But I am not sure if it's worth to changing the
order here, because It will only optimize the case where user enable parallel
apply but never get an available worker which should be rare. And in such a
case, it'd be better to increase the number of workers or disable the parallel mode.

Best Regards,
Hou zj

  8. Re: releasing ParallelApplyTxnHash when pa_launch_parallel_worker returns NULL

    Ted Yu <yuzhihong@gmail.com> — 2023-01-11T04:01:15Z

    On Tue, Jan 10, 2023 at 7:55 PM houzj.fnst@fujitsu.com <
houzj.fnst@fujitsu.com> wrote:

> On Wednesday, January 11, 2023 10:21 AM Ted Yu <yuzhihong@gmail.com>
> wrote:
> >         /* First time through, initialize parallel apply worker state
> hashtable. */
> >         if (!ParallelApplyTxnHash)
> >
> > I think it would be better if `ParallelApplyTxnHash` is created by the
> first
> > successful parallel apply worker.
>
> Thanks for the suggestion. But I am not sure if it's worth to changing the
> order here, because It will only optimize the case where user enable
> parallel
> apply but never get an available worker which should be rare. And in such a
> case, it'd be better to increase the number of workers or disable the
> parallel mode.
>
> Best Regards,
> Hou zj
>

I think even though the chance is rare, we shouldn't leak resource.

The `ParallelApplyTxnHash` shouldn't be created if there is no single apply
worker.

  9. Re: releasing ParallelApplyTxnHash when pa_launch_parallel_worker returns NULL

    Amit Kapila <amit.kapila16@gmail.com> — 2023-01-12T02:54:07Z

    On Wed, Jan 11, 2023 at 9:31 AM Ted Yu <yuzhihong@gmail.com> wrote:
>
> On Tue, Jan 10, 2023 at 7:55 PM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote:
>>
>> On Wednesday, January 11, 2023 10:21 AM Ted Yu <yuzhihong@gmail.com> wrote:
>> >         /* First time through, initialize parallel apply worker state hashtable. */
>> >         if (!ParallelApplyTxnHash)
>> >
>> > I think it would be better if `ParallelApplyTxnHash` is created by the first
>> > successful parallel apply worker.
>>
>> Thanks for the suggestion. But I am not sure if it's worth to changing the
>> order here, because It will only optimize the case where user enable parallel
>> apply but never get an available worker which should be rare. And in such a
>> case, it'd be better to increase the number of workers or disable the parallel mode.
>>
>
>
> I think even though the chance is rare, we shouldn't leak resource.
>

But that is true iff we are never able to start the worker. Anyway, I
think it is probably fine either way but we can change it as per your
suggestion to make it more robust and probably for the code clarity
sake. I'll push this tomorrow unless someone thinks otherwise.

-- 
With Regards,
Amit Kapila.

  10. Re: releasing ParallelApplyTxnHash when pa_launch_parallel_worker returns NULL

    Ted Yu <yuzhihong@gmail.com> — 2023-01-12T02:55:18Z

    On Wed, Jan 11, 2023 at 6:54 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

> On Wed, Jan 11, 2023 at 9:31 AM Ted Yu <yuzhihong@gmail.com> wrote:
> >
> > On Tue, Jan 10, 2023 at 7:55 PM houzj.fnst@fujitsu.com <
> houzj.fnst@fujitsu.com> wrote:
> >>
> >> On Wednesday, January 11, 2023 10:21 AM Ted Yu <yuzhihong@gmail.com>
> wrote:
> >> >         /* First time through, initialize parallel apply worker state
> hashtable. */
> >> >         if (!ParallelApplyTxnHash)
> >> >
> >> > I think it would be better if `ParallelApplyTxnHash` is created by
> the first
> >> > successful parallel apply worker.
> >>
> >> Thanks for the suggestion. But I am not sure if it's worth to changing
> the
> >> order here, because It will only optimize the case where user enable
> parallel
> >> apply but never get an available worker which should be rare. And in
> such a
> >> case, it'd be better to increase the number of workers or disable the
> parallel mode.
> >>
> >
> >
> > I think even though the chance is rare, we shouldn't leak resource.
> >
>
> But that is true iff we are never able to start the worker. Anyway, I
> think it is probably fine either way but we can change it as per your
> suggestion to make it more robust and probably for the code clarity
> sake. I'll push this tomorrow unless someone thinks otherwise.
>
> --
> With Regards,
> Amit Kapila.
>

Thanks Amit for the confirmation.

Cheers

  11. Re: releasing ParallelApplyTxnHash when pa_launch_parallel_worker returns NULL

    Amit Kapila <amit.kapila16@gmail.com> — 2023-01-13T09:44:12Z

    On Thu, Jan 12, 2023 at 8:25 AM Ted Yu <yuzhihong@gmail.com> wrote:

> On Wed, Jan 11, 2023 at 6:54 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Wed, Jan 11, 2023 at 9:31 AM Ted Yu <yuzhihong@gmail.com> wrote:
>> >
>> > On Tue, Jan 10, 2023 at 7:55 PM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote:
>> >>
>> >> On Wednesday, January 11, 2023 10:21 AM Ted Yu <yuzhihong@gmail.com> wrote:
>> >> >         /* First time through, initialize parallel apply worker state hashtable. */
>> >> >         if (!ParallelApplyTxnHash)
>> >> >
>> >> > I think it would be better if `ParallelApplyTxnHash` is created by the first
>> >> > successful parallel apply worker.
>> >>
>> >> Thanks for the suggestion. But I am not sure if it's worth to changing the
>> >> order here, because It will only optimize the case where user enable parallel
>> >> apply but never get an available worker which should be rare. And in such a
>> >> case, it'd be better to increase the number of workers or disable the parallel mode.
>> >>
>> >
>> >
>> > I think even though the chance is rare, we shouldn't leak resource.
>> >
>>
>> But that is true iff we are never able to start the worker. Anyway, I
>> think it is probably fine either way but we can change it as per your
>> suggestion to make it more robust and probably for the code clarity
>> sake. I'll push this tomorrow unless someone thinks otherwise.
>>

Pushed.

-- 
With Regards,
Amit Kapila.