Re: Backup throttling

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