Re: Backup throttling

Hello,
the purpose of this patch is to limit impact of pg_backup on running
server. Feedback is appreciated.

// Antonin Houska (Tony)

On Wed, Jul 24, 2013 at 4:20 PM, Antonin Houska
<antonin(dot)houska(at)gmail(dot)com> wrote:
> Hello,
> the purpose of this patch is to limit impact of pg_backup on running server.
> Feedback is appreciated.

Interesting. Please add this patch to the next commitfeat.
https://commitfest.postgresql.org/action/commitfest_view?id=19

Regards,

--
Fujii Masao

On Wed, 24 Jul 2013 09:20:52 +0200
Antonin Houska <antonin(dot)houska(at)gmail(dot)com> wrote:

> Hello,
> the purpose of this patch is to limit impact of pg_backup on running
> server. Feedback is appreciated.
>
> // Antonin Houska (Tony)

Hi,

That is a really nice feature. I took a first look at your patch and
some empty lines you added (e.g. line 60 your patch). Can you remove
them?

Why did you move localGetCurrentTimestamp() into streamutil.c? Is
sys/time.h still needed in receivelog.c after the move?

I will try your patch later today to see, if it works.

regards,

Stefan Radomski

Gibheer escribió:

> Why did you move localGetCurrentTimestamp() into streamutil.c? Is
> sys/time.h still needed in receivelog.c after the move?

I think we should have GetCurrentTimestamp() in src/common; there are
way too many copies of that functionality now. I looked into this
awhile back, but eviscerating the datetime.c/timestamp.c code out of the
backend proved much messier than I anticipated.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 07/31/2013 07:13 AM, Gibheer wrote:
> Hi,
>
> That is a really nice feature.
I don't pretend it's my idea, I just coded it. My boss proposed the
feature as such :-)
> I took a first look at your patch and some empty lines you added (e.g. line 60 your patch). Can you remove
> them?
Sure, will do in the next version.
> Why did you move localGetCurrentTimestamp() into streamutil.c? Is sys/time.h still needed in receivelog.c after the move?
Because both receivelog.c and pg_basebackup.c need it now. I thought I
could move localTimestampDifference() and
localTimestampDifferenceExceeds() as well for the sake of consistency
(these are actually utilities too) but I didn't get convinced enough
that the feature alone justifies such a change.

As mentioned in
http://www.postgresql.org/message-id/20130731173624.GX14652@eldon.alvh.no-ip.org
these functions ideally shouldn't have separate implementation at all.
However the problem is that pg_basebackup is not linked to the backend.

You're right about sys/time.h, it's included via via streamutil.h. I'll
fix that too.
> I will try your patch later today to see, if it works.
>
Whenever you have time. Thanks!

// Tony

On Wed, 31 Jul 2013 22:50:19 +0200
Antonin Houska <antonin(dot)houska(at)gmail(dot)com> wrote:

> On 07/31/2013 07:13 AM, Gibheer wrote:
> > Hi,
> >
> > That is a really nice feature.
> I don't pretend it's my idea, I just coded it. My boss proposed the
> feature as such :-)
> > I took a first look at your patch and some empty lines you added
> > (e.g. line 60 your patch). Can you remove them?
> Sure, will do in the next version.
> > Why did you move localGetCurrentTimestamp() into streamutil.c? Is
> > sys/time.h still needed in receivelog.c after the move?
> Because both receivelog.c and pg_basebackup.c need it now. I thought
> I could move localTimestampDifference() and
> localTimestampDifferenceExceeds() as well for the sake of consistency
> (these are actually utilities too) but I didn't get convinced enough
> that the feature alone justifies such a change.
>
> As mentioned in
> http://www.postgresql.org/message-id/20130731173624.GX14652@eldon.alvh.no-ip.org
> these functions ideally shouldn't have separate implementation at
> all. However the problem is that pg_basebackup is not linked to the
> backend.
>
> You're right about sys/time.h, it's included via via streamutil.h.
> I'll fix that too.
> > I will try your patch later today to see, if it works.
> >
> Whenever you have time. Thanks!
>
> // Tony

Hi,

I got around to test your patch and it works. I've build a small script
for others to test it easily. You can tell it with ROOTDIR and BASEDIR
environment variables where to look for the binaries and where to put
the test directories.

There is only one small thing I hit, namely the error message when the
limit is too small. It was like

transfer rate out of range '10k'

but it does not mention what the actual range is.

Maybe a message like

transfer rate of 10k is too small. Lower limit is 32k.

or

transfer rate has to be between 32k and 1GB, was 10k.

would be better as they mention what the actual limit is. That avoids
that people have to look up the limits in the manual.
We should also add the current limits to the documentation.

regards,

Stefan Radomski

2013-07-31 22:50 keltezéssel, Antonin Houska írta:
> On 07/31/2013 07:13 AM, Gibheer wrote:
>> Hi,
>>
>> That is a really nice feature.
> I don't pretend it's my idea, I just coded it. My boss proposed the feature as such :-)
>> I took a first look at your patch and some empty lines you added (e.g. line 60 your patch). Can you remove
>> them?
> Sure, will do in the next version.
>> Why did you move localGetCurrentTimestamp() into streamutil.c? Is sys/time.h still needed in receivelog.c after the move?
> Because both receivelog.c and pg_basebackup.c need it now. I thought I could move
> localTimestampDifference() and localTimestampDifferenceExceeds() as well for the sake of
> consistency (these are actually utilities too) but I didn't get convinced enough that
> the feature alone justifies such a change.
>
> As mentioned in
> http://www.postgresql.org/message-id/20130731173624.GX14652@eldon.alvh.no-ip.org these
> functions ideally shouldn't have separate implementation at all. However the problem is
> that pg_basebackup is not linked to the backend.

Have you considered the src/common directory?

>
> You're right about sys/time.h, it's included via via streamutil.h. I'll fix that too.
>> I will try your patch later today to see, if it works.
>>
> Whenever you have time. Thanks!
>
> // Tony

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/

Hi,

On 2013-07-24 09:20:52 +0200, Antonin Houska wrote:
> Hello,
> the purpose of this patch is to limit impact of pg_backup on running server.
> Feedback is appreciated.

Based on a quick look it seems like you're throttling on the receiving
side. Is that a good idea? Especially over longer latency links, TCP
buffering will reduce the effect on the sender side considerably.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

2013-08-19 19:20 keltezéssel, Andres Freund írta:
> Hi,
>
> On 2013-07-24 09:20:52 +0200, Antonin Houska wrote:
>> Hello,
>> the purpose of this patch is to limit impact of pg_backup on running server.
>> Feedback is appreciated.
> Based on a quick look it seems like you're throttling on the receiving
> side. Is that a good idea? Especially over longer latency links, TCP
> buffering will reduce the effect on the sender side considerably.
>
> Greetings,
>
> Andres Freund

