Re: [COMMITTERS] pgsql: Fix base backup rate limiting in presence of slow i/o

2017-01-01 Thread Magnus Hagander 
On Thu, Dec 22, 2016 at 11:10 AM, Dean Rasheed 
wrote:

> On 21 December 2016 at 20:20, Magnus Hagander  wrote:
> > On Wed, Dec 21, 2016 at 6:55 PM, Dean Rasheed 
> >> 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.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [COMMITTERS] pgsql: Fix base backup rate limiting in presence of slow i/o

2016-12-22 Thread Dean Rasheed 
On 21 December 2016 at 20:20, Magnus Hagander  wrote:
> On Wed, Dec 21, 2016 at 6:55 PM, Dean Rasheed 
>> 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


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Re: [COMMITTERS] pgsql: Fix base backup rate limiting in presence of slow i/o

2016-12-21 Thread Magnus Hagander 
On Wed, Dec 21, 2016 at 6:55 PM, Dean Rasheed 
wrote:

> On 19 December 2016 at 09:17, Magnus Hagander  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?


-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [COMMITTERS] pgsql: Fix base backup rate limiting in presence of slow i/o

2016-12-21 Thread Dean Rasheed 
On 19 December 2016 at 09:17, Magnus Hagander  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


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

[COMMITTERS] pgsql: Fix base backup rate limiting in presence of slow i/o

2016-12-19 Thread Magnus Hagander 
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(-)


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

[COMMITTERS] pgsql: Fix base backup rate limiting in presence of slow i/o

2016-12-19 Thread Magnus Hagander 
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_5_STABLE

Details
---
http://git.postgresql.org/pg/commitdiff/bc53d71308a2b4bd8216932fda3e21cec4915ff8

Modified Files
--
src/backend/replication/basebackup.c | 24 +++-
1 file changed, 7 insertions(+), 17 deletions(-)


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

[COMMITTERS] pgsql: Fix base backup rate limiting in presence of slow i/o

2016-12-19 Thread Magnus Hagander 
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
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/10238fad0389175f71739bc9b4d00bb24d9b8c44

Modified Files
--
src/backend/replication/basebackup.c | 24 +++-
1 file changed, 7 insertions(+), 17 deletions(-)


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

[COMMITTERS] pgsql: Fix base backup rate limiting in presence of slow i/o

2016-12-19 Thread Magnus Hagander 
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_6_STABLE

Details
---
http://git.postgresql.org/pg/commitdiff/1c8ad594e5cb46952bd70f8a0f2f8681912dc223

Modified Files
--
src/backend/replication/basebackup.c | 24 +++-
1 file changed, 7 insertions(+), 17 deletions(-)


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers