Antonin Houska escribió:
> Thanks for checking. The new version addresses your findings.
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.
Perhaps we should make sure pg_basebackup rejects numbers larger than 1G
as well.
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.
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.
Other thoughts?
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