Throttling on the sender side requires extending the syntax of
BASE_BACKUP and maybe START_REPLICATION so both can be
throttled but throttling is still initiated by the receiver side.

Maybe throttling the walsender is not a good idea, it can lead
to DoS via disk space shortage.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/

On 2013-08-19 20:15:51 +0200, Boszormenyi Zoltan wrote:
> 2013-08-19 19:20 keltezéssel, Andres Freund írta:
> >Hi,
> >
> >On 2013-07-24 09:20:52 +0200, Antonin Houska wrote:
> >>Hello,
> >>the purpose of this patch is to limit impact of pg_backup on running server.
> >>Feedback is appreciated.
> >Based on a quick look it seems like you're throttling on the receiving
> >side. Is that a good idea? Especially over longer latency links, TCP
> >buffering will reduce the effect on the sender side considerably.

> Throttling on the sender side requires extending the syntax of
> BASE_BACKUP and maybe START_REPLICATION so both can be
> throttled but throttling is still initiated by the receiver side.

Seems fine to me. Under the premise that the idea is decided to be
worthwile to be integrated. Which I am not yet convinced of.

> Maybe throttling the walsender is not a good idea, it can lead
> to DoS via disk space shortage.

Not in a measurably different way than receiver side throttling?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

2013-08-19 21:11 keltezéssel, Andres Freund írta:
> On 2013-08-19 20:15:51 +0200, Boszormenyi Zoltan wrote:
>> 2013-08-19 19:20 keltezéssel, Andres Freund írta:
>>> Hi,
>>>
>>> On 2013-07-24 09:20:52 +0200, Antonin Houska wrote:
>>>> Hello,
>>>> the purpose of this patch is to limit impact of pg_backup on running server.
>>>> Feedback is appreciated.
>>> Based on a quick look it seems like you're throttling on the receiving
>>> side. Is that a good idea? Especially over longer latency links, TCP
>>> buffering will reduce the effect on the sender side considerably.
>> Throttling on the sender side requires extending the syntax of
>> BASE_BACKUP and maybe START_REPLICATION so both can be
>> throttled but throttling is still initiated by the receiver side.
> Seems fine to me. Under the premise that the idea is decided to be
> worthwile to be integrated. Which I am not yet convinced of.
>
>> Maybe throttling the walsender is not a good idea, it can lead
>> to DoS via disk space shortage.
> Not in a measurably different way than receiver side throttling?

No, but that's not what I meant.

START_BACKUP has to deal with big data but it finishes after some time.
But pg_receivexlog may sit there indefinitely...

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/

On 19.08.2013 21:15, Boszormenyi Zoltan wrote:
> 2013-08-19 19:20 keltezéssel, Andres Freund írta:
>> Based on a quick look it seems like you're throttling on the receiving
>> side. Is that a good idea? Especially over longer latency links, TCP
>> buffering will reduce the effect on the sender side considerably.
>
> Throttling on the sender side requires extending the syntax of
> BASE_BACKUP and maybe START_REPLICATION so both can be
> throttled but throttling is still initiated by the receiver side.

Throttling in the client seems much better to me. TCP is designed to
handle a slow client.

> Maybe throttling the walsender is not a good idea, it can lead
> to DoS via disk space shortage.

If a client can initiate a backup and/or streaming replication, he can
already do much more damage than a DoS via out of disk space. And a
nothing stops even a non-privileged user from causing an out of disk
space situation anyway. IOW that's a non-issue.

- Heikki

2013-08-20 08:37 keltezéssel, Heikki Linnakangas írta:
> On 19.08.2013 21:15, Boszormenyi Zoltan wrote:
>> 2013-08-19 19:20 keltezéssel, Andres Freund írta:
>>> Based on a quick look it seems like you're throttling on the receiving
>>> side. Is that a good idea? Especially over longer latency links, TCP
>>> buffering will reduce the effect on the sender side considerably.
>>
>> Throttling on the sender side requires extending the syntax of
>> BASE_BACKUP and maybe START_REPLICATION so both can be
>> throttled but throttling is still initiated by the receiver side.
>
> Throttling in the client seems much better to me. TCP is designed to handle a slow client.
>
>> Maybe throttling the walsender is not a good idea, it can lead
>> to DoS via disk space shortage.
>
> If a client can initiate a backup and/or streaming replication, he can already do much
> more damage than a DoS via out of disk space. And a nothing stops even a non-privileged
> user from causing an out of disk space situation anyway. IOW that's a non-issue.

I got to the same conclusion this morning, but because of wal_keep_segments.

Best regards,
Zoltán Böszörményi

>
> - Heikki
>
>

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/

On Aug 19, 2013, at 9:11 PM, Andres Freund wrote:

> On 2013-08-19 20:15:51 +0200, Boszormenyi Zoltan wrote:
>> 2013-08-19 19:20 keltezéssel, Andres Freund írta:
>>> Hi,
>>>
>>> On 2013-07-24 09:20:52 +0200, Antonin Houska wrote:
>>>> Hello,
>>>> the purpose of this patch is to limit impact of pg_backup on running server.
>>>> Feedback is appreciated.
>>> Based on a quick look it seems like you're throttling on the receiving
>>> side. Is that a good idea? Especially over longer latency links, TCP
>>> buffering will reduce the effect on the sender side considerably.
>
>> Throttling on the sender side requires extending the syntax of
>> BASE_BACKUP and maybe START_REPLICATION so both can be
>> throttled but throttling is still initiated by the receiver side.
>
> Seems fine to me. Under the premise that the idea is decided to be
> worthwile to be integrated. Which I am not yet convinced of.

i think there is a lot of value for this one. the scenario we had a couple of times is pretty simple:
just assume a weak server - maybe just one disk or two - and a slave.
master and slave are connected via a 1 GB network.
pg_basebackup will fetch data full speed basically putting those lonely disks out of business.
we actually had a case where a client asked if "PostgreSQL is locked during base backup". of
course it was just disk wait caused by a full speed pg_basebackup.

regarding the client side implementation: we have chosen this way because it is less invasive.
i cannot see a reason to do this on the server side because we won't have 10
pg_basebackup-style tools making use of this feature anyway.

of course, if you got 20 disk and a 1 gbit network this is useless - but many people don't have that.

regards,

hans-jürgen schönig

--
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de

On 2013-08-21 08:10:42 +0200, PostgreSQL - Hans-Jürgen Schönig wrote:
>
> On Aug 19, 2013, at 9:11 PM, Andres Freund wrote:
>
> > On 2013-08-19 20:15:51 +0200, Boszormenyi Zoltan wrote:
> >> 2013-08-19 19:20 keltezéssel, Andres Freund írta:
> >>> Hi,
> >>>
> >>> On 2013-07-24 09:20:52 +0200, Antonin Houska wrote:
> >>>> Hello,
> >>>> the purpose of this patch is to limit impact of pg_backup on running server.
> >>>> Feedback is appreciated.
> >>> Based on a quick look it seems like you're throttling on the receiving
> >>> side. Is that a good idea? Especially over longer latency links, TCP
> >>> buffering will reduce the effect on the sender side considerably.
> >
> >> Throttling on the sender side requires extending the syntax of
> >> BASE_BACKUP and maybe START_REPLICATION so both can be
> >> throttled but throttling is still initiated by the receiver side.
> >
> > Seems fine to me. Under the premise that the idea is decided to be
> > worthwile to be integrated. Which I am not yet convinced of.
>
> i think there is a lot of value for this one. the scenario we had a couple of times is pretty simple:
> just assume a weak server - maybe just one disk or two - and a slave.
> master and slave are connected via a 1 GB network.
> pg_basebackup will fetch data full speed basically putting those lonely disks out of business.
> we actually had a case where a client asked if "PostgreSQL is locked during base backup". of
> course it was just disk wait caused by a full speed pg_basebackup.

> regarding the client side implementation: we have chosen this way because it is less invasive.
> i cannot see a reason to do this on the server side because we won't have 10
> pg_basebackup-style tools making use of this feature anyway.

The problem is that receiver side throttling over TCP doesn't always
work all that nicely unless you have a low rate of transfer and/or very
low latency . Quite often you will have OS buffers/the TCP Window being
filled in bursts where the sender sends at max capacity and then a
period where nothing happens on the sender. That's often not what you
want when you need to throttle.

Besides, I can see some value in e.g. normal streaming replication also
being rate limited...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Aug 21, 2013, at 10:57 AM, Andres Freund wrote:

> On 2013-08-21 08:10:42 +0200, PostgreSQL - Hans-Jürgen Schönig wrote:
>>
>> On Aug 19, 2013, at 9:11 PM, Andres Freund wrote:
>>
>>> On 2013-08-19 20:15:51 +0200, Boszormenyi Zoltan wrote:
>>>> 2013-08-19 19:20 keltezéssel, Andres Freund írta:
>>>>> Hi,
>>>>>
>>>>> On 2013-07-24 09:20:52 +0200, Antonin Houska wrote:
>>>>>> Hello,
>>>>>> the purpose of this patch is to limit impact of pg_backup on running server.
>>>>>> Feedback is appreciated.
>>>>> Based on a quick look it seems like you're throttling on the receiving
>>>>> side. Is that a good idea? Especially over longer latency links, TCP
>>>>> buffering will reduce the effect on the sender side considerably.
>>>
>>>> Throttling on the sender side requires extending the syntax of
>>>> BASE_BACKUP and maybe START_REPLICATION so both can be
>>>> throttled but throttling is still initiated by the receiver side.
>>>
>>> Seems fine to me. Under the premise that the idea is decided to be
>>> worthwile to be integrated. Which I am not yet convinced of.
>>
>> i think there is a lot of value for this one. the scenario we had a couple of times is pretty simple:
>> just assume a weak server - maybe just one disk or two - and a slave.
>> master and slave are connected via a 1 GB network.
>> pg_basebackup will fetch data full speed basically putting those lonely disks out of business.
>> we actually had a case where a client asked if "PostgreSQL is locked during base backup". of
>> course it was just disk wait caused by a full speed pg_basebackup.
>
>> regarding the client side implementation: we have chosen this way because it is less invasive.
>> i cannot see a reason to do this on the server side because we won't have 10
>> pg_basebackup-style tools making use of this feature anyway.
>
> The problem is that receiver side throttling over TCP doesn't always
> work all that nicely unless you have a low rate of transfer and/or very
> low latency . Quite often you will have OS buffers/the TCP Window being
> filled in bursts where the sender sends at max capacity and then a
> period where nothing happens on the sender. That's often not what you
> want when you need to throttle.
>
> Besides, I can see some value in e.g. normal streaming replication also
> being rate limited...
>

what would be a reasonable scenario where limiting streaming would make sense? i cannot think of any to be honest.

regards,

hans

--
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de

On 08/22/2013 01:39 PM, PostgreSQL - Hans-Jürgen Schönig wrote:

> what would be a reasonable scenario where limiting streaming would make sense? i cannot think of any to be honest.

I tend to agree. If anything we're likely to want the reverse - the
ability to throttle WAL *generation* on the master so streaming can keep up.

I see a lot of value in throttling base backup transfer rates. It's
something PgBarman does per-tablespace using rsync at the moment, but
it'd be nice if it available as an option possible over the streaming
replication protocol via pg_basebackup so it was easier for people to
use ad-hoc and without all the shell access wrangling.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2013-08-22 07:39:41 +0200, PostgreSQL - Hans-Jürgen Schönig wrote:
> >> regarding the client side implementation: we have chosen this way because it is less invasive.
> >> i cannot see a reason to do this on the server side because we won't have 10
> >> pg_basebackup-style tools making use of this feature anyway.
> >
> > The problem is that receiver side throttling over TCP doesn't always
> > work all that nicely unless you have a low rate of transfer and/or very
> > low latency . Quite often you will have OS buffers/the TCP Window being
> > filled in bursts where the sender sends at max capacity and then a
> > period where nothing happens on the sender. That's often not what you
> > want when you need to throttle.
> >
> > Besides, I can see some value in e.g. normal streaming replication also
> > being rate limited...
> >
>
>
> what would be a reasonable scenario where limiting streaming would make sense? i cannot think of any to be honest.

It's not an unreasonable goal if you have several streaming replicas
with only some of them being synchronous replicas. Also, analytics
replicas that need to catchup don't really need priority over local
operations.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, 2013-07-24 at 09:20 +0200, Antonin Houska wrote:
> the purpose of this patch is to limit impact of pg_backup on running
> server. Feedback is appreciated.

Please replace the tabs in the SGML files with spaces.

On 08/22/2013 03:33 PM, Craig Ringer wrote:
> On 08/22/2013 01:39 PM, PostgreSQL - Hans-Jürgen Schönig wrote:
>
>> what would be a reasonable scenario where limiting streaming would make sense? i cannot think of any to be honest.
>
> I tend to agree. If anything we're likely to want the reverse - the
> ability to throttle WAL *generation* on the master so streaming can keep up.

(I assume you mean WAL *sending* rather than *generation*.)

I don't think we want to throttle WAL sending/receiving at all. The WAL
senders should always keep up with the amount of WAL generated. If the
rate of WAL sending is restricted, then the pg_basebackup (or another
client application) might never catch up.

Also, IMO, pg_basebackup starts at the current WAL segment. Thus the
unthrottled WAL transfer shouldn't cause significant additional load,
such as reading many old WAL segments from the master's disk.

> I see a lot of value in throttling base backup transfer rates. It's
> something PgBarman does per-tablespace using rsync at the moment, but
> it'd be nice if it available as an option possible over the streaming
> replication protocol via pg_basebackup so it was easier for people to
> use ad-hoc and without all the shell access wrangling.

(Possibly huge) DATA directory (tablespaces, etc.) is the actual purpose
of this patch. This is the additional load that pg_basebackup imposes on
the master. As pointed out elsewhere in the thread, the client-side
throttling was chosen because it seemed to be less invasive. But the
discussion (including this your comment) keeps me no longer convinced
that it's the best way. I'll reconsider things.

// Antonin Houska (Tony)

On 08/26/2013 12:50 PM, Antonin Houska wrote:
> On 08/22/2013 03:33 PM, Craig Ringer wrote:
>> On 08/22/2013 01:39 PM, PostgreSQL - Hans-Jürgen Schönig wrote:
>>
>>> what would be a reasonable scenario where limiting streaming would make sense? i cannot think of any to be honest.
>> I tend to agree. If anything we're likely to want the reverse - the
>> ability to throttle WAL *generation* on the master so streaming can keep up.
> (I assume you mean WAL *sending* rather than *generation*.)
I think he meant *generation*, that is making sure that no more
than X amount of WAL is generated in Y amount of time, thereby
making sure that you can stream less WAL without falling behind.
> I don't think we want to throttle WAL sending/receiving at all. The WAL
> senders should always keep up with the amount of WAL generated. If the
> rate of WAL sending is restricted, then the pg_basebackup (or another
> client application) might never catch up.
Yes, and this is exactly why restricting *generation* is needed.
> Also, IMO, pg_basebackup starts at the current WAL segment. Thus the
> unthrottled WAL transfer shouldn't cause significant additional load,
> such as reading many old WAL segments from the master's disk.
The possible extra load happens if the *network* not because of disk.
Think of replication over - possibly slow - WAN.

Cheers

--
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ

On 08/26/2013 08:15 PM, Hannu Krosing wrote:
> On 08/26/2013 12:50 PM, Antonin Houska wrote:
>> > On 08/22/2013 03:33 PM, Craig Ringer wrote:
>>> >> On 08/22/2013 01:39 PM, PostgreSQL - Hans-Jürgen Schönig wrote:
>>> >>
>>>> >>> what would be a reasonable scenario where limiting streaming would make sense? i cannot think of any to be honest.
>>> >> I tend to agree. If anything we're likely to want the reverse - the
>>> >> ability to throttle WAL *generation* on the master so streaming can keep up.
>> > (I assume you mean WAL *sending* rather than *generation*.)
> I think he meant *generation*, that is making sure that no more
> than X amount of WAL is generated in Y amount of time, thereby
> making sure that you can stream less WAL without falling behind.
>

Exactly so.

Sometimes one doesn't want the latency of synchronous replication, but
still wants to set a limit on how far behind the standby can fall. It
might be for limiting data loss, for making sure a replica can keep up
sustainably, or just to make sure the replica never gets far enough
behind that the master discards WAL that it still requires.

For many people the idea of the replica(s) affecting the master is
horrifying. For others, missing a single transaction on a crash is
unthinkable. We handle both those pretty well, but the middle ground is
currently very hard to walk, and I think that's actually where many
people will want to be.

I'd certainly rather throttle the master (and send a loud, angry alert
to the admin via icinga/nagios) than have a replica fall too far behind
and need to be re-inited from scratch.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 08/26/2013 02:33 PM, Craig Ringer wrote:
> On 08/26/2013 08:15 PM, Hannu Krosing wrote:
>> On 08/26/2013 12:50 PM, Antonin Houska wrote:
>>>> On 08/22/2013 03:33 PM, Craig Ringer wrote:
>>>>>> On 08/22/2013 01:39 PM, PostgreSQL - Hans-Jürgen Schönig wrote:
>>>>>>
>>>>>>>> what would be a reasonable scenario where limiting streaming would make sense? i cannot think of any to be honest.
>>>>>> I tend to agree. If anything we're likely to want the reverse - the
>>>>>> ability to throttle WAL *generation* on the master so streaming can keep up.
>>>> (I assume you mean WAL *sending* rather than *generation*.)
>> I think he meant *generation*, that is making sure that no more
>> than X amount of WAL is generated in Y amount of time, thereby
>> making sure that you can stream less WAL without falling behind.
>>
>
> Exactly so.
>
> Sometimes one doesn't want the latency of synchronous replication, but
> still wants to set a limit on how far behind the standby can fall. It
> might be for limiting data loss, for making sure a replica can keep up
> sustainably, or just to make sure the replica never gets far enough
> behind that the master discards WAL that it still requires.

I think the question that brought us here was whether new functionality
of pg_basebackup should be implemented on server side, so that other
client applications - including the Barman that you mentioned in the
previous mail - don't have to duplicate it. Ability to throttle (one
time) transfer of all the files that standby setup requires falls into
this category.

However what you stress now is control of the (continuous) WAL stream
and thus something that affects normal operation, rather than setup. I
still think the pg_basebackup does not have to throttle the WAL stream,
so this your request does not overlap with the current patch.

// Antonin Houska (Tony)

On 08/27/2013 01:56 AM, Antonin Houska wrote:
> However what you stress now is control of the (continuous) WAL stream
> and thus something that affects normal operation, rather than setup. I
> still think the pg_basebackup does not have to throttle the WAL stream,
> so this your request does not overlap with the current patch.

I totally agree; I was getting off topic for this thread, and it's not
relevant to the patch as it stands.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Aug 20, 2013 at 2:37 AM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> Throttling in the client seems much better to me. TCP is designed to handle
> a slow client.

Other people have already offered some good points in this area, but
let me just add one thought that I don't think has been mentioned yet.
We have a *general* need to be able to throttle server-side resource
utilization, particularly I/O. This is a problem not only for
pg_basebackup, but for COPY, CLUSTER, VACUUM, and even things like
UPDATE. Of all of those, the only one for which we currently have any
kind of a solution is VACUUM. Now, maybe pg_basebackup also needs its
own special-purpose solution, but I think we'd do well to consider a
general I/O rate-limiting strategy and then consider particular needs
in the light of that framework. In that context, server-side seems
better to me, because something like CLUSTER isn't going to produce
anything that the client can effectively limit.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Aug 27, 2013 at 12:58 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Tue, Aug 20, 2013 at 2:37 AM, Heikki Linnakangas
> <hlinnakangas(at)vmware(dot)com> wrote:
> > Throttling in the client seems much better to me. TCP is designed to
> handle
> > a slow client.
>
> Other people have already offered some good points in this area, but
> let me just add one thought that I don't think has been mentioned yet.
> We have a *general* need to be able to throttle server-side resource
> utilization, particularly I/O. This is a problem not only for
> pg_basebackup, but for COPY, CLUSTER, VACUUM, and even things like
> UPDATE. Of all of those, the only one for which we currently have any
> kind of a solution is VACUUM. Now, maybe pg_basebackup also needs its
> own special-purpose solution, but I think we'd do well to consider a
> general I/O rate-limiting strategy and then consider particular needs
> in the light of that framework. In that context, server-side seems
> better to me, because something like CLUSTER isn't going to produce
> anything that the client can effectively limit.
>

