agora inbox for [email protected]help / color / mirror / Atom feed
Slow I/O can break throttling of base backup 10+ messages / 5 participants [nested] [flat]
* Slow I/O can break throttling of base backup @ 2016-12-15 15:20 Magnus Hagander <[email protected]> 2016-12-15 16:21 ` Re: Slow I/O can break throttling of base backup Antonin Houska <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Magnus Hagander @ 2016-12-15 15:20 UTC (permalink / raw) To: pgsql-hackers Running pg_basebackup with a throttling of say 10M runs it into the risk of the I/O on the server actually being slower than pg_basebackup (I have preproduced similar issues on fake-slow disks with lower rate limits). What happens in this case in basebackup.c is that the value for "sleep" comes out negative. This means we don't sleep, which is fine. However, that also means we don't set throttle_last. That means that the next time we come around to throttle(), the value for "elapsed" is even bigger, which results in an even bigger negative number, and we're "stuck". AFAICT this means that a temporary I/O spike that makes reading of the disk too slow can leave us in a situation where we never recover, and thus never end up sleeping ever again, effectively turning off rate limiting. I wonder if the else if (sleep > 0) at the bottom of throttle() should just be a simple else and always run, resetting last_throttle? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Slow I/O can break throttling of base backup 2016-12-15 15:20 Slow I/O can break throttling of base backup Magnus Hagander <[email protected]> @ 2016-12-15 16:21 ` Antonin Houska <[email protected]> 2016-12-16 10:24 ` Re: Slow I/O can break throttling of base backup Antonin Houska <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Antonin Houska @ 2016-12-15 16:21 UTC (permalink / raw) To: pgsql-hackers It seems to be my bug. I'll check tomorrow. Magnus Hagander <[email protected]> wrote: > Running pg_basebackup with a throttling of say 10M runs it into the risk of > the I/O on the server actually being slower than pg_basebackup (I have > preproduced similar issues on fake-slow disks with lower rate limits). > > What happens in this case in basebackup.c is that the value for "sleep" > comes out negative. This means we don't sleep, which is fine. > > However, that also means we don't set throttle_last. > > That means that the next time we come around to throttle(), the value for > "elapsed" is even bigger, which results in an even bigger negative number, > and we're "stuck". > > AFAICT this means that a temporary I/O spike that makes reading of the disk > too slow can leave us in a situation where we never recover, and thus never > end up sleeping ever again, effectively turning off rate limiting. > > I wonder if the else if (sleep > 0) at the bottom of throttle() should just > be a simple else and always run, resetting last_throttle? > > -- > Magnus Hagander > Me: http://www.hagander.net/ > Work: http://www.redpill-linpro.com/ > -- Antonin Houska Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt Web: http://www.postgresql-support.de, http://www.cybertec.at -- Sent via pgsql-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Slow I/O can break throttling of base backup 2016-12-15 15:20 Slow I/O can break throttling of base backup Magnus Hagander <[email protected]> 2016-12-15 16:21 ` Re: Slow I/O can break throttling of base backup Antonin Houska <[email protected]> @ 2016-12-16 10:24 ` Antonin Houska <[email protected]> 2016-12-19 09:17 ` Re: Slow I/O can break throttling of base backup Magnus Hagander <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Antonin Houska @ 2016-12-16 10:24 UTC (permalink / raw) To: pgsql-hackers Antonin Houska <[email protected]> wrote: > It seems to be my bug. I'll check tomorrow. I could reproduce the problem by adding sufficient sleep time to the loop. > Magnus Hagander <[email protected]> wrote: >> I wonder if the else if (sleep > 0) at the bottom of throttle() should just >> be a simple else and always run, resetting last_throttle? I agree. In fact, I could simplify the code even more. Since (elapsed + sleep) almost equals to GetCurrentIntegerTimestamp(), we can use the following statement unconditionally (I think I tried too hard to avoid calling GetCurrentIntegerTimestamp too often in the original patch): throttled_last = GetCurrentIntegerTimestamp(); Thus we can also get rid of the "else" branch that clears both "sleep" and "wait_result". (The patch contains minor comment refinement that I found useful when seeing the code after years.) -- Antonin Houska Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt Web: http://www.postgresql-support.de, http://www.cybertec.at -- Sent via pgsql-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers Attachments: [text/x-diff] throttling.patch (1.4K, ../../27929.1481883870@localhost/2-throttling.patch) download | inline diff: diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c new file mode 100644 index ffc7e58..40b3c11 *** a/src/backend/replication/basebackup.c --- b/src/backend/replication/basebackup.c *************** throttle(size_t increment) *** 1370,1395 **** if (wait_result & WL_LATCH_SET) CHECK_FOR_INTERRUPTS(); } - else - { - /* - * The actual transfer rate is below the limit. A negative value - * would distort the adjustment of throttled_last. - */ - wait_result = 0; - sleep = 0; - } /* ! * Only a whole multiple of throttling_sample was processed. The rest will ! * be done during the next call of this function. */ throttling_counter %= throttling_sample; ! /* Once the (possible) sleep has ended, new period starts. */ ! if (wait_result & WL_TIMEOUT) ! throttled_last += elapsed + sleep; ! else if (sleep > 0) ! /* Sleep was necessary but might have been interrupted. */ ! throttled_last = GetCurrentIntegerTimestamp(); } --- 1370,1385 ---- if (wait_result & WL_LATCH_SET) CHECK_FOR_INTERRUPTS(); } /* ! * As we work with integers, only whole multiple of throttling_sample was ! * processed. The rest will be done during the next call of this function. */ throttling_counter %= throttling_sample; ! /* ! * Time interval for the remaining amount and possible next increments ! * starts now. ! */ ! throttled_last = GetCurrentIntegerTimestamp(); } ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Slow I/O can break throttling of base backup 2016-12-15 15:20 Slow I/O can break throttling of base backup Magnus Hagander <[email protected]> 2016-12-15 16:21 ` Re: Slow I/O can break throttling of base backup Antonin Houska <[email protected]> 2016-12-16 10:24 ` Re: Slow I/O can break throttling of base backup Antonin Houska <[email protected]> @ 2016-12-19 09:17 ` Magnus Hagander <[email protected]> 0 siblings, 0 replies; 10+ messages in thread From: Magnus Hagander @ 2016-12-19 09:17 UTC (permalink / raw) To: Antonin Houska <[email protected]>; +Cc: pgsql-hackers On Fri, Dec 16, 2016 at 11:24 AM, Antonin Houska <[email protected]> wrote: > Antonin Houska <[email protected]> wrote: > > > It seems to be my bug. I'll check tomorrow. > > I could reproduce the problem by adding sufficient sleep time to the > loop. > > > Magnus Hagander <[email protected]> wrote: > >> I wonder if the else if (sleep > 0) at the bottom of throttle() should > just > >> be a simple else and always run, resetting last_throttle? > > I agree. In fact, I could simplify the code even more. > > Since (elapsed + sleep) almost equals to GetCurrentIntegerTimestamp(), we > can > use the following statement unconditionally (I think I tried too hard to > avoid > calling GetCurrentIntegerTimestamp too often in the original patch): > > throttled_last = GetCurrentIntegerTimestamp(); > > Thus we can also get rid of the "else" branch that clears both "sleep" and > "wait_result". > > (The patch contains minor comment refinement that I found useful when > seeing > the code after years.) > > Thanks! Applied and backpatched. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ ^ permalink raw reply [nested|flat] 10+ messages in thread
* [PATCH 8/8] tweak costing for bloom/minmax-multi indexes @ 2020-08-07 13:53 Tomas Vondra <[email protected]> 0 siblings, 0 replies; 10+ messages in thread From: Tomas Vondra @ 2020-08-07 13:53 UTC (permalink / raw) --- src/backend/access/brin/brin_bloom.c | 1 + src/backend/access/brin/brin_minmax_multi.c | 1 + src/backend/utils/adt/selfuncs.c | 19 ++++++++++++++++++- src/include/access/brin_internal.h | 3 +++ 4 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/backend/access/brin/brin_bloom.c b/src/backend/access/brin/brin_bloom.c index e7ca100821..f64381b657 100644 --- a/src/backend/access/brin/brin_bloom.c +++ b/src/backend/access/brin/brin_bloom.c @@ -643,6 +643,7 @@ brin_bloom_opcinfo(PG_FUNCTION_ARGS) result = palloc0(MAXALIGN(SizeofBrinOpcInfo(1)) + sizeof(BloomOpaque)); + result->oi_ignore_correlation = true; result->oi_nstored = 1; result->oi_regular_nulls = true; result->oi_opaque = (BloomOpaque *) diff --git a/src/backend/access/brin/brin_minmax_multi.c b/src/backend/access/brin/brin_minmax_multi.c index f700700d46..5f637e0629 100644 --- a/src/backend/access/brin/brin_minmax_multi.c +++ b/src/backend/access/brin/brin_minmax_multi.c @@ -1301,6 +1301,7 @@ brin_minmax_multi_opcinfo(PG_FUNCTION_ARGS) result = palloc0(MAXALIGN(SizeofBrinOpcInfo(1)) + sizeof(MinmaxMultiOpaque)); + result->oi_ignore_correlation = true; result->oi_nstored = 1; result->oi_regular_nulls = true; result->oi_opaque = (MinmaxMultiOpaque *) diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c index 53d974125f..956d6f0a84 100644 --- a/src/backend/utils/adt/selfuncs.c +++ b/src/backend/utils/adt/selfuncs.c @@ -98,6 +98,7 @@ #include <math.h> #include "access/brin.h" +#include "access/brin_internal.h" #include "access/brin_page.h" #include "access/gin.h" #include "access/table.h" @@ -7348,7 +7349,8 @@ brincostestimate(PlannerInfo *root, IndexPath *path, double loop_count, double minimalRanges; double estimatedRanges; double selec; - Relation indexRel; + Relation indexRel = NULL; + TupleDesc tupdesc = NULL; ListCell *l; VariableStatData vardata; @@ -7370,6 +7372,7 @@ brincostestimate(PlannerInfo *root, IndexPath *path, double loop_count, */ indexRel = index_open(index->indexoid, NoLock); brinGetStats(indexRel, &statsData); + tupdesc = RelationGetDescr(indexRel); index_close(indexRel, NoLock); /* work out the actual number of ranges in the index */ @@ -7403,6 +7406,17 @@ brincostestimate(PlannerInfo *root, IndexPath *path, double loop_count, { IndexClause *iclause = lfirst_node(IndexClause, l); AttrNumber attnum = index->indexkeys[iclause->indexcol]; + FmgrInfo *opcInfoFn; + BrinOpcInfo *opcInfo; + Form_pg_attribute attr = TupleDescAttr(tupdesc, iclause->indexcol); + bool ignore_correlation; + + opcInfoFn = index_getprocinfo(indexRel, iclause->indexcol + 1, BRIN_PROCNUM_OPCINFO); + + opcInfo = (BrinOpcInfo *) + DatumGetPointer(FunctionCall1(opcInfoFn, attr->atttypid)); + + ignore_correlation = opcInfo->oi_ignore_correlation; /* attempt to lookup stats in relation for this index column */ if (attnum != 0) @@ -7473,6 +7487,9 @@ brincostestimate(PlannerInfo *root, IndexPath *path, double loop_count, if (sslot.nnumbers > 0) varCorrelation = Abs(sslot.numbers[0]); + if (ignore_correlation) + varCorrelation = 1.0; + if (varCorrelation > *indexCorrelation) *indexCorrelation = varCorrelation; diff --git a/src/include/access/brin_internal.h b/src/include/access/brin_internal.h index ee4d0706df..67aea62a02 100644 --- a/src/include/access/brin_internal.h +++ b/src/include/access/brin_internal.h @@ -30,6 +30,9 @@ typedef struct BrinOpcInfo /* Regular processing of NULLs in BrinValues? */ bool oi_regular_nulls; + /* Ignore correlation during cost estimation */ + bool oi_ignore_correlation; + /* Opaque pointer for the opclass' private use */ void *oi_opaque; -- 2.25.4 --jsivyprvf2oxfjz3-- ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: cfbot is failing all tests on FreeBSD/Meson builds @ 2024-01-13 00:39 Thomas Munro <[email protected]> 2024-01-13 00:51 ` Re: cfbot is failing all tests on FreeBSD/Meson builds Tom Lane <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Thomas Munro @ 2024-01-13 00:39 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: [email protected]; Peter Eisentraut <[email protected]> On Sat, Jan 13, 2024 at 9:32 AM Tom Lane <[email protected]> wrote: > It looks like every recent cfbot run has failed in the > FreeBSD-13-Meson build, even if it worked in other ones. > The symptoms are failures in the TAP tests that try to > use interactive_psql: > > Can't call method "slave" on an undefined value at /usr/local/lib/perl5/site_perl/IPC/Run.pm line 2889. > > I suspect that we are looking at some bug in IPC::Run that exists in > the version that that FreeBSD release has (but not, seemingly, > elsewhere in the community), and that was mostly harmless until > c53859295 made all Perl warnings fatal. Not sure what we want > to do about this. Right, I see this locally on my FreeBSD box. Reverting the fatal warnings thing doesn't help, it's more broken than that. I tried to understand https://github.com/cpan-authors/IPC-Run/blob/master/lib/IPC/Run.pm https://github.com/cpan-authors/IO-Tty/blob/master/Pty.pm but I am not good at perl. I think the error means that in ## Close all those temporary filehandles that the kids needed. for my $pty ( values %{ $self->{PTYS} } ) { close $pty->slave; } the variable $pty holds undef, so then we have to find out where undef was put into $self->{PTYS}. There is a place that does: ## Just flag the pyt's existence for now. It'll be ## converted to a real IO::Pty by _open_pipes. $self->{PTYS}->{$pty_id} = undef; We can see that this started a couple of days ago: https://github.com/postgres/postgres/commits/master/ But at older commits I see the problem locally too, so, yeah, it must be coming from where else. Looking at the relevant packages p5-IPC-Run and p5-IO-Tty I see that they recently moved to 20231003.0 and 1.18, respectively. Downgrading p5-IPC-Run to p5-IPC-Run-20220807.0.pkg was not enough. Downgrading p5-IO-Tty to p5-IO-Tty-1.17.pkg allowed psql/010_tab_completion to pass with either version of p5-IPC-Run. The answer may lie in the commits between those versions here: https://github.com/cpan-authors/IO-Tty/commits/master/ I see that Debian Bookwork is still using 1.17, which fits with your guess that FreeBSD is just breaking first because it is using a newer version. ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: cfbot is failing all tests on FreeBSD/Meson builds 2024-01-13 00:39 Re: cfbot is failing all tests on FreeBSD/Meson builds Thomas Munro <[email protected]> @ 2024-01-13 00:51 ` Tom Lane <[email protected]> 2024-01-13 00:57 ` Re: cfbot is failing all tests on FreeBSD/Meson builds Thomas Munro <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Tom Lane @ 2024-01-13 00:51 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: [email protected]; Peter Eisentraut <[email protected]> Thomas Munro <[email protected]> writes: > Looking at the relevant packages > p5-IPC-Run and p5-IO-Tty I see that they recently moved to 20231003.0 > and 1.18, respectively. Downgrading p5-IPC-Run to > p5-IPC-Run-20220807.0.pkg was not enough. Downgrading p5-IO-Tty to > p5-IO-Tty-1.17.pkg allowed psql/010_tab_completion to pass with either > version of p5-IPC-Run. > The answer may lie in the commits between those versions here: > https://github.com/cpan-authors/IO-Tty/commits/master/ > I see that Debian Bookwork is still using 1.17, which fits with your > guess that FreeBSD is just breaking first because it is using a newer > version. Yeah, I have nothing newer than 1.17 here either. Time for a bug report to IO::Tty's authors, I guess. regards, tom lane ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: cfbot is failing all tests on FreeBSD/Meson builds 2024-01-13 00:39 Re: cfbot is failing all tests on FreeBSD/Meson builds Thomas Munro <[email protected]> 2024-01-13 00:51 ` Re: cfbot is failing all tests on FreeBSD/Meson builds Tom Lane <[email protected]> @ 2024-01-13 00:57 ` Thomas Munro <[email protected]> 2024-01-30 04:06 ` Re: cfbot is failing all tests on FreeBSD/Meson builds Tom Lane <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Thomas Munro @ 2024-01-13 00:57 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: [email protected]; Peter Eisentraut <[email protected]> On Sat, Jan 13, 2024 at 1:51 PM Tom Lane <[email protected]> wrote: > Time for a bug report to IO::Tty's authors, I guess. Ahh, there is one: https://github.com/cpan-authors/IO-Tty/issues/38 In the meantime, will look into whether I can pin that package to 1.17 somewhere in the pipeline, hopefully later today... ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: cfbot is failing all tests on FreeBSD/Meson builds 2024-01-13 00:39 Re: cfbot is failing all tests on FreeBSD/Meson builds Thomas Munro <[email protected]> 2024-01-13 00:51 ` Re: cfbot is failing all tests on FreeBSD/Meson builds Tom Lane <[email protected]> 2024-01-13 00:57 ` Re: cfbot is failing all tests on FreeBSD/Meson builds Thomas Munro <[email protected]> @ 2024-01-30 04:06 ` Tom Lane <[email protected]> 2024-02-08 01:27 ` Re: cfbot is failing all tests on FreeBSD/Meson builds Thomas Munro <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Tom Lane @ 2024-01-30 04:06 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: [email protected]; Peter Eisentraut <[email protected]> Thomas Munro <[email protected]> writes: > On Sat, Jan 13, 2024 at 1:51 PM Tom Lane <[email protected]> wrote: >> Time for a bug report to IO::Tty's authors, I guess. > Ahh, there is one: https://github.com/cpan-authors/IO-Tty/issues/38 Just for the archives' sake: I hit this today on a fresh install of FreeBSD 14.0, which has pulled in p5-IO-Tty-1.18. Annoying... regards, tom lane ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: cfbot is failing all tests on FreeBSD/Meson builds 2024-01-13 00:39 Re: cfbot is failing all tests on FreeBSD/Meson builds Thomas Munro <[email protected]> 2024-01-13 00:51 ` Re: cfbot is failing all tests on FreeBSD/Meson builds Tom Lane <[email protected]> 2024-01-13 00:57 ` Re: cfbot is failing all tests on FreeBSD/Meson builds Thomas Munro <[email protected]> 2024-01-30 04:06 ` Re: cfbot is failing all tests on FreeBSD/Meson builds Tom Lane <[email protected]> @ 2024-02-08 01:27 ` Thomas Munro <[email protected]> 0 siblings, 0 replies; 10+ messages in thread From: Thomas Munro @ 2024-02-08 01:27 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: [email protected]; Peter Eisentraut <[email protected]> On Tue, Jan 30, 2024 at 5:06 PM Tom Lane <[email protected]> wrote: > Thomas Munro <[email protected]> writes: > > On Sat, Jan 13, 2024 at 1:51 PM Tom Lane <[email protected]> wrote: > >> Time for a bug report to IO::Tty's authors, I guess. > > > Ahh, there is one: https://github.com/cpan-authors/IO-Tty/issues/38 > > Just for the archives' sake: I hit this today on a fresh install > of FreeBSD 14.0, which has pulled in p5-IO-Tty-1.18. Annoying... FWIW here's what I did to downgrade: # remove the problematic version (also removes p5-IPC-Run) pkg remove -y p5-IO-Tty # fetch the known good 1.17 package and install it curl -O "https://pkg.freebsd.org/freebsd:14:x86:64/release_0/All/p5-IO-Tty-1.17.pkg"; pkg install -y p5-IO-Tty-1.17.pkg # put back p5-IPC-Run pkg install -y p5-IPC-Run # temporarily prevent future "pkg upgrade" from upgrading p5-IO-Tty pkg lock -y p5-IO-Tty ^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2024-02-08 01:27 UTC | newest] Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2016-12-15 15:20 Slow I/O can break throttling of base backup Magnus Hagander <[email protected]> 2016-12-15 16:21 ` Antonin Houska <[email protected]> 2016-12-16 10:24 ` Antonin Houska <[email protected]> 2016-12-19 09:17 ` Magnus Hagander <[email protected]> 2020-08-07 13:53 [PATCH 8/8] tweak costing for bloom/minmax-multi indexes Tomas Vondra <[email protected]> 2024-01-13 00:39 Re: cfbot is failing all tests on FreeBSD/Meson builds Thomas Munro <[email protected]> 2024-01-13 00:51 ` Re: cfbot is failing all tests on FreeBSD/Meson builds Tom Lane <[email protected]> 2024-01-13 00:57 ` Re: cfbot is failing all tests on FreeBSD/Meson builds Thomas Munro <[email protected]> 2024-01-30 04:06 ` Re: cfbot is failing all tests on FreeBSD/Meson builds Tom Lane <[email protected]> 2024-02-08 01:27 ` Re: cfbot is failing all tests on FreeBSD/Meson builds Thomas Munro <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox