Re: Suppressing useless wakeups in walreceiver

2022-11-16 Thread Thomas Munro
On Wed, Nov 16, 2022 at 5:24 PM Nathan Bossart wrote: > On Wed, Nov 16, 2022 at 04:57:08PM +1300, Thomas Munro wrote: > > On Tue, Nov 15, 2022 at 5:49 PM Nathan Bossart > > wrote: > >> Another option might be to just force initial reply/feedback messages when > >> streaming starts. The

Re: Suppressing useless wakeups in walreceiver

2022-11-15 Thread Nathan Bossart
On Wed, Nov 16, 2022 at 04:57:08PM +1300, Thomas Munro wrote: > On Tue, Nov 15, 2022 at 5:49 PM Nathan Bossart > wrote: >> Another option might be to just force initial reply/feedback messages when >> streaming starts. The attached patch improves src/test/recovery test >> runtime just like the

Re: Suppressing useless wakeups in walreceiver

2022-11-15 Thread Thomas Munro
On Tue, Nov 15, 2022 at 5:49 PM Nathan Bossart wrote: > Another option might be to just force initial reply/feedback messages when > streaming starts. The attached patch improves src/test/recovery test > runtime just like the previous one I posted. Yeah, looks good in my tests. I think we just

Re: Suppressing useless wakeups in walreceiver

2022-11-14 Thread Nathan Bossart
Another option might be to just force initial reply/feedback messages when streaming starts. The attached patch improves src/test/recovery test runtime just like the previous one I posted. On Mon, Nov 14, 2022 at 11:01:27AM -0800, Nathan Bossart wrote: > I wonder if we > ought to do something

Re: Suppressing useless wakeups in walreceiver

2022-11-14 Thread Thomas Munro
On Tue, Nov 15, 2022 at 12:14 PM Nathan Bossart wrote: > On Tue, Nov 15, 2022 at 09:42:26AM +1300, Thomas Munro wrote: > > That works for 020_pg_receivewal. I wonder if there are also tests > > that stream a bit of WAL first and then do wait_for_catchup that were > > previously benefiting from

Re: Suppressing useless wakeups in walreceiver