+1 it is very easy at the moment to for example run a manual vacuum
full/cluster against a big table and generate WAL so quickly that the hot
standby disconnects because it gets "too far behind".

On 08/27/2013 01:58 PM, Robert Haas wrote:
> On Tue, Aug 20, 2013 at 2:37 AM, Heikki Linnakangas
> <hlinnakangas(at)vmware(dot)com> wrote:
>> Throttling in the client seems much better to me. TCP is designed to handle
>> a slow client.
>
> Other people have already offered some good points in this area, but
> let me just add one thought that I don't think has been mentioned yet.
> We have a *general* need to be able to throttle server-side resource
> utilization, particularly I/O. This is a problem not only for
> pg_basebackup, but for COPY, CLUSTER, VACUUM, and even things like
> UPDATE. Of all of those, the only one for which we currently have any
> kind of a solution is VACUUM. Now, maybe pg_basebackup also needs its
> own special-purpose solution, but I think we'd do well to consider a
> general I/O rate-limiting strategy and then consider particular needs
> in the light of that framework. In that context, server-side seems
> better to me, because something like CLUSTER isn't going to produce
> anything that the client can effectively limit.
>

In fact I already concluded that server-side control is better and even
started to implement it for the next version of the patch. However the
pg_basebackup is different from VACUUM, CLUSTER, etc. in that it
retrieves the data directly from file system, as opposed to buffers. So
there seems to be little room here for utilization of (future)
'throttling infrastructure'.

// Antonin Houska (Tony)

On 8/27/13 7:58 AM, Robert Haas wrote:
> We have a *general* need to be able to throttle server-side resource
> utilization, particularly I/O. This is a problem not only for
> pg_basebackup, but for COPY, CLUSTER, VACUUM, and even things like
> UPDATE. Of all of those, the only one for which we currently have any
> kind of a solution is VACUUM.

I didn't mention it specifically, but I always presumed that the "Cost
limited statements RFC" proposal I floated:
http://www.postgresql.org/message-id/519EA5FF.5040606@2ndQuadrant.com
(and am still working on) would handle the base backup case too.
pg_basebackup is just another client. Some sort of purpose made
solution for pg_basebackup alone may be useful, but I'd be shocked if
the sort of general mechanism I described there wasn't good enough to
handle many of the backup limiting cases too.

Also, once that and the block write counters I also sent an RFC out for
are in place, I have a plan for adding a server-wide throttling
mechanism. I want to extract a cluster wide read and write rate and put
a cluster wide limit on that whole thing. It seemed too hard to jump
into without these other two pieces of plumbing in place first.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

On 07/24/2013 09:20 AM, Antonin Houska wrote:
> Hello,
> the purpose of this patch is to limit impact of pg_backup on running
> server.

Attached is a new version. Server-side implementation this time.

Antonin Houska (Tony)

Hi,

On 2013-09-03 14:35:18 +0200, Antonin Houska wrote:

> + /*
> + * THROTTLING_SAMPLE_MIN / MAX_RATE_LOWER (in seconds) should be the
> + * longest possible time to sleep.
> + */
> + pg_usleep((long) sleep);
> + else
> +
> + /*
> + * The actual transfer rate is below the limit. Negative value would
> + * distort the adjustment of throttled_last.
> + */
> + sleep = 0;
> +
> + /*
> + * Only the whole multiples of throttling_sample processed. The rest will
> + * be done during the next call of this function.
> + */
> + throttling_counter %= throttling_sample;
> + /* Once the (possible) sleep ends, new period starts. */
> + throttled_last += elapsed + sleep;
> +}

It's probably better to use latches for the waiting, those have properly
defined interruption semantics. Whether pg_usleep will be interrupted is
platform dependant...

Greetings,

Andres Freund

Antonin Houska wrote:

> + <para>
> + Suffixes <literal>k</literal> (kilobytes) and <literal>m</literal>
> + (megabytes) are accepted. For example: <literal>10m</literal>
> + </para>

"m" is for meters, or milli. Please use "M" here.

