Re: Backup throttling

On 01/31/2014 06:26 AM, Fujii Masao wrote:
>> Is there a good place to define the constant, so that both backend and
>> client can use it? I'd say 'include/common' but no existing file seems
>> to be appropriate. I'm not sure if it's worth to add a new file there.
>
> If there is no good place to define them, it's okay to define them
> also in client side
> for now.
> +    <term>BASE_BACKUP [<literal>LABEL</literal>
> <replaceable>'label'</replaceable>] [<literal>PROGRESS</literal>]
> [<literal>FAST</literal>] [<literal>WAL</literal>]
> [<literal>NOWAIT</literal>] [<literal>MAX_RATE</literal>]</term>
>
> It's better to add something like <replaceable>'rate'</replaceable> just after
> <literal>MAX_RATE</literal>.
>
> +         <para>
> +          <literal>MAX_RATE</literal> does not affect WAL streaming.
> +         </para>
>
> I don't think that this paragraph is required here because BASE_BACKUP is
> basically independent from WAL streaming.
>
> Why did you choose "bytes per second" as a valid rate which we can specify?
> Since the minimum rate is 32kB, isn't it better to use "KB per second" for that?
> If we do that, we can easily increase the maximum rate from 1GB to very large
> number in the future if required.

The attached version addresses all the comments above.


> +        wait_result = WaitLatch(&MyWalSnd->latch, WL_LATCH_SET | WL_TIMEOUT
> +            | WL_POSTMASTER_DEATH, (long) (sleep / 1000));
>
> If WL_POSTMASTER_DEATH is triggered, we should exit immediately like
> other process does? This is not a problem of this patch. This problem exists
> also in current master. But ISTM it's better to solve that together. Thought?

Once we're careful about not missing signals, I think PM death should be
noticed too. The backup functionality itself would probably manage to
finish without postmaster, however it's executed under walsender process.

Question is where !PostmasterIsAlive() check should be added. I think it
should go to the main loop of perform_base_backup(), but that's probably
not in the scope of this patch.

Do you think that my patch should only add a comment like "Don't forget
to set WL_POSTMASTER_DEATH flag when making basebackup.c sensitive to PM
death?"

// Antonin Houska (Tony)