On 12/02/2013 02:23 PM, Boszormenyi Zoltan wrote:
> Hi,
>
> I am reviewing your patch.
Thanks. New version attached.
>
> * Does it follow the project coding guidelines?
>
> Yes. A nitpicking: this else branch below might need brackets
> because there is also a comment in that branch:
>
> + /* The 'real data' starts now (header was ignored). */
> + throttled_last = GetCurrentIntegerTimestamp();
> + }
> + else
> + /* Disable throttling. */
> + throttling_counter = -1;
> +
Done.
>
> * Does it do what it says, correctly?
>
> Yes.
>
> Although it should be mentioned in the docs that rate limiting
> applies to walsenders individually, not globally. I tried this
> on a freshly created database:
>
> $ time pg_basebackup -D data2 -r 1M -X stream -h 127.0.0.1
> real 0m26.508s
> user 0m0.054s
> sys 0m0.360s
>
> The source database had 3 WAL files in pg_xlog, one of them was
> also streamed. The final size of "data2" was 43MB or 26MB without pg_xlog,
> i.e. without the "-X stream" option. The backup used 2 walsenders
> in parallel (one for WAL) which is a known feature.
Right, if the method is 'stream', a separate WAL sender is used and the
transfer is not limited: client must keep up with the stream
unconditionally. I added a note to documentation.
But there's no reason not to throttle WAL files if the method is
'fetch'. It's fixed now.
> Another note. This chunk should be submitted separately as a comment bugfix:
>
> diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
> index c3c71b7..5736fd8 100644
> --- a/src/backend/utils/adt/timestamp.c
> +++ b/src/backend/utils/adt/timestamp.c
> @@ -1288,7 +1288,7 @@ GetCurrentTimestamp(void)
> /*
> * GetCurrentIntegerTimestamp -- get the current operating system time as int64
> *
> - * Result is the number of milliseconds since the Postgres epoch. If compiled
> + * Result is the number of microseconds since the Postgres epoch. If compiled
> * with --enable-integer-datetimes, this is identical to GetCurrentTimestamp(),
> * and is implemented as a macro.
> */
Will do.
// Tony