> +static uint32
> +parse_max_rate(char *src)
> +{
> + int factor;
> + char *after_num;
> + int64 result;
> + int errno_copy;
> +
> + result = strtol(src, &after_num, 0);
> + errno_copy = errno;
> + if (src == after_num)
> + {
> + fprintf(stderr, _("%s: transfer rate %s is not a valid integer value\n"), progname, src);
> + exit(1);
> + }

Please add quotes to the invalid value.

> +
> + /*
> + * Evaluate (optional) suffix.
> + *
> + * after_num should now be right behind the numeric value.
> + */
> + factor = 1;
> + switch (tolower(*after_num))
> + {
> + /*
> + * Only the following suffixes are allowed. It's not too useful to
> + * restrict the rate to gigabytes: such a rate will probably bring
> + * significant impact on the master anyway, so the throttling
> + * won't help much.
> + */
> + case 'g':
> + factor <<= 10;

I don't understand why you allow a 'g' here, given the comment above ...
but in any case it should be G.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 09/03/2013 06:56 PM, Alvaro Herrera wrote:

>> + /*
>> + * Only the following suffixes are allowed. It's not too useful to
>> + * restrict the rate to gigabytes: such a rate will probably bring
>> + * significant impact on the master anyway, so the throttling
>> + * won't help much.
>> + */
>> + case 'g':
>> + factor <<= 10;
>
> I don't understand why you allow a 'g' here, given the comment above ...
> but in any case it should be G.
>

This reflects my hesitation whether GB should be accepted as a unit or
not. I'll probably remove this suffix.

(Will fix the other findings too,)

Thanks,
Tony

On 2013-09-03 12:56:53 -0400, Alvaro Herrera wrote:
> Antonin Houska wrote:
>
> > + <para>
> > + Suffixes <literal>k</literal> (kilobytes) and <literal>m</literal>
> > + (megabytes) are accepted. For example: <literal>10m</literal>
> > + </para>
>
> "m" is for meters, or milli. Please use "M" here.

Shouldn't it be MB? Consistent with GUCs?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Sep 3, 2013 at 8:35 AM, Antonin Houska <antonin(dot)houska(at)gmail(dot)com> wrote:
> On 07/24/2013 09:20 AM, Antonin Houska wrote:
>> Hello,
>> the purpose of this patch is to limit impact of pg_backup on running
>> server.
>
> Attached is a new version. Server-side implementation this time.
>
> Antonin Houska (Tony)

There seem to be several review comments made since you posted this
version. I'll mark this Waiting on Author in the CommitFest
application, since it seems that the patch needs to be further
updated.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 10/09/2013 08:56 PM, Robert Haas wrote:
> There seem to be several review comments made since you posted this
> version. I'll mark this Waiting on Author in the CommitFest
> application, since it seems that the patch needs to be further
> updated.

Since it was waiting for reviewer, I was not sure whether I should wait
for his findings and fix everything in a single transaction.
Nevertheless, the next version is attached here.

It fixes findings reported in
http://www.postgresql.org/message-id/20130903165652.GC5227@eldon.alvh.no-ip.org

As for units
http://www.postgresql.org/message-id/20130903224127.GD7018@awork2.anarazel.de
I'm not convinced about "MB" at the moment. Unfortunately I couldn't
find any other command-line PG utility requiring amount of data as an
option. Thus I use single-letter suffix, just as wget does for the same
purposes.

As for this
http://www.postgresql.org/message-id/20130903125155.GA18486@awork2.anarazel.de
there eventually seems to be a consensus (I notice now the discussion
was off-list):

> On 2013-09-03 23:21:57 +0200, Antonin Houska wrote:
>> On 09/03/2013 02:51 PM, Andres Freund wrote:
>>
>>>
>>> It's probably better to use latches for the waiting, those have properly
>>> defined interruption semantics. Whether pg_usleep will be interrupted is
>>> platform dependant...
>>
>> I noticed a comment about interruptions around the definition of
>> pg_usleep() function, but concluded that the sleeps are rather frequent
>> in this applications (typically in the order of tens to hundreds per
>> second, although the maximum of 256 might need to be decreased).
>> Therefore occasional interruptions shouldn't distort the actual rate
>> much. I'll think about it again. Thanks,
>
> The issue is rather that you might not get woken up when you want to
> be. Signal handlers in postgres tend to do a
> SetLatch(&MyProc->procLatch); which then will interrupt sleeps done via
> WaitLatch(). It's probably not that important with the duration of the
> sleeps you have.
>
> Greetings,
>
> Andres Freund

// Antonin Houska (Tony)

Hi,

I am reviewing your patch.

2013-10-10 15:32 keltezéssel, Antonin Houska írta:
> On 10/09/2013 08:56 PM, Robert Haas wrote:
>> There seem to be several review comments made since you posted this
>> version. I'll mark this Waiting on Author in the CommitFest
>> application, since it seems that the patch needs to be further
>> updated.
> Since it was waiting for reviewer, I was not sure whether I should wait
> for his findings and fix everything in a single transaction.
> Nevertheless, the next version is attached here.
>
> It fixes findings reported in
> http://www.postgresql.org/message-id/20130903165652.GC5227@eldon.alvh.no-ip.org
>
> As for units
> http://www.postgresql.org/message-id/20130903224127.GD7018@awork2.anarazel.de
> I'm not convinced about "MB" at the moment. Unfortunately I couldn't
> find any other command-line PG utility requiring amount of data as an
> option. Thus I use single-letter suffix, just as wget does for the same
> purposes.
>
> As for this
> http://www.postgresql.org/message-id/20130903125155.GA18486@awork2.anarazel.de
> there eventually seems to be a consensus (I notice now the discussion
> was off-list):
>
>> On 2013-09-03 23:21:57 +0200, Antonin Houska wrote:
>>> On 09/03/2013 02:51 PM, Andres Freund wrote:
>>>
>>>> It's probably better to use latches for the waiting, those have properly
>>>> defined interruption semantics. Whether pg_usleep will be interrupted is
>>>> platform dependant...
>>> I noticed a comment about interruptions around the definition of
>>> pg_usleep() function, but concluded that the sleeps are rather frequent
>>> in this applications (typically in the order of tens to hundreds per
>>> second, although the maximum of 256 might need to be decreased).
>>> Therefore occasional interruptions shouldn't distort the actual rate
>>> much. I'll think about it again. Thanks,
>> The issue is rather that you might not get woken up when you want to
>> be. Signal handlers in postgres tend to do a
>> SetLatch(&MyProc->procLatch); which then will interrupt sleeps done via
>> WaitLatch(). It's probably not that important with the duration of the
>> sleeps you have.
>>
>> Greetings,
>>
>> Andres Freund
> // Antonin Houska (Tony)

* Is the patch in a patch format which has context?

Yes

* Does it apply cleanly to the current git master?

It applies with some offsets. Version "3a" that applies cleanly is attached.

* Does it include reasonable tests, necessary doc patches, etc?

Docs: yes. Tests: N/A

* Does the patch actually implement what it advertises?

Yes.

* Do we want that?

Yes.

* Do we already have it?

No.

* Does it follow SQL spec, or the community-agreed behavior?

No such SQL spec. The latest patch fixed all previously raised comments.

* Does it include pg_dump support (if applicable)?

N/A

* Are there dangers?

Yes, the "danger" of slowing down taking a base backup.
But this is the point.

* Have all the bases been covered?

Yes.

* Does the feature work as advertised?

Yes.

* Are there corner cases the author has failed to consider?

No.

* Are there any assertion failures or crashes?

No.

* Does the patch slow down simple tests?

No.

* If it claims to improve performance, does it?

N/A

* Does it slow down other things?

No.

* Does it follow the project coding guidelines?

Yes. A nitpicking: this else branch below might need brackets
because there is also a comment in that branch:

+ /* The 'real data' starts now (header was ignored). */
+ throttled_last = GetCurrentIntegerTimestamp();
+ }
+ else
+ /* Disable throttling. */
+ throttling_counter = -1;
+

* Are there portability issues?

No.

* Will it work on Windows/BSD etc?

It should, there are no dangerous calls besides the above mentioned
pg_usleep(). But waking up from pg_usleep() earlier makes rate limiting
fluctuate slightly, not fail.

* Are the comments sufficient and accurate?

Yes.

* Does it do what it says, correctly?

Yes.

Although it should be mentioned in the docs that rate limiting
applies to walsenders individually, not globally. I tried this
on a freshly created database:

$ time pg_basebackup -D data2 -r 1M -X stream -h 127.0.0.1
real 0m26.508s
user 0m0.054s
sys 0m0.360s

The source database had 3 WAL files in pg_xlog, one of them was
also streamed. The final size of "data2" was 43MB or 26MB without pg_xlog,
i.e. without the "-X stream" option. The backup used 2 walsenders
in parallel (one for WAL) which is a known feature.

* Does it produce compiler warnings?

No.

*Can you make it crash?

No.

Consider the changes to the code in the context of the project as a whole:

* Is everything done in a way that fits together coherently with other features/modules?

Yes.

* Are there interdependencies that can cause problems?

No.

Another note. This chunk should be submitted separately as a comment bugfix:

diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index c3c71b7..5736fd8 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -1288,7 +1288,7 @@ GetCurrentTimestamp(void)
/*
* GetCurrentIntegerTimestamp -- get the current operating system time as int64
*
- * Result is the number of milliseconds since the Postgres epoch. If compiled
+ * Result is the number of microseconds since the Postgres epoch. If compiled
* with --enable-integer-datetimes, this is identical to GetCurrentTimestamp(),
* and is implemented as a macro.
*/

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/

On 12/02/2013 02:23 PM, Boszormenyi Zoltan wrote:
> Hi,
>
> I am reviewing your patch.

Thanks. New version attached.

>
> * Does it follow the project coding guidelines?
>
> Yes. A nitpicking: this else branch below might need brackets
> because there is also a comment in that branch:
>
> + /* The 'real data' starts now (header was ignored). */
> + throttled_last = GetCurrentIntegerTimestamp();
> + }
> + else
> + /* Disable throttling. */
> + throttling_counter = -1;
> +