2022-11-14 Thread Nathan Bossart
On Tue, Nov 15, 2022 at 09:42:26AM +1300, Thomas Munro wrote: > That works for 020_pg_receivewal. I wonder if there are also tests > that stream a bit of WAL first and then do wait_for_catchup that were > previously benefiting from the 100ms-after-startup message by > scheduling luck (as in, that

Re: Suppressing useless wakeups in walreceiver

2022-11-14 Thread Thomas Munro
On Tue, Nov 15, 2022 at 8:01 AM Nathan Bossart wrote: > Is there any reason we should wait for 100ms before sending the initial > reply? ISTM the previous behavior essentially caused the first reply (and > feedback message) to be sent at the first opportunity after streaming > begins. My

Re: Suppressing useless wakeups in walreceiver

2022-11-14 Thread Nathan Bossart
On Mon, Nov 14, 2022 at 02:47:14PM +1300, Thomas Munro wrote: > Ahh, I think I might have it. In the old coding, sendTime starts out > as 0, which I think caused it to send its first reply message after > the first 100ms sleep, and only after that fall into a cadence of >

Re: Suppressing useless wakeups in walreceiver

2022-11-13 Thread Thomas Munro
On Mon, Nov 14, 2022 at 1:05 PM Nathan Bossart wrote: > Yeah, I'm able to sort-of reproduce the problem by sending fewer replies, > but it's not clear to me why that's the problem. AFAICT all of the code > paths that write/flush are careful to send a reply shortly afterward, which > should keep

Re: Suppressing useless wakeups in walreceiver

2022-11-13 Thread Nathan Bossart
On Mon, Nov 14, 2022 at 12:51:14PM +1300, Thomas Munro wrote: > On Mon, Nov 14, 2022 at 12:35 PM Thomas Munro wrote: >> Maybe there is a better way to code this (I mean, who likes global >> variables?) and I need to test some more, but I suspect the attached >> is approximately what we missed. >

Re: Suppressing useless wakeups in walreceiver

2022-11-13 Thread Thomas Munro
On Mon, Nov 14, 2022 at 12:35 PM Thomas Munro wrote: > Maybe there is a better way to code this (I mean, who likes global > variables?) and I need to test some more, but I suspect the attached > is approximately what we missed. Erm, ENOCOFFEE, sorry that's not quite right, it needs the apply

Re: Suppressing useless wakeups in walreceiver

2022-11-13 Thread Thomas Munro
On Mon, Nov 14, 2022 at 12:11 PM Thomas Munro wrote: > On Mon, Nov 14, 2022 at 11:26 AM Nathan Bossart > wrote: > > On Sun, Nov 13, 2022 at 05:08:04PM -0500, Tom Lane wrote: > > > There is something very seriously wrong with this patch. > > > > > > On my machine, running "make -j10 check-world"

Re: Suppressing useless wakeups in walreceiver

2022-11-13 Thread Thomas Munro
On Mon, Nov 14, 2022 at 11:26 AM Nathan Bossart wrote: > On Sun, Nov 13, 2022 at 05:08:04PM -0500, Tom Lane wrote: > > There is something very seriously wrong with this patch. > > > > On my machine, running "make -j10 check-world" (with compilation > > already done) has been taking right about 2

Re: Suppressing useless wakeups in walreceiver

2022-11-13 Thread Nathan Bossart
On Sun, Nov 13, 2022 at 05:08:04PM -0500, Tom Lane wrote: > There is something very seriously wrong with this patch. > > On my machine, running "make -j10 check-world" (with compilation > already done) has been taking right about 2 minutes for some time. > Since this patch, it's taking around

Re: Suppressing useless wakeups in walreceiver

2022-11-13 Thread Thomas Munro
On Mon, Nov 14, 2022 at 11:08 AM Tom Lane wrote: > There is something very seriously wrong with this patch. > > On my machine, running "make -j10 check-world" (with compilation > already done) has been taking right about 2 minutes for some time. > Since this patch, it's taking around 2:45 --- I

Re: Suppressing useless wakeups in walreceiver

2022-11-13 Thread Tom Lane
Thomas Munro writes: > And with that change and a pgindent, pushed. There is something very seriously wrong with this patch. On my machine, running "make -j10 check-world" (with compilation already done) has been taking right about 2 minutes for some time. Since this patch, it's taking around

Re: Suppressing useless wakeups in walreceiver

2022-11-08 Thread Bharath Rupireddy
On Wed, Nov 9, 2022 at 2:38 AM Nathan Bossart wrote: > > On Tue, Nov 08, 2022 at 09:45:40PM +1300, Thomas Munro wrote: > > On Tue, Nov 8, 2022 at 9:20 PM Bharath Rupireddy > > wrote: > >> Thanks. Do we need a similar wakeup approach for logical replication > >> workers in worker.c? Or is it okay

Re: Suppressing useless wakeups in walreceiver

2022-11-08 Thread Nathan Bossart
On Tue, Nov 08, 2022 at 09:45:40PM +1300, Thomas Munro wrote: > On Tue, Nov 8, 2022 at 9:20 PM Bharath Rupireddy > wrote: >> Thanks. Do we need a similar wakeup approach for logical replication >> workers in worker.c? Or is it okay that the nap time is 1sec there? > > Yeah, I think so. At least

Re: Suppressing useless wakeups in walreceiver

2022-11-08 Thread Nathan Bossart
On Tue, Nov 08, 2022 at 08:46:26PM +1300, Thomas Munro wrote: > On Tue, Nov 8, 2022 at 3:20 PM Thomas Munro wrote: >> This looks pretty good to me. Thanks for picking it up! I can live >> with the change to use a global variable; it seems we couldn't quite >> decide what the scope was for

Re: Suppressing useless wakeups in walreceiver

2022-11-08 Thread Thomas Munro
On Tue, Nov 8, 2022 at 9:20 PM Bharath Rupireddy wrote: > Thanks. Do we need a similar wakeup approach for logical replication > workers in worker.c? Or is it okay that the nap time is 1sec there? Yeah, I think so. At least for its idle/nap case (waiting for flush is also a technically

Re: Suppressing useless wakeups in walreceiver

2022-11-08 Thread Bharath Rupireddy
On Tue, Nov 8, 2022 at 1:17 PM Thomas Munro wrote: > > And with that change and a pgindent, pushed. Thanks. Do we need a similar wakeup approach for logical replication workers in worker.c? Or is it okay that the nap time is 1sec there? -- Bharath Rupireddy PostgreSQL Contributors Team RDS

Re: Suppressing useless wakeups in walreceiver

2022-11-07 Thread Thomas Munro
On Tue, Nov 8, 2022 at 3:20 PM Thomas Munro wrote: > On Sun, Nov 6, 2022 at 12:01 PM Nathan Bossart > wrote: > > Here is a new version of the patch that addresses this feedback. > > This looks pretty good to me. Thanks for picking it up! I can live > with the change to use a global variable;

Re: Suppressing useless wakeups in walreceiver

2022-11-07 Thread Thomas Munro
On Sun, Nov 6, 2022 at 12:01 PM Nathan Bossart wrote: > Here is a new version of the patch that addresses this feedback. This looks pretty good to me. Thanks for picking it up! I can live with the change to use a global variable; it seems we couldn't quite decide what the scope was for moving

Re: Suppressing useless wakeups in walreceiver

2022-11-05 Thread Nathan Bossart
On Sat, Nov 05, 2022 at 03:00:55PM -0700, Nathan Bossart wrote: > On Mon, Oct 17, 2022 at 03:21:18PM +0900, Kyotaro Horiguchi wrote: >> Now that I see the fix for the implicit conversion: >> >> L527: + nap = Max(0, (nextWakeup - now + 999) / >> 1000); >> .. >> L545:

Re: Suppressing useless wakeups in walreceiver

2022-11-05 Thread Nathan Bossart
On Wed, Oct 26, 2022 at 03:05:20PM +0530, Bharath Rupireddy wrote: > A comment on the patch: > Isn't it better we track the soonest wakeup time or min of wakeup[] > array (update the min whenever the array element is updated in > WalRcvComputeNextWakeup()) instead of recomputing everytime by

Re: Suppressing useless wakeups in walreceiver

2022-11-05 Thread Nathan Bossart
On Mon, Oct 17, 2022 at 03:21:18PM +0900, Kyotaro Horiguchi wrote: > Now that I see the fix for the implicit conversion: > > L527: + nap = Max(0, (nextWakeup - now + 999) / > 1000); > .. > L545: +

Re: Suppressing useless wakeups in walreceiver

2022-10-26 Thread Bharath Rupireddy
On Sun, Oct 16, 2022 at 9:29 AM Nathan Bossart wrote: > > Here's a different take. Instead of creating structs and altering function > signatures, we can just make the wake-up times file-global, and we can skip > the changes related to reducing the number of calls to > GetCurrentTimestamp() for

Re: Suppressing useless wakeups in walreceiver

2022-10-17 Thread Kyotaro Horiguchi
Thanks for taking this up. At Sat, 15 Oct 2022 20:59:00 -0700, Nathan Bossart wrote in > On Thu, Oct 13, 2022 at 12:09:54PM -0700, Nathan Bossart wrote: > > On Thu, Oct 13, 2022 at 12:37:39PM +0200, Alvaro Herrera wrote: > >> The main reason is that it seems odd to have startpointTLI in the

Re: Suppressing useless wakeups in walreceiver

2022-10-15 Thread Nathan Bossart
On Thu, Oct 13, 2022 at 12:09:54PM -0700, Nathan Bossart wrote: > On Thu, Oct 13, 2022 at 12:37:39PM +0200, Alvaro Herrera wrote: >> The main reason is that it seems odd to have startpointTLI in the struct >> used in some places together with a file-global recvFileTLI which isn't. >> The way one

Re: Suppressing useless wakeups in walreceiver

2022-10-13 Thread Nathan Bossart
On Thu, Oct 13, 2022 at 12:37:39PM +0200, Alvaro Herrera wrote: > I think in 0001 we should put more stuff in the state struct -- > specifically these globals: > > static intrecvFile = -1; > static TimeLineID recvFileTLI = 0; > static XLogSegNo recvSegNo = 0; > > The main reason is that it

Re: Suppressing useless wakeups in walreceiver

2022-10-13 Thread Nathan Bossart
On Thu, Oct 13, 2022 at 03:35:14PM +0530, Bharath Rupireddy wrote: > I think the hot standby feedback message gets sent too frequently > without honouring the wal_receiver_status_interval because the 'now' > is actually not the current time with your patch but 'now + > XLogWalRcvSendReply()'s

Re: Suppressing useless wakeups in walreceiver

2022-10-13 Thread Alvaro Herrera
I think in 0001 we should put more stuff in the state struct -- specifically these globals: static int recvFile = -1; static TimeLineID recvFileTLI = 0; static XLogSegNo recvSegNo = 0; The main reason is that it seems odd to have startpointTLI in the struct used in some places together with

Re: Suppressing useless wakeups in walreceiver

2022-10-13 Thread Bharath Rupireddy
On Tue, Oct 11, 2022 at 11:22 PM Nathan Bossart wrote: > > On Tue, Oct 11, 2022 at 09:34:25AM +0530, Bharath Rupireddy wrote: > > now = t1; > > XLogWalRcvSendReply() /* say it ran for a minute or so for whatever reasons > > */ > > XLogWalRcvSendHSFeedback() /* with patch walrecevier sends hot

Re: Suppressing useless wakeups in walreceiver

2022-10-11 Thread Nathan Bossart
On Tue, Oct 11, 2022 at 09:34:25AM +0530, Bharath Rupireddy wrote: > now = t1; > XLogWalRcvSendReply() /* say it ran for a minute or so for whatever reasons */ > XLogWalRcvSendHSFeedback() /* with patch walrecevier sends hot standby > feedback more often without properly honouring >

Re: Suppressing useless wakeups in walreceiver

2022-10-10 Thread Bharath Rupireddy
On Tue, Oct 11, 2022 at 8:20 AM Nathan Bossart wrote: > > > AFICS, the aim of the patch isn't optimizing around > > GetCurrentTimestamp() calls and the patch does seem to change quite a > > bit of them which may cause a change of the behaviour. I think that > > the GetCurrentTimestamp()

Re: Suppressing useless wakeups in walreceiver

2022-10-10 Thread Nathan Bossart
On Tue, Oct 11, 2022 at 07:01:26AM +0530, Bharath Rupireddy wrote: > On Mon, Oct 10, 2022 at 11:21 PM Nathan Bossart >> Outside of the code snippet you pointed out, >> I think WalReceiverMain() has a similar problem. That being said, I'm >> generally skeptical that this sort of thing is

Re: Suppressing useless wakeups in walreceiver

2022-10-10 Thread Bharath Rupireddy
On Mon, Oct 10, 2022 at 11:21 PM Nathan Bossart wrote: > Thanks for the updated patches. > > 2. With the below change, the time walreceiver spends in > > XLogWalRcvSendReply() is also included for XLogWalRcvSendHSFeedback > > right? I think it's a problem given that XLogWalRcvSendReply() can >

Re: Suppressing useless wakeups in walreceiver

2022-10-10 Thread Nathan Bossart
On Mon, Oct 10, 2022 at 11:10:14AM -0700, Nathan Bossart wrote: > On Mon, Oct 10, 2022 at 10:51:14AM -0700, Nathan Bossart wrote: >>> +/* Find the soonest wakeup time, to limit our nap. */ >>> +nextWakeup = INT64_MAX; >>> +for (int i = 0; i <

Re: Suppressing useless wakeups in walreceiver

2022-10-10 Thread Nathan Bossart
On Mon, Oct 10, 2022 at 10:51:14AM -0700, Nathan Bossart wrote: >> +/* Find the soonest wakeup time, to limit our nap. */ >> +nextWakeup = INT64_MAX; >> +for (int i = 0; i < NUM_WALRCV_WAKEUPS; ++i) >> +nextWakeup =

Re: Suppressing useless wakeups in walreceiver

2022-10-10 Thread Nathan Bossart
On Mon, Oct 10, 2022 at 03:22:15PM +0530, Bharath Rupireddy wrote: > Some comments on v4-0002: Thanks for taking a look. > 1. You might want to use PG_INT64_MAX instead of INT64_MAX for portability? Yes, I used PG_INT64_MAX in v5. > 2. With the below change, the time walreceiver spends in >

Re: Suppressing useless wakeups in walreceiver

2022-10-10 Thread Bharath Rupireddy
On Wed, Oct 5, 2022 at 11:38 PM Nathan Bossart wrote: > > I moved as much as I could to 0001 in v4. Some comments on v4-0002: 1. You might want to use PG_INT64_MAX instead of INT64_MAX for portability? 2. With the below change, the time walreceiver spends in XLogWalRcvSendReply() is also

Re: Suppressing useless wakeups in walreceiver

2022-10-05 Thread Nathan Bossart
Thanks for taking a look. On Wed, Oct 05, 2022 at 09:57:00AM +0200, Alvaro Herrera wrote: > On 2022-Oct-04, Nathan Bossart wrote: >> * The creation of the struct for non-shared WAL receiver state is moved to >> a prerequisite 0001 patch. This should help ease review of 0002 a bit. > > I think

Re: Suppressing useless wakeups in walreceiver

2022-10-05 Thread Alvaro Herrera
On 2022-Oct-04, Nathan Bossart wrote: > Here is an updated patch set with the following changes: > > * The creation of the struct for non-shared WAL receiver state is moved to > a prerequisite 0001 patch. This should help ease review of 0002 a bit. I think that would be even better if you

Re: Suppressing useless wakeups in walreceiver

2022-10-04 Thread Nathan Bossart
Here is an updated patch set with the following changes: * The creation of the struct for non-shared WAL receiver state is moved to a prerequisite 0001 patch. This should help ease review of 0002 a bit. * I updated the nap time calculation to round up to the next millisecond, as discussed

Re: Suppressing useless wakeups in walreceiver

2022-09-30 Thread Nathan Bossart
On Fri, Sep 30, 2022 at 05:04:43PM +1300, Thomas Munro wrote: > Please go ahead! One thing I had in my notes to check with this patch > is whether there might be a bug due to computing all times in > microseconds, but sleeping for a number of milliseconds without > rounding up (what I mean is

Re: Suppressing useless wakeups in walreceiver

2022-09-29 Thread Thomas Munro
On Fri, Sep 30, 2022 at 4:51 PM Nathan Bossart wrote: > Thomas, are you planning to continue with this patch? If not, I don't mind > picking it up. Hi Nathan, Please go ahead! One thing I had in my notes to check with this patch is whether there might be a bug due to computing all times in

Re: Suppressing useless wakeups in walreceiver

2022-09-29 Thread Nathan Bossart
I was surprised to see that this patch was never committed! On Mon, Jan 31, 2022 at 04:28:11PM +0900, Kyotaro Horiguchi wrote: > +static void > +WalRcvComputeNextWakeup(WalRcvInfo *state, > + WalRcvWakeupReason reason, > +

Re: Suppressing useless wakeups in walreceiver

2022-01-30 Thread Kyotaro Horiguchi
At Fri, 28 Jan 2022 22:41:32 +1300, Thomas Munro wrote in > On Fri, Jan 28, 2022 at 8:26 PM Kyotaro Horiguchi > wrote: > > At Thu, 27 Jan 2022 23:50:04 +1300, Thomas Munro > > wrote in > The reason why I put the timeout computation into a function is > because there are about 3 places you

Re: Suppressing useless wakeups in walreceiver

2022-01-28 Thread Thomas Munro
On Fri, Jan 28, 2022 at 8:26 PM Kyotaro Horiguchi wrote: > At Thu, 27 Jan 2022 23:50:04 +1300, Thomas Munro > wrote in > > While working on WaitEventSet-ifying various codepaths, I found it > > strange that walreceiver wakes up 10 times per second while idle. > > Here's a draft patch to compute

Re: Suppressing useless wakeups in walreceiver

2022-01-27 Thread Kyotaro Horiguchi
Hello. At Thu, 27 Jan 2022 23:50:04 +1300, Thomas Munro wrote in > While working on WaitEventSet-ifying various codepaths, I found it > strange that walreceiver wakes up 10 times per second while idle. > Here's a draft patch to compute the correct sleep time. Agree to the objective. However

Suppressing useless wakeups in walreceiver

2022-01-27 Thread Thomas Munro
Hi, While working on WaitEventSet-ifying various codepaths, I found it strange that walreceiver wakes up 10 times per second while idle. Here's a draft patch to compute the correct sleep time. From 553d2dae8f8e7bb46ac73ca739aac03862f473cc Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Thu, 27