Re: Backup throttling

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)