Done.

>
> * Does it do what it says, correctly?
>
> Yes.
>
> Although it should be mentioned in the docs that rate limiting
> applies to walsenders individually, not globally. I tried this
> on a freshly created database:
>
> $ time pg_basebackup -D data2 -r 1M -X stream -h 127.0.0.1
> real 0m26.508s
> user 0m0.054s
> sys 0m0.360s
>
> The source database had 3 WAL files in pg_xlog, one of them was
> also streamed. The final size of "data2" was 43MB or 26MB without pg_xlog,
> i.e. without the "-X stream" option. The backup used 2 walsenders
> in parallel (one for WAL) which is a known feature.

Right, if the method is 'stream', a separate WAL sender is used and the
transfer is not limited: client must keep up with the stream
unconditionally. I added a note to documentation.

But there's no reason not to throttle WAL files if the method is
'fetch'. It's fixed now.

> Another note. This chunk should be submitted separately as a comment bugfix:
>
> diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
> index c3c71b7..5736fd8 100644
> --- a/src/backend/utils/adt/timestamp.c
> +++ b/src/backend/utils/adt/timestamp.c
> @@ -1288,7 +1288,7 @@ GetCurrentTimestamp(void)
> /*
> * GetCurrentIntegerTimestamp -- get the current operating system time as int64
> *
> - * Result is the number of milliseconds since the Postgres epoch. If compiled
> + * Result is the number of microseconds since the Postgres epoch. If compiled
> * with --enable-integer-datetimes, this is identical to GetCurrentTimestamp(),
> * and is implemented as a macro.
> */

Will do.

// Tony

Hi,

2013-12-05 15:36 keltezéssel, Antonin Houska írta:
> On 12/02/2013 02:23 PM, Boszormenyi Zoltan wrote:
>> Hi,
>>
>> I am reviewing your patch.
> Thanks. New version attached.

I have reviewed and tested it and marked it as ready for committer.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/

On Fri, Dec 6, 2013 at 6:43 PM, Boszormenyi Zoltan <zb(at)cybertec(dot)at> wrote:
> Hi,
>
> 2013-12-05 15:36 keltezéssel, Antonin Houska írta:
>
>> On 12/02/2013 02:23 PM, Boszormenyi Zoltan wrote:
>>>
>>> Hi,
>>>
>>> I am reviewing your patch.
>>
>> Thanks. New version attached.
>
>
> I have reviewed and tested it and marked it as ready for committer.

Here are the review comments:

+ <term><option>-r</option></term>
+ <term><option>--max-rate</option></term>

You need to add something like <replaceable
class="parameter">rate</replaceable>.

+ The purpose is to limit impact of
<application>pg_basebackup</application>
+ on a running master server.

s/"master server"/"server" because we can take a backup from also the standby.

I think that it's better to document the default value and the accepted range of
the rate that we can specify.

You need to change the protocol.sgml because you changed BASE_BACKUP
replication command.

