Re: Backup throttling

I realize the following should be applied on the top of v7:

index a0216c1..16dd939 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -1263,7 +1263,7 @@ throttle(size_t increment)       throttling_counter %= throttling_sample;
       /* Once the (possible) sleep has ended, new period starts. */
-       if (wait_result | WL_TIMEOUT)
+       if (wait_result & WL_TIMEOUT)               throttled_last += elapsed + sleep;       else if (sleep > 0)
      /* Sleep was necessary but might have been interrupted. */
 

// Tony

On 01/20/2014 05:10 PM, Antonin Houska wrote:
> On 01/15/2014 10:52 PM, Alvaro Herrera wrote:
>> I gave this patch a look.  There was a bug that the final bounds check
>> for int32 range was not done when there was no suffix, so in effect you
>> could pass numbers larger than UINT_MAX and pg_basebackup would not
>> complain until the number reached the server via BASE_BACKUP.  Maybe
>> that's fine, given that numbers above 1G will cause a failure on the
>> server side anyway, but it looked like a bug to me.  I tweaked the parse
>> routine slightly; other than fix the bug, I made it accept fractional
>> numbers, so you can say 0.5M for instance.
> 
> Thanks.
> 
>> Perhaps we should make sure pg_basebackup rejects numbers larger than 1G
>> as well.
> 
> Is there a good place to define the constant, so that both backend and
> client can use it? I'd say 'include/common' but no existing file seems
> to be appropriate. I'm not sure if it's worth to add a new file there.
> 
>> Another thing I found a bit strange was the use of the latch.  What this
>> patch does is create a separate latch which is used for the throttling.
>> This means that if the walsender process receives a signal, it will not
>> wake up if it's sleeping in throttling.  Perhaps this is okay: as Andres
>> was quoted upthread as saying, maybe this is not a problem because the
>> sleep times are typically short anyway.  But we're pretty much used to
>> the idea that whenever a signal is sent, processes act on it
>> *immediately*.  Maybe some admin will not feel comfortable about waiting
>> some extra 20ms when they cancel their base backups.  In any case,
>> having a secondary latch to sleep on in a process seems weird.  Maybe
>> this should be using MyWalSnd->latch somehow.
> 
> o.k., MyWalSnd->latch is used now.
> 
>> You have this interesting THROTTLING_MAX_FREQUENCY constant defined to
>> 128, with the comment "check this many times per second".
>> Let's see: if the user requests 1MB/s, this value results in
>> throttling_sample = 1MB / 128 = 8192.  So for every 8kB transferred, we
>> would stop, check the wall clock time, and if less time has lapsed than
>> we were supposed to spend transferring those 8kB then we sleep.  Isn't a
>> check every 8kB a bit too frequent?  This doesn't seem sensible to me.
>> I think we should be checking perhaps every tenth of the requested
>> maximum rate, or something like that, not every 1/128th.
>>
>> Now, what the code actually does is not necessarily that, because the
>> sampling value is clamped to a minimum of 32 kB.  But then if we're
>> going to do that, why use such a large divisor value in the first place?
>> I propose we set that constant to a smaller value such as 8.
> 
> I tried to use THROTTLING_SAMPLE_MIN and THROTTLING_MAX_FREQUENCY to
> control both the minimum and maximum chunk size. It was probably too
> generic, THROTTLING_SAMPLE_MIN is no longer there.
> 
> New patch version is attached.
> 
> // Antonin Houska (Tony)
>