Thread: [COMMITTERS] pgsql: Fix base backup rate limiting in presence of slow i/o
[COMMITTERS] pgsql: Fix base backup rate limiting in presence of slow i/o
From
Magnus Hagander
Date:
Fix base backup rate limiting in presence of slow i/o When source i/o on disk was too slow compared to the rate limiting specified, the system could end up with a negative value for sleep that it never got out of, which caused rate limiting to effectively be turned off. Discussion: https://postgr.es/m/CABUevEy_-e0YvL4ayoX8bH_Ja9w%2BBHoP6jUgdxZuG2nEj3uAfQ%40mail.gmail.com Analysis by me, patch by Antonin Houska Branch ------ REL9_4_STABLE Details ------- http://git.postgresql.org/pg/commitdiff/f6508827afe76b2c3735a9ce073620e708d60c79 Modified Files -------------- src/backend/replication/basebackup.c | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-)
Re: [COMMITTERS] pgsql: Fix base backup rate limiting in presence ofslow i/o
From
Dean Rasheed
Date:
On 19 December 2016 at 09:17, Magnus Hagander <magnus@hagander.net> wrote: > Fix base backup rate limiting in presence of slow i/o > > Branch > ------ > REL9_4_STABLE I'm seeing a warning on the 9.4 branch that looks to have been caused by this commit: basebackup.c: In function ‘throttle’: basebackup.c:1284:8: warning: variable ‘wait_result’ set but not used [-Wunused-but-set-variable] int wait_result; Regards, Dean
Re: [COMMITTERS] pgsql: Fix base backup rate limiting in presence ofslow i/o
From
Magnus Hagander
Date:
On Wed, Dec 21, 2016 at 6:55 PM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On 19 December 2016 at 09:17, Magnus Hagander <magnus@hagander.net> wrote:
> Fix base backup rate limiting in presence of slow i/o
>
> Branch
> ------
> REL9_4_STABLE
I'm seeing a warning on the 9.4 branch that looks to have been caused
by this commit:
basebackup.c: In function ‘throttle’:
basebackup.c:1284:8: warning: variable ‘wait_result’ set but not used
[-Wunused-but-set-variable]
int wait_result;
Interesting.
ff44fba4 replaced the latch in walsender, which was not backported (of course).
But it also added a CHECK_FOR_INTERRUPTS there.
9.4 does not have a CHECK_FOR_INTERRUPTS anywhere near there. Perhaps what's really needed is to put one of those in regardless?
Re: [COMMITTERS] pgsql: Fix base backup rate limiting in presence ofslow i/o
From
Dean Rasheed
Date:
On 21 December 2016 at 20:20, Magnus Hagander <magnus@hagander.net> wrote: > On Wed, Dec 21, 2016 at 6:55 PM, Dean Rasheed <dean.a.rasheed@gmail.com> >> basebackup.c: In function ‘throttle’: >> basebackup.c:1284:8: warning: variable ‘wait_result’ set but not used >> [-Wunused-but-set-variable] >> int wait_result; > > Interesting. > > ff44fba4 replaced the latch in walsender, which was not backported (of > course). > > But it also added a CHECK_FOR_INTERRUPTS there. > > 9.4 does not have a CHECK_FOR_INTERRUPTS anywhere near there. Perhaps what's > really needed is to put one of those in regardless? > Yeah, it seems reasonable that there should be a CHECK_FOR_INTERRUPTS() there. I'm not really familiar with that code, but it looks like the signal handler in 9.4 might not set that latch, so throttle() would have to rely on setting ImmediateInterruptOK to make the sleep interruptable, in which case the wait result would be irrelevant, right? Regards, Dean
Re: [COMMITTERS] pgsql: Fix base backup rate limiting in presence ofslow i/o
From
Magnus Hagander
Date:
On Thu, Dec 22, 2016 at 11:10 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On 21 December 2016 at 20:20, Magnus Hagander <magnus@hagander.net> wrote:
> On Wed, Dec 21, 2016 at 6:55 PM, Dean Rasheed <dean.a.rasheed@gmail.com>
>> basebackup.c: In function ‘throttle’:
>> basebackup.c:1284:8: warning: variable ‘wait_result’ set but not used
>> [-Wunused-but-set-variable]
>> int wait_result;
>
> Interesting.
>
> ff44fba4 replaced the latch in walsender, which was not backported (of
> course).
>
> But it also added a CHECK_FOR_INTERRUPTS there.
>
> 9.4 does not have a CHECK_FOR_INTERRUPTS anywhere near there. Perhaps what's
> really needed is to put one of those in regardless?
>
Yeah, it seems reasonable that there should be a CHECK_FOR_INTERRUPTS() there.
I'm not really familiar with that code, but it looks like the signal
handler in 9.4 might not set that latch, so throttle() would have to
rely on setting ImmediateInterruptOK to make the sleep interruptable,
in which case the wait result would be irrelevant, right?
Sorry for the delay in getting back to this. Looking it over again, yes, I think you are right, and will commit a patch that just removes the check for result.