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,
--
Fujii Masao