+ printf(_(" -r, --max-rate maximum transfer rate to
transfer data directory\n"));

You need to add something like =RATE just after --max-rate.

+ result = strtol(src, &after_num, 0);

errno should be set to 0 just before calling strtol().

+ if (errno_copy == ERANGE || result != (uint64) ((uint32) result))
+ {
+ fprintf(stderr, _("%s: transfer rate \"%s\" exceeds integer
range\n"), progname, src);
+ exit(1);
+ }

We can move this check after the check of "src == after_num" like
parse_int() in guc.c does.
If we do this, the local variable 'errno_copy' is no longer necessary.

I think that it's better to output the hint message like "Valid units for
the transfer rate are \"k\" and \"M\"." when a user specified wrong unit.

+ /*
+ * THROTTLING_SAMPLE_MIN / MAX_RATE_LOWER (in seconds) should be the
+ * longest possible time to sleep. Thus the cast to long is safe.
+ */
+ pg_usleep((long) sleep);

It's better to use the latch here so that we can interrupt immediately.

Regards,

--
Fujii Masao

Thanks for checking. The new version addresses your findings.

// Antonin Houska (Tony)

On 12/09/2013 03:49 PM, Fujii Masao wrote:
> On Fri, Dec 6, 2013 at 6:43 PM, Boszormenyi Zoltan <zb(at)cybertec(dot)at> wrote:
>> Hi,
>>
>> 2013-12-05 15:36 keltezéssel, Antonin Houska írta:
>>
>>> On 12/02/2013 02:23 PM, Boszormenyi Zoltan wrote:
>>>>
>>>> Hi,
>>>>
>>>> I am reviewing your patch.
>>>
>>> Thanks. New version attached.
>>
>>
>> I have reviewed and tested it and marked it as ready for committer.
>
> Here are the review comments:
>
> + <term><option>-r</option></term>
> + <term><option>--max-rate</option></term>
>
> You need to add something like <replaceable
> class="parameter">rate</replaceable>.
>
> + The purpose is to limit impact of
> <application>pg_basebackup</application>
> + on a running master server.
>
> s/"master server"/"server" because we can take a backup from also the standby.
>
> I think that it's better to document the default value and the accepted range of
> the rate that we can specify.
>
> You need to change the protocol.sgml because you changed BASE_BACKUP
> replication command.
>
> + printf(_(" -r, --max-rate maximum transfer rate to
> transfer data directory\n"));
>
> You need to add something like =RATE just after --max-rate.
>
> + result = strtol(src, &after_num, 0);
>
> errno should be set to 0 just before calling strtol().
>
> + if (errno_copy == ERANGE || result != (uint64) ((uint32) result))
> + {
> + fprintf(stderr, _("%s: transfer rate \"%s\" exceeds integer
> range\n"), progname, src);
> + exit(1);
> + }
>
> We can move this check after the check of "src == after_num" like
> parse_int() in guc.c does.
> If we do this, the local variable 'errno_copy' is no longer necessary.
>
> I think that it's better to output the hint message like "Valid units for
> the transfer rate are \"k\" and \"M\"." when a user specified wrong unit.
>
> + /*
> + * THROTTLING_SAMPLE_MIN / MAX_RATE_LOWER (in seconds) should be the
> + * longest possible time to sleep. Thus the cast to long is safe.
> + */
> + pg_usleep((long) sleep);
>
> It's better to use the latch here so that we can interrupt immediately.
>
> Regards,
>

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

Alvaro Herrera escribió:
> Antonin Houska escribió:
> > Thanks for checking. The new version addresses your findings.
>
> I gave this patch a look.

BTW I also moved the patch the commitfest currently running, and set it
waiting-on-author.

Your move.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hi,

On 2014-01-15 18:52:32 -0300, Alvaro Herrera wrote:
> 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.

Yes, this definitely should reuse MyWalSnd->latch.

slightly related: we should start to reuse procLatch for walsenders
instead of having a separate latch someday.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Jan 16, 2014 at 12:03 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> slightly related: we should start to reuse procLatch for walsenders
> instead of having a separate latch someday.

+1. The potential for bugs from failing to account for this within
signal handlers seems like a concern. I think that every process
should use the process latch, except for the archiver which uses a
local latch because it pointedly does not touch shared memory. I think
I wrote a comment that made it into the latch header comments
encouraging this, but never saw to it that it was universally adhered
to.

--
Peter Geoghegan

On Fri, Jan 17, 2014 at 5:03 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> slightly related: we should start to reuse procLatch for walsenders
> instead of having a separate latch someday.
+ 1 on that.
--
Michael

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)

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)
>

On Tue, Jan 21, 2014 at 1:10 AM, Antonin Houska
<antonin(dot)houska(at)gmail(dot)com> 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.

If there is no good place to define them, it's okay to define them
also in client side
for now.

+ <term>BASE_BACKUP [<literal>LABEL</literal>
<replaceable>'label'</replaceable>] [<literal>PROGRESS</literal>]
[<literal>FAST</literal>] [<literal>WAL</literal>]
[<literal>NOWAIT</literal>] [<literal>MAX_RATE</literal>]</term>

It's better to add something like <replaceable>'rate'</replaceable> just after
<literal>MAX_RATE</literal>.

+ <para>
+ <literal>MAX_RATE</literal> does not affect WAL streaming.
+ </para>

I don't think that this paragraph is required here because BASE_BACKUP is
basically independent from WAL streaming.

Why did you choose "bytes per second" as a valid rate which we can specify?
Since the minimum rate is 32kB, isn't it better to use "KB per second" for that?
If we do that, we can easily increase the maximum rate from 1GB to very large
number in the future if required.

+ wait_result = WaitLatch(&MyWalSnd->latch, WL_LATCH_SET | WL_TIMEOUT
+ | WL_POSTMASTER_DEATH, (long) (sleep / 1000));

If WL_POSTMASTER_DEATH is triggered, we should exit immediately like
other process does? This is not a problem of this patch. This problem exists
also in current master. But ISTM it's better to solve that together. Thought?

Regards,

--
Fujii Masao

On 01/31/2014 06:26 AM, Fujii Masao wrote:
>> 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.
>
> If there is no good place to define them, it's okay to define them
> also in client side
> for now.
> + <term>BASE_BACKUP [<literal>LABEL</literal>
> <replaceable>'label'</replaceable>] [<literal>PROGRESS</literal>]
> [<literal>FAST</literal>] [<literal>WAL</literal>]
> [<literal>NOWAIT</literal>] [<literal>MAX_RATE</literal>]</term>
>
> It's better to add something like <replaceable>'rate'</replaceable> just after
> <literal>MAX_RATE</literal>.
>
> + <para>
> + <literal>MAX_RATE</literal> does not affect WAL streaming.
> + </para>
>
> I don't think that this paragraph is required here because BASE_BACKUP is
> basically independent from WAL streaming.
>
> Why did you choose "bytes per second" as a valid rate which we can specify?
> Since the minimum rate is 32kB, isn't it better to use "KB per second" for that?
> If we do that, we can easily increase the maximum rate from 1GB to very large
> number in the future if required.

The attached version addresses all the comments above.

> + wait_result = WaitLatch(&MyWalSnd->latch, WL_LATCH_SET | WL_TIMEOUT
> + | WL_POSTMASTER_DEATH, (long) (sleep / 1000));
>
> If WL_POSTMASTER_DEATH is triggered, we should exit immediately like
> other process does? This is not a problem of this patch. This problem exists
> also in current master. But ISTM it's better to solve that together. Thought?

Once we're careful about not missing signals, I think PM death should be
noticed too. The backup functionality itself would probably manage to
finish without postmaster, however it's executed under walsender process.

Question is where !PostmasterIsAlive() check should be added. I think it
should go to the main loop of perform_base_backup(), but that's probably
not in the scope of this patch.

Do you think that my patch should only add a comment like "Don't forget
to set WL_POSTMASTER_DEATH flag when making basebackup.c sensitive to PM
death?"

// Antonin Houska (Tony)

Antonin Houska escribió:

> > Why did you choose "bytes per second" as a valid rate which we can specify?
> > Since the minimum rate is 32kB, isn't it better to use "KB per second" for that?
> > If we do that, we can easily increase the maximum rate from 1GB to very large
> > number in the future if required.
>
> The attached version addresses all the comments above.

I pushed this patch with a few further tweaks. In your changes to
address the above point, you made the suffix mandatory in the
pg_basebackup -r option. This seemed a strange restriction, so I
removed it. It seems more user-friendly to me to accept the value as
being expressed in kilobytes per second without requiring the suffix to
be there; the 'k' suffix is then also accepted and has no effect. I
amended the docs to say that also.

If you or others feel strongly about this, we can still tweak it, of
course.

I also moved the min/max #defines to replication/basebackup.h, and
included that file in pg_basebackup.c. This avoids the duplicated
values. That file is okay to be included there.

> > If WL_POSTMASTER_DEATH is triggered, we should exit immediately like
> > other process does? This is not a problem of this patch. This problem exists
> > also in current master. But ISTM it's better to solve that together. Thought?
>
> Once we're careful about not missing signals, I think PM death should be
> noticed too. The backup functionality itself would probably manage to
> finish without postmaster, however it's executed under walsender process.
>
> Question is where !PostmasterIsAlive() check should be added. I think it
> should go to the main loop of perform_base_backup(), but that's probably
> not in the scope of this patch.

Feel free to submit patches about this.

Thanks for your patch, and the numerous reviewers who took part.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 02/27/2014 11:04 PM, Alvaro Herrera wrote:
> I pushed this patch with a few further tweaks. In your changes to
> address the above point, you made the suffix mandatory in the
> pg_basebackup -r option. This seemed a strange restriction, so I
> removed it. It seems more user-friendly to me to accept the value as
> being expressed in kilobytes per second without requiring the suffix to
> be there; the 'k' suffix is then also accepted and has no effect. I
> amended the docs to say that also.
>
> If you or others feel strongly about this, we can still tweak it, of
> course.

I'm used to assume the base unit if there's no suffix, but have no
objections against considering kB as the default. I see you adjusted
documentation too.

> I also moved the min/max #defines to replication/basebackup.h, and
> included that file in pg_basebackup.c. This avoids the duplicated
> values. That file is okay to be included there.

I kept in mind that pg_basebackup.c is not linked to the backend, but
you're right, mere inclusion is something else.

> Thanks for your patch, and the numerous reviewers who took part.

Thanks for committing - this is my first patch :-)

// Tony