Re: [HACKERS] Backup throttling
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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. +termBASE_BACKUP [literalLABEL/literal replaceable'label'/replaceable] [literalPROGRESS/literal] [literalFAST/literal] [literalWAL/literal] [literalNOWAIT/literal] [literalMAX_RATE/literal]/term It's better to add something like replaceable'rate'/replaceable just after literalMAX_RATE/literal. + para + literalMAX_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) diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml index 832524e..704b653 100644 --- a/doc/src/sgml/protocol.sgml +++ b/doc/src/sgml/protocol.sgml @@ -1772,7 +1772,7 @@ The commands accepted in walsender mode are: /varlistentry varlistentry -termBASE_BACKUP [literalLABEL/literal replaceable'label'/replaceable] [literalPROGRESS/literal] [literalFAST/literal] [literalWAL/literal] [literalNOWAIT/literal]/term +termBASE_BACKUP [literalLABEL/literal replaceable'label'/replaceable] [literalPROGRESS/literal] [literalFAST/literal] [literalWAL/literal] [literalNOWAIT/literal] [literalMAX_RATE/literal replaceable'rate'/replaceable]/term listitem para Instructs the server to start streaming a base backup. @@ -1840,7 +1840,20 @@ The commands accepted in walsender mode are: the waiting and the warning, leaving the client responsible for ensuring the required log is available. /para - /listitem +/listitem + /varlistentry + varlistentry +termliteralMAX_RATE/literal replaceablerate//term +listitem + para + Limit (throttle) the maximum amount of data transferred from server + to client per unit of time. The expected unit is kilobytes per second. + If this option is specified, the value must either be equal to zero + or it must fall within the range from 32 kB through 1 GB (inclusive). + If zero is passed or the option is not specified, no restriction is + imposed on the transfer. + /para +/listitem /varlistentry /variablelist /para diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml index c379df5..f8866db 100644 --- a/doc/src/sgml/ref/pg_basebackup.sgml +++ b/doc/src/sgml/ref/pg_basebackup.sgml @@ -189,6 +189,27 @@ PostgreSQL documentation /varlistentry varlistentry + termoption-r replaceable class=parameterrate/replaceable/option/term + termoption--max-rate=replaceable class=parameterrate/replaceable/option/term + listitem + para +The maximum amount of data transferred from server per second. +The purpose is to limit the impact of applicationpg_basebackup/application +on the running server. + /para + para +This option always affects transfer of the data directory. Transfer of +WAL files is only affected if the collection method is literalfetch/literal. + /para + para +Valid values are between literal32 kB/literal and literal1024 MB/literal. +Suffixes literalk/literal (kilobytes) and literalM/literal +(megabytes) are accepted. + /para + /listitem + /varlistentry + + varlistentry
Re: [HACKERS] Backup throttling
On Tue, Jan 21, 2014 at 1:10 AM, Antonin Houska antonin.hou...@gmail.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. +termBASE_BACKUP [literalLABEL/literal replaceable'label'/replaceable] [literalPROGRESS/literal] [literalFAST/literal] [literalWAL/literal] [literalNOWAIT/literal] [literalMAX_RATE/literal]/term It's better to add something like replaceable'rate'/replaceable just after literalMAX_RATE/literal. + para + literalMAX_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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 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) diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml index 7d99976..799d214 100644 --- a/doc/src/sgml/protocol.sgml +++ b/doc/src/sgml/protocol.sgml @@ -1720,7 +1720,7 @@ The commands accepted in walsender mode are: /varlistentry varlistentry -termBASE_BACKUP [literalLABEL/literal replaceable'label'/replaceable] [literalPROGRESS/literal] [literalFAST/literal] [literalWAL/literal] [literalNOWAIT/literal]/term +termBASE_BACKUP [literalLABEL/literal replaceable'label'/replaceable] [literalPROGRESS/literal] [literalFAST/literal] [literalWAL/literal] [literalNOWAIT/literal] [literalMAX_RATE/literal]/term listitem para Instructs the server to start streaming a base backup. @@ -1788,7 +1788,23 @@ The commands accepted in walsender mode are: the waiting and the warning, leaving the client responsible for ensuring the required log is available. /para - /listitem +/listitem + /varlistentry + varlistentry +termliteralMAX_RATE/literal replaceablerate//term +listitem + para + Limit (throttle) the maximum amount of data transferred from server + to client per unit of time. The expected unit is bytes per second. + If this option is specified, the value must either be equal to zero + or it must fall within the range from 32 kB through 1 GB (inclusive). + If zero is passed or the option is not specified, no restriction is + imposed on the transfer. + /para + para + literalMAX_RATE/literal does not affect WAL streaming. + /para +/listitem /varlistentry /variablelist /para diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml index c379df5..2ec81b7 100644 --- a/doc/src/sgml/ref/pg_basebackup.sgml +++ b/doc/src/sgml/ref/pg_basebackup.sgml @@ -189,6 +189,27 @@ PostgreSQL documentation /varlistentry varlistentry + termoption-r replaceable class=parameterrate/replaceable/option/term + termoption--max-rate=replaceable
Re: [HACKERS] Backup throttling
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
On Thu, Jan 16, 2014 at 12:03 PM, Andres Freund and...@2ndquadrant.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
On Fri, Jan 17, 2014 at 5:03 AM, Andres Freund and...@2ndquadrant.com wrote: slightly related: we should start to reuse procLatch for walsenders instead of having a separate latch someday. + 1 on that. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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 z...@cybertec.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:
+ termoption-r/option/term
+ termoption--max-rate/option/term
You need to add something like replaceable
class=parameterrate/replaceable.
+The purpose is to limit impact of
applicationpg_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,
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 0b2e60e..2f24fff 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -1719,7 +1719,7 @@ The commands accepted in walsender mode are:
/varlistentry
varlistentry
-termBASE_BACKUP [literalLABEL/literal replaceable'label'/replaceable] [literalPROGRESS/literal] [literalFAST/literal] [literalWAL/literal] [literalNOWAIT/literal]/term
+termBASE_BACKUP [literalLABEL/literal replaceable'label'/replaceable] [literalPROGRESS/literal] [literalFAST/literal] [literalWAL/literal] [literalNOWAIT/literal] [literalMAX_RATE/literal]/term
listitem
para
Instructs the server to start streaming a base backup.
@@ -1787,7 +1787,23 @@ The commands accepted in walsender mode are:
the waiting and the warning, leaving the client responsible for
ensuring the required log is available.
/para
- /listitem
+/listitem
+ /varlistentry
+ varlistentry
+termliteralMAX_RATE/literal/term
+listitem
+ para
+ Limit the maximum amount of data transferred to client per unit of
+ time. The expected unit is bytes per second. If
+ literalMAX_RATE/literal is passed, it must be either equal to
+ zero or fall to range 32 kB through 1 GB (inclusive). If zero is
+ passed or the option is not passed at all, no restriction is imposed
+ on the transfer.
+ /para
+ para
+ literalMAX_RATE/literal does not affect WAL streaming.
+ /para
+/listitem
/varlistentry
/variablelist
/para
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index c379df5..caede77 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -189,6 +189,28 @@ PostgreSQL documentation
/varlistentry
varlistentry
+ termoption-r replaceable class=parameterrate/replaceable/option/term
+ termoption--max-rate=replaceable class=parameterrate/replaceable/option/term
+ listitem
+ para
+The maximum amount of data transferred from server per second.
+The purpose is to limit impact of applicationpg_basebackup/application
+on running server.
+ /para
+ para
+This option always affects transfer of the data directory. Transfer of
+WAL files is only affected if the collection method is literalfetch/literal.
+ /para
+ para
+Value between literal32 kB/literal and literal1024 MB/literal
+is expected. Suffixes literalk/literal (kilobytes) and
+literalM/literal (megabytes) are accepted. For example:
+literal10M/literal
+ /para
+ /listitem
+ /varlistentry
+
+ varlistentry
Re: [HACKERS] Backup throttling
On Fri, Dec 6, 2013 at 6:43 PM, Boszormenyi Zoltan z...@cybertec.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:
+ termoption-r/option/term
+ termoption--max-rate/option/term
You need to add something like replaceable
class=parameterrate/replaceable.
+The purpose is to limit impact of
applicationpg_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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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 real0m26.508s user0m0.054s sys0m0.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 diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml index c379df5..e878592 100644 --- a/doc/src/sgml/ref/pg_basebackup.sgml +++ b/doc/src/sgml/ref/pg_basebackup.sgml @@ -189,6 +189,26 @@ PostgreSQL documentation /varlistentry varlistentry + termoption-r/option/term + termoption--max-rate/option/term + listitem + para +The maximum amount of data transferred from server per second. +The purpose is to limit impact of applicationpg_basebackup/application +on a running master server. + /para + para +This option always affects transfer of the data directory. Transfer of +WAL files is only affected if the collection method is literalfetch/literal. + /para + para +Suffixes literalk/literal (kilobytes) and literalM/literal +(megabytes) are accepted. For example: literal10M/literal + /para + /listitem + /varlistentry + + varlistentry termoption-R/option/term termoption--write-recovery-conf/option/term listitem diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c index ba8d173..f194746 100644 --- a/src/backend/replication/basebackup.c +++ b/src/backend/replication/basebackup.c @@ -33,6 +33,7 @@ #include utils/builtins.h #include utils/elog.h #include utils/ps_status.h +#include utils/timestamp.h #include pgtar.h typedef struct @@ -42,6 +43,7 @@ typedef struct bool fastcheckpoint; bool nowait; bool includewal; + uint32 maxrate; } basebackup_options; @@ -59,6 +61,7 @@ static void perform_base_backup(basebackup_options *opt, DIR *tblspcdir); static void parse_basebackup_options(List *options, basebackup_options *opt); static void SendXlogRecPtrResult(XLogRecPtr ptr, TimeLineID tli); static int compareWalFileNames(const void *a, const void *b); +static void throttle(size_t increment); /* Was the backup currently in-progress initiated in recovery mode? */ static bool backup_started_in_recovery = false; @@ -68,6 +71,41 @@ static bool backup_started_in_recovery = false; */ #define TAR_SEND_SIZE 32768 + +/* + * The maximum amount of data per second - bounds of the user input. + * + * If the maximum should be increased to more than 4 GB, uint64 must + * be introduced for the related variables. However such high values have + * little to do with throttling. + */ +#define MAX_RATE_LOWER 32768 +#define MAX_RATE_UPPER (1024 20) + +/* + * Transfer rate is only measured when this number of bytes has been sent. + * (Too frequent checks would impose too high CPU overhead.) + * + * The default value is used unless it'd result in too frequent checks. + */ +#define THROTTLING_SAMPLE_MIN 32768 + +/* The maximum number of checks per
Re: [HACKERS] Backup throttling
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.gc5...@eldon.alvh.no-ip.org As for units http://www.postgresql.org/message-id/20130903224127.gd7...@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.ga18...@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 real0m26.508s user0m0.054s sys0m0.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 +++
Re: [HACKERS] Backup throttling
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.gc5...@eldon.alvh.no-ip.org As for units http://www.postgresql.org/message-id/20130903224127.gd7...@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.ga18...@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) diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml index eb0c1d6..882a26b 100644 --- a/doc/src/sgml/ref/pg_basebackup.sgml +++ b/doc/src/sgml/ref/pg_basebackup.sgml @@ -189,6 +189,22 @@ PostgreSQL documentation /varlistentry varlistentry + termoption-r/option/term + termoption--max-rate/option/term + listitem + para +The maximum amount of data transferred from server per second. +The purpose is to limit impact of applicationpg_basebackup/application +on a running master server while transfering data directory. + /para + para +Suffixes literalk/literal (kilobytes) and literalM/literal +(megabytes) are accepted. For example: literal10M/literal + /para + /listitem + /varlistentry + + varlistentry termoption-R/option/term termoption--write-recovery-conf/option/term listitem diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c index ba8d173..b2f10c3 100644 --- a/src/backend/replication/basebackup.c +++ b/src/backend/replication/basebackup.c @@ -33,6 +33,7 @@ #include utils/builtins.h #include utils/elog.h #include utils/ps_status.h +#include utils/timestamp.h #include pgtar.h typedef struct @@ -42,6 +43,7 @@ typedef struct bool fastcheckpoint; bool nowait; bool includewal; + uint32 maxrate; } basebackup_options; @@ -59,6 +61,7 @@ static void perform_base_backup(basebackup_options *opt, DIR *tblspcdir); static void parse_basebackup_options(List *options, basebackup_options *opt); static void SendXlogRecPtrResult(XLogRecPtr ptr, TimeLineID tli); static int compareWalFileNames(const void *a, const void *b); +static void throttle(size_t increment); /* Was the backup currently in-progress initiated in recovery mode? */ static bool backup_started_in_recovery = false; @@ -68,6 +71,41 @@ static bool backup_started_in_recovery = false; */ #define TAR_SEND_SIZE 32768 + +/* + * The maximum amount of data per second - bounds of the user input. + * + * If the maximum should be increased to more than 4 GB, uint64 must + * be introduced for the related variables. However such high values have + * little to do with throttling. + */ +#define MAX_RATE_LOWER 32768 +#define MAX_RATE_UPPER (1024 20) + +/* + * Transfer rate is only measured when this number of bytes has been sent. + * (Too frequent checks would impose too high CPU overhead.) + * + * The default value is used unless it'd result in too frequent checks. + */ +#define THROTTLING_SAMPLE_MIN 32768 + +/* The maximum number of checks per second. */ +#define THROTTLING_MAX_FREQUENCY 128 + +/* The actual value, transfer of which may cause sleep. */ +static uint32 throttling_sample; + +/* Amount of data already transfered but not yet throttled. */ +static int32 throttling_counter; + +/* The minimum time required to transfer throttling_sample bytes. */
Re: [HACKERS] Backup throttling
On Tue, Sep 3, 2013 at 8:35 AM, Antonin Houska antonin.hou...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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)
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index eb0c1d6..f58eb60 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -189,6 +189,22 @@ PostgreSQL documentation
/varlistentry
varlistentry
+ termoption-r/option/term
+ termoption--max-rate/option/term
+ listitem
+ para
+The maximum amount of data transferred from server per second.
+The purpose is to limit impact of applicationpg_basebackup/application
+on a running master server while transfering data directory.
+ /para
+ para
+Suffixes literalk/literal (kilobytes) and literalm/literal
+(megabytes) are accepted. For example: literal10m/literal
+ /para
+ /listitem
+ /varlistentry
+
+ varlistentry
termoption-R/option/term
termoption--write-recovery-conf/option/term
listitem
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index ba8d173..08e4f26 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -33,6 +33,7 @@
#include utils/builtins.h
#include utils/elog.h
#include utils/ps_status.h
+#include utils/timestamp.h
#include pgtar.h
typedef struct
@@ -42,6 +43,7 @@ typedef struct
bool fastcheckpoint;
bool nowait;
bool includewal;
+ uint32 maxrate;
} basebackup_options;
@@ -59,6 +61,7 @@ static void perform_base_backup(basebackup_options *opt, DIR *tblspcdir);
static void parse_basebackup_options(List *options, basebackup_options *opt);
static void SendXlogRecPtrResult(XLogRecPtr ptr, TimeLineID tli);
static int compareWalFileNames(const void *a, const void *b);
+static void throttle(size_t increment);
/* Was the backup currently in-progress initiated in recovery mode? */
static bool backup_started_in_recovery = false;
@@ -68,6 +71,41 @@ static bool backup_started_in_recovery = false;
*/
#define TAR_SEND_SIZE 32768
+
+/*
+ * The maximum amount of data per second - bounds of the user input.
+ *
+ * If the maximum should be increased to more than 4 GB, uint64 must
+ * be introduced for the related variables. However such high values have
+ * little to do with throttling.
+ */
+#define MAX_RATE_LOWER 32768
+#define MAX_RATE_UPPER (1024 20)
+
+/*
+ * Transfer rate is only measured when this number of bytes has been sent.
+ * (Too frequent checks would impose too high CPU overhead.)
+ *
+ * The default value is used unless it'd result in too frequent checks.
+ */
+#define THROTTLING_SAMPLE_MIN 32768
+
+/* The maximum number of checks per second. */
+#define THROTTLING_MAX_FREQUENCY 256
+
+/* The actual value, transfer of which may cause sleep. */
+static uint32 throttling_sample;
+
+/* Amount of data already transfered but not yet throttled. */
+static int32 throttling_counter;
+
+/* The minimum time required to transfer throttling_sample bytes. */
+static int64 elapsed_min_unit;
+
+/* The last check of the transfer rate. */
+static int64 throttled_last;
+
+
typedef struct
{
char *oid;
@@ -171,6 +209,31 @@ perform_base_backup(basebackup_options *opt, DIR *tblspcdir)
/* Send tablespace header */
SendBackupHeader(tablespaces);
+ if (opt-maxrate 0)
+ {
+ throttling_sample = opt-maxrate / THROTTLING_MAX_FREQUENCY;
+
+ /* Don't measure too small pieces of data. */
+ if (throttling_sample THROTTLING_SAMPLE_MIN)
+throttling_sample = THROTTLING_SAMPLE_MIN;
+
+ /*
+ * opt-maxrate is bytes per seconds. Thus the expression in
+ * brackets is microseconds per byte.
+ */
+ elapsed_min_unit = throttling_sample *
+((double) USECS_PER_SEC / opt-maxrate);
+
+ /* Enable throttling. */
+ throttling_counter = 0;
+
+ /* The 'real data' starts now (header was ignored). */
+ throttled_last = GetCurrentIntegerTimestamp();
+ }
+ else
+ /* Disable throttling. */
+ throttling_counter = -1;
+
/* Send off our tablespaces one by one */
foreach(lc, tablespaces)
{
@@ -468,6 +531,7 @@ parse_basebackup_options(List *options, basebackup_options *opt)
bool o_fast = false;
bool o_nowait = false;
bool o_wal = false;
+ bool o_maxrate = false;
MemSet(opt, 0, sizeof(*opt));
foreach(lopt, options)
@@ -519,6 +583,29 @@ parse_basebackup_options(List *options, basebackup_options *opt)
opt-includewal = true;
o_wal = true;
}
+ else if (strcmp(defel-defname, maxrate) == 0)
+ {
+ long maxrate;
+
+ if (o_maxrate)
+ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg(duplicate option \%s\, defel-defname)));
+ maxrate = intVal(defel-arg);
+
+ opt-maxrate = (uint32) maxrate;
+ if (opt-maxrate 0
+ (opt-maxrate MAX_RATE_LOWER || opt-maxrate
Re: [HACKERS] Backup throttling
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
Antonin Houska wrote:
+ para
+Suffixes literalk/literal (kilobytes) and literalm/literal
+(megabytes) are accepted. For example: literal10m/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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
On 2013-09-03 12:56:53 -0400, Alvaro Herrera wrote: Antonin Houska wrote: + para +Suffixes literalk/literal (kilobytes) and literalm/literal +(megabytes) are accepted. For example: literal10m/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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
On Tue, Aug 20, 2013 at 2:37 AM, Heikki Linnakangas hlinnakan...@vmware.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
On Tue, Aug 27, 2013 at 12:58 PM, Robert Haas robertmh...@gmail.com wrote: On Tue, Aug 20, 2013 at 2:37 AM, Heikki Linnakangas hlinnakan...@vmware.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.
Re: [HACKERS] Backup throttling
On 08/27/2013 01:58 PM, Robert Haas wrote: On Tue, Aug 20, 2013 at 2:37 AM, Heikki Linnakangas hlinnakan...@vmware.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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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.5040...@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 USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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Ü -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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.gx14...@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/
Re: [HACKERS] Backup throttling
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
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.gx14...@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
Re: [HACKERS] Backup throttling
On Wed, 31 Jul 2013 22:50:19 +0200 Antonin Houska antonin.hou...@gmail.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.gx14...@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 test.sh Description: application/shellscript -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Backup throttling
On Wed, 24 Jul 2013 09:20:52 +0200 Antonin Houska antonin.hou...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Backup throttling
Hello,
the purpose of this patch is to limit impact of pg_backup on running
server. Feedback is appreciated.
// Antonin Houska (Tony)
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index eb0c1d6..3b7ecfd 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -189,6 +189,22 @@ PostgreSQL documentation
/varlistentry
varlistentry
+ termoption-r/option/term
+ termoption--max-rate/option/term
+ listitem
+ para
+ The maximum amount of data transferred from server per second.
+ The purpose is to limit impact of
+ applicationpg_basebackup/application on a running master server.
+ /para
+ para
+ Suffixes literalk/literal (kilobytes) and literalm/literal
+ (megabytes) are accepted. For example: literal10m/literal
+ /para
+ /listitem
+ /varlistentry
+
+ varlistentry
termoption-R/option/term
termoption--write-recovery-conf/option/term
listitem
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index a1e12a8..7fb2d78 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -45,11 +45,23 @@ bool streamwal = false;
bool fastcheckpoint = false;
bool writerecoveryconf = false;
int standby_message_timeout = 10 * 1000; /* 10 sec = default */
+double max_rate = 0; /* Maximum bytes per second. 0 implies
+ * unlimited rate. */
+
+#define MAX_RATE_LOWER 0x8000 /* 32 kB */
+#define MAX_RATE_UPPER 0x4000 /* 1 GB */
+/*
+ * We shouldn't measure time whenever a small piece of data (e.g. TAR file
+ * header) has arrived. That would introduce high CPU overhead.
+ */
+#define RATE_MIN_SAMPLE 32768
/* Progress counters */
static uint64 totalsize;
static uint64 totaldone;
+static uint64 last_measured;
static int tablespacecount;
+static int64 last_measured_time;
/* Pipe to communicate with background wal receiver process */
#ifndef WIN32
@@ -72,9 +84,14 @@ static volatile LONG has_xlogendptr = 0;
static PQExpBuffer recoveryconfcontents = NULL;
/* Function headers */
+
+
+
static void usage(void);
static void verify_dir_is_empty_or_create(char *dirname);
static void progress_report(int tablespacenum, const char *filename);
+static double parse_max_rate(char *src);
+static void enforce_max_rate();
static void ReceiveTarFile(PGconn *conn, PGresult *res, int rownum);
static void ReceiveAndUnpackTarFile(PGconn *conn, PGresult *res, int rownum);
@@ -110,6 +127,7 @@ usage(void)
printf(_(\nOptions controlling the output:\n));
printf(_( -D, --pgdata=DIRECTORY receive base backup into directory\n));
printf(_( -F, --format=p|t output format (plain (default), tar)\n));
+ printf(_( -r, --max-rate maximum transfer rate between server and client\n));
printf(_( -R, --write-recovery-conf\n
write recovery.conf after backup\n));
printf(_( -x, --xlog include required WAL files in backup (fetch mode)\n));
@@ -473,6 +491,113 @@ progress_report(int tablespacenum, const char *filename)
fprintf(stderr, \r);
}
+static double
+parse_max_rate(char *src)
+{
+ int factor;
+ char *after_num;
+ double result;
+
+ result = strtod(src, after_num);
+ if (src == after_num)
+ {
+ fprintf(stderr, _(%s: invalid transfer rate \%s\\n), progname, src);
+ exit(1);
+ }
+
+ /*
+ * 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 'm':
+ factor = 10;
+ case 'k':
+ factor = 10;
+ after_num++;
+ break;
+
+ default:
+
+ /*
+ * If there's no suffix, byte is the unit. Possible invalid letter
+ * will make conversion to integer fail.
+ */
+ break;
+ }
+
+ /* The rest can only consist of white space. */
+ while (*after_num != '\0')
+ {
+ if (!isspace(*after_num))
+ {
+ fprintf(stderr, _(%s: invalid transfer rate \%s\\n), progname, src);
+ exit(1);
+ }
+ after_num++;
+ }
+
+ result *= factor;
+ if (result MAX_RATE_LOWER || result MAX_RATE_UPPER)
+ {
+ fprintf(stderr, _(%s: transfer rate out of range \%s\\n), progname, src);
+ exit(1);
+ }
+ return result;
+}
+
+/*
+ * If the progress is more than what max_rate allows, sleep.
+ *
+ * Do not call if max_rate == 0.
+ */
+static void
+enforce_max_rate()
+{
+ int64 min_elapsed,
+now,
+elapsed,
+to_sleep;
+
+ int64 last_chunk;
+
+ last_chunk = totaldone - last_measured;
+
+ /* The measurements shouldn't be more frequent then necessary. */
+ if (last_chunk RATE_MIN_SAMPLE)
+ return;
+
+
+ now = localGetCurrentTimestamp();
+ elapsed = now - last_measured_time;
+
+ /*
+ *
Re: [HACKERS] Backup throttling
On Wed, Jul 24, 2013 at 4:20 PM, Antonin Houska antonin.hou...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
