From: | Antonin Houska <antonin(dot)houska(at)gmail(dot)com> |
---|---|
To: | Boszormenyi Zoltan <zb(at)cybertec(dot)at>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Backup throttling |
Date: | 2013-12-05 14:36:50 |
Message-ID: | 52A08F82.7020501@gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
backup_throttling_v4.patch | text/x-patch | 14.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Eisentraut | 2013-12-05 14:39:41 | Re: More legacy code: pg_ctl |
Previous Message | Tom Lane | 2013-12-05 14:24:12 | Re: Regression tests failing if not launched on db "regression" |