From: | Boszormenyi Zoltan <zb(at)cybertec(dot)at> |
---|---|
To: | Antonin Houska <antonin(dot)houska(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Backup throttling |
Date: | 2013-12-02 13:23:11 |
Message-ID: | 529C89BF.3000702@cybertec.at |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
I am reviewing your patch.
2013-10-10 15:32 keltezéssel, Antonin Houska írta:
> On 10/09/2013 08:56 PM, Robert Haas wrote:
>> There seem to be several review comments made since you posted this
>> version. I'll mark this Waiting on Author in the CommitFest
>> application, since it seems that the patch needs to be further
>> updated.
> Since it was waiting for reviewer, I was not sure whether I should wait
> for his findings and fix everything in a single transaction.
> Nevertheless, the next version is attached here.
>
> It fixes findings reported in
> http://www.postgresql.org/message-id/20130903165652.GC5227@eldon.alvh.no-ip.org
>
> As for units
> http://www.postgresql.org/message-id/20130903224127.GD7018@awork2.anarazel.de
> I'm not convinced about "MB" at the moment. Unfortunately I couldn't
> find any other command-line PG utility requiring amount of data as an
> option. Thus I use single-letter suffix, just as wget does for the same
> purposes.
>
> As for this
> http://www.postgresql.org/message-id/20130903125155.GA18486@awork2.anarazel.de
> there eventually seems to be a consensus (I notice now the discussion
> was off-list):
>
>> On 2013-09-03 23:21:57 +0200, Antonin Houska wrote:
>>> On 09/03/2013 02:51 PM, Andres Freund wrote:
>>>
>>>> It's probably better to use latches for the waiting, those have properly
>>>> defined interruption semantics. Whether pg_usleep will be interrupted is
>>>> platform dependant...
>>> I noticed a comment about interruptions around the definition of
>>> pg_usleep() function, but concluded that the sleeps are rather frequent
>>> in this applications (typically in the order of tens to hundreds per
>>> second, although the maximum of 256 might need to be decreased).
>>> Therefore occasional interruptions shouldn't distort the actual rate
>>> much. I'll think about it again. Thanks,
>> The issue is rather that you might not get woken up when you want to
>> be. Signal handlers in postgres tend to do a
>> SetLatch(&MyProc->procLatch); which then will interrupt sleeps done via
>> WaitLatch(). It's probably not that important with the duration of the
>> sleeps you have.
>>
>> Greetings,
>>
>> Andres Freund
> // Antonin Houska (Tony)
* Is the patch in a patch format which has context?
Yes
* Does it apply cleanly to the current git master?
It applies with some offsets. Version "3a" that applies cleanly is attached.
* Does it include reasonable tests, necessary doc patches, etc?
Docs: yes. Tests: N/A
* Does the patch actually implement what it advertises?
Yes.
* Do we want that?
Yes.
* Do we already have it?
No.
* Does it follow SQL spec, or the community-agreed behavior?
No such SQL spec. The latest patch fixed all previously raised comments.
* Does it include pg_dump support (if applicable)?
N/A
* Are there dangers?
Yes, the "danger" of slowing down taking a base backup.
But this is the point.
* Have all the bases been covered?
Yes.
* Does the feature work as advertised?
Yes.
* Are there corner cases the author has failed to consider?
No.
* Are there any assertion failures or crashes?
No.
* Does the patch slow down simple tests?
No.
* If it claims to improve performance, does it?
N/A
* Does it slow down other things?
No.
* 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;
+
* Are there portability issues?
No.
* Will it work on Windows/BSD etc?
It should, there are no dangerous calls besides the above mentioned
pg_usleep(). But waking up from pg_usleep() earlier makes rate limiting
fluctuate slightly, not fail.
* Are the comments sufficient and accurate?
Yes.
* 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.
* Does it produce compiler warnings?
No.
*Can you make it crash?
No.
Consider the changes to the code in the context of the project as a whole:
* Is everything done in a way that fits together coherently with other features/modules?
Yes.
* Are there interdependencies that can cause problems?
No.
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.
*/
Best regards,
Zoltán Böszörményi
--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/
Attachment | Content-Type | Size |
---|---|---|
backup_throttling_v3a.patch | text/x-patch | 14.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Dimitri Fontaine | 2013-12-02 13:38:38 | Re: Extension Templates S03E11 |
Previous Message | MauMau | 2013-12-02 13:22:47 | Re: DATE type output does not follow datestyle parameter |