Re: Backup throttling

On Tue, Jan 21, 2014 at 1:10 AM, Antonin Houska
<antonin(dot)houska(at)gmail(dot)com> wrote:
> On 01/15/2014 10:52 PM, Alvaro Herrera wrote:
>> I gave this patch a look. There was a bug that the final bounds check
>> for int32 range was not done when there was no suffix, so in effect you
>> could pass numbers larger than UINT_MAX and pg_basebackup would not
>> complain until the number reached the server via BASE_BACKUP. Maybe
>> that's fine, given that numbers above 1G will cause a failure on the
>> server side anyway, but it looked like a bug to me. I tweaked the parse
>> routine slightly; other than fix the bug, I made it accept fractional
>> numbers, so you can say 0.5M for instance.
>
> Thanks.
>
>> Perhaps we should make sure pg_basebackup rejects numbers larger than 1G
>> as well.
>
> 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.

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

Regards,

--
Fujii Masao