On Fri, Dec 6, 2013 at 6:43 PM, Boszormenyi Zoltan <z...@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

+        The purpose is to limit impact of
+        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.


Fujii Masao

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to