Re: Backup throttling

Thanks for checking. The new version addresses your findings.

// Antonin Houska (Tony)

On 12/09/2013 03:49 PM, Fujii Masao wrote:
> On Fri, Dec 6, 2013 at 6:43 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
>> Hi,
>>
>> 2013-12-05 15:36 keltezéssel, Antonin Houska írta:
>>
>>> On 12/02/2013 02:23 PM, Boszormenyi Zoltan wrote:
>>>>
>>>> Hi,
>>>>
>>>> I am reviewing your patch.
>>>
>>> Thanks. New version attached.
>>
>>
>> I have reviewed and tested it and marked it as ready for committer.
>
> Here are the review comments:
>
> +      <term><option>-r</option></term>
> +      <term><option>--max-rate</option></term>
>
> You need to add something like <replaceable
> class="parameter">rate</replaceable>.
>
> +        The purpose is to limit impact of
> <application>pg_basebackup</application>
> +        on a running master server.
>
> s/"master server"/"server" because we can take a backup from also the standby.
>
> I think that it's better to document the default value and the accepted range of
> the rate that we can specify.
>
> You need to change the protocol.sgml because you changed BASE_BACKUP
> replication command.
>
> +    printf(_("  -r, --max-rate         maximum transfer rate to
> transfer data directory\n"));
>
> You need to add something like =RATE just after --max-rate.
>
> +    result = strtol(src, &after_num, 0);
>
> errno should be set to 0 just before calling strtol().
>
> +    if (errno_copy == ERANGE || result != (uint64) ((uint32) result))
> +    {
> +        fprintf(stderr, _("%s: transfer rate \"%s\" exceeds integer
> range\n"), progname, src);
> +        exit(1);
> +    }
>
> We can move this check after the check of "src == after_num" like
> parse_int() in guc.c does.
> If we do this, the local variable 'errno_copy' is no longer necessary.
>
> I think that it's better to output the hint message like "Valid units for
> the transfer rate are \"k\" and \"M\"." when a user specified wrong unit.
>
> +        /*
> +         * THROTTLING_SAMPLE_MIN / MAX_RATE_LOWER (in seconds) should be the
> +         * longest possible time to sleep. Thus the cast to long is safe.
> +         */
> +        pg_usleep((long) sleep);
>
> It's better to use the latch here so that we can interrupt immediately.
>
> Regards,
>