Re: Backup throttling

On Fri, Dec 6, 2013 at 6:43 PM, Boszormenyi Zoltan <zb(at)cybertec(dot)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,

--
Fujii Masao