Re: [HACKERS] Allow interrupts on waiting standby

[Michael Paquier]

On Fri, Mar 31, 2017 at 4:46 PM, Tsunakawa, Takayuki
 wrote:
> Thank you, but did you remove WL_LATCH_SET from WaitLatch() intentionally?  I 
> understood you added it for startup process to respond quickly to events 
> other than the postmaster death.  Why don't we restore WL_LATCH_SET?  I won't 
> object to not adding the flag if there's a reason.

It doesn't matter. MyLatch is never set in the context of the startup
process. The other code paths of the startup process using WaitLatch()
may be awaken by the latch set by the WAL receiver, but that does not
apply here.

> I'll mark this as ready for committer when I see WL_LATCH_SET added (optional)

Objection on this point.

> and you have reported that you did the following test cases:
> * Startup process vanishes immediately after postmaster dies, while it is 
> waiting for a recovery conflict to be resolved.
> * Startup process vanishes immediately after "pg_ctl stop -m fast", while it 
> is waiting for a recovery conflict to be resolved.
> * Startup process resumes WAL application when max_standby_{archive | 
> streaming}_delay is changed from the default -1 to a short period, e.g. 10s, 
> and "pg_ctl reload" is performed, while it is waiting for a recovery conflict 
> to be resolved.

Fine for me, I have checked those multiple scenarios and the startup
process is more responsive. I have emulated the conflict with a
transaction doing repeatable read on the standby while the master was
deleting and vacuuming a table.

But, actually, after looking again at this patch with fresher eyes, I
am being a bit doubtful that this is really correct... Calling
HandleStartupProcInterrupts() is actually dangerous I think, because
it would reload the GUC context while replaying a record. But I think
that it is cleaner to reload the context after being done with a
record.

It seems to me that the correct way to do things would be to switch
all the conflict code paths to use a latch instead of pg_usleep, by
either use a dedicated latch like the recovery wakeup one, or the
MyLatch of the startup process. Then, when a signal arrives in, we
simply set up the conflict latch which wakes up what is waiting for
activity. The latch needs to be reset because calling WaitLatch().
Leaving immediately on WL_POSTMASTER_DEATH looks fine though as far as
I have tested.

As time is growing short, I am marking this patch as returned with
feedback. Thanks for the input, Tsunakawa-san!
-- 
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] Allow interrupts on waiting standby

[Tsunakawa, Takayuki]

From: Michael Paquier [mailto:michael.paqu...@gmail.com]
> Oops, sorry for that, I quite mess up with this patch. The WaitLatch() call
> should still have WL_POSTMASTER_DEATH so as it can leave earlier, but yes
> I agree with your analysis that HandleStartupProcInterrupts() as this is
> part of the redo work.

Thank you, but did you remove WL_LATCH_SET from WaitLatch() intentionally?  I 
understood you added it for startup process to respond quickly to events other 
than the postmaster death.  Why don't we restore WL_LATCH_SET?  I won't object 
to not adding the flag if there's a reason.

I'll mark this as ready for committer when I see WL_LATCH_SET added (optional) 
and you have reported that you did the following test cases:

* Startup process vanishes immediately after postmaster dies, while it is 
waiting for a recovery conflict to be resolved.

* Startup process vanishes immediately after "pg_ctl stop -m fast", while it is 
waiting for a recovery conflict to be resolved.

* Startup process resumes WAL application when max_standby_{archive | 
streaming}_delay is changed from the default -1 to a short period, e.g. 10s, 
and "pg_ctl reload" is performed, while it is waiting for a recovery conflict 
to be resolved.


> > Did Simon's committed patch solve the problem as expected?
> 
> Does not seem so, I'll let Simon comment on this matter...

Agreed.  I guess his patch for earlier releases should work if 
CHECK_FOR_INTERRUPTS() is replaced with HandleStartupProcInterrupts().

Regards
Takayuki Tsunakawa


-- 
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] Allow interrupts on waiting standby

[Michael Paquier]

On Thu, Mar 30, 2017 at 4:02 PM, Tsunakawa, Takayuki
 wrote:
> I think you can call HandleStartupProcInterrupts() here, instead of checking 
> postmaster death.

Oops, sorry for that, I quite mess up with this patch. The WaitLatch()
call should still have WL_POSTMASTER_DEATH so as it can leave earlier,
but yes I agree with your analysis that HandleStartupProcInterrupts()
as this is part of the redo work.

> Did Simon's committed patch solve the problem as expected?

Does not seem so, I'll let Simon comment on this matter...
-- 
Michael


standby-delay-latch-v6.patch
Description: Binary data

-- 
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] Allow interrupts on waiting standby

[Tsunakawa, Takayuki]

Hi Michael, Simon,

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> > Oh, I see.  But how does the startup process respond quickly?  It seems
> that you need to call HandleStartupProcInterrupts() instead of
> CHECK_FOR_INTERRUPTS().  But I'm not sure whether
> HandleStartupProcInterrupts() can be called here.
> 
> Bah. Of course you are right. We don't care about SetLatch() here as signals
> are processed with a different code path than normal backends.

So, I understood you agreed that CHECK_FOR_INTERRUPTS() here does nothing.  But 
your patch still calls it:

+   /* An interrupt may have occurred while waiting */
+   CHECK_FOR_INTERRUPTS();

I got confused because the problem is not defined in this thread.  What problem 
does this patch address?  These ones?

* The startup process terminates as soon as postmaster dies.
* pg_stat_activity does not show the wait event of startup process waiting for 
a recovery conflict resolution.


My guess about why you decided to not call HandleStartupProcInterrupts() here 
is:

* Respond to postmaster death here.
* No need to reload config file here because there's no parameter to affect 
this conflict wait.  But max_standby_{archive | streaming}_delay seems to 
affect the wait period.
* No need to handle SIGTERM and exit here, because the startup process doesn't 
wait for a conflict resolution here when he can terminate.

I think you can call HandleStartupProcInterrupts() here, instead of checking 
postmaster death.  Did you perform tests?

Did Simon's committed patch solve the problem as expected?

Regards
Takayuki Tsunakawa


-- 
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] Allow interrupts on waiting standby

[Michael Paquier]

On Thu, Mar 30, 2017 at 1:52 PM, Tsunakawa, Takayuki
 wrote:
> From: pgsql-hackers-ow...@postgresql.org
>> > (2) standby.c
>> > Do we need to specify WL_LATCH_SET?  Who can set the latch?  Do the
>> backends who ended the conflict set the latch?
>>
>> This makes the process able to react on SIGHUP. That's useful for
>> responsiveness.
>
> Oh, I see.  But how does the startup process respond quickly?  It seems that 
> you need to call HandleStartupProcInterrupts() instead of 
> CHECK_FOR_INTERRUPTS().  But I'm not sure whether 
> HandleStartupProcInterrupts() can be called here.

Bah. Of course you are right. We don't care about SetLatch() here as
signals are processed with a different code path than normal backends.

>> > (3) standby.c
>> > +   if (rc & WL_LATCH_SET)
>> > +   ResetLatch(MyLatch);
>> > +
>> > +   /* emergency bailout if postmaster has died */
>> > +   if (rc & WL_POSTMASTER_DEATH)
>> > +   proc_exit(1);
>> >
>> > I thought the child processes have to terminate as soon as postmaster
>> vanishes.  So, it would be better for the order of the two if statements
>> above to be reversed.  proc_exit() could be exit(), as some children do
>> in postmaster/*.c.
>>
>> OK, reversed this order.
>
> I think exit() instead of proc_exit() better represents what the code wants 
> to do -- terminate the process ASAP without cleaning up.  Many other 
> background children do so.

Hm... OK.
-- 
Michael


standby-delay-latch-v5.patch
Description: Binary data

-- 
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] Allow interrupts on waiting standby

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> > By the way, doesn't this wait event belong to IPC wait event type, because
> the process is waiting for other conflicting processes to terminate the
> conflict conditions?  Did you choose Timeout type because
> max_standby_{archive | streaming}_delay relates to this wait?  I'm not
> confident which is appropriate, but I'm afraid users can associate this
> wait with a timeout.
> 
> Actually I think that this event belongs to the timeout category, because
> we wait until the timeout has finished, the latch being here to be sure
> that the process is more responsive in case of a postmaster death.

OK.  I confirmed the doc is fixed.

> > (2) standby.c
> > Do we need to specify WL_LATCH_SET?  Who can set the latch?  Do the
> backends who ended the conflict set the latch?
> 
> This makes the process able to react on SIGHUP. That's useful for
> responsiveness.

Oh, I see.  But how does the startup process respond quickly?  It seems that 
you need to call HandleStartupProcInterrupts() instead of 
CHECK_FOR_INTERRUPTS().  But I'm not sure whether HandleStartupProcInterrupts() 
can be called here.



> > (3) standby.c
> > +   if (rc & WL_LATCH_SET)
> > +   ResetLatch(MyLatch);
> > +
> > +   /* emergency bailout if postmaster has died */
> > +   if (rc & WL_POSTMASTER_DEATH)
> > +   proc_exit(1);
> >
> > I thought the child processes have to terminate as soon as postmaster
> vanishes.  So, it would be better for the order of the two if statements
> above to be reversed.  proc_exit() could be exit(), as some children do
> in postmaster/*.c.
> 
> OK, reversed this order.

I think exit() instead of proc_exit() better represents what the code wants to 
do -- terminate the process ASAP without cleaning up.  Many other background 
children do so.


Regards
Takayuki Tsunakawa


-- 
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] Allow interrupts on waiting standby

[Michael Paquier]

On Wed, Mar 29, 2017 at 5:04 PM, Tsunakawa, Takayuki
 wrote:
> From: pgsql-hackers-ow...@postgresql.org
>> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
>> What do you think about the updated version attached?
> I reviewed this patch.  Here are some comments and questions:

Thanks!

> (1) monitoring.sgml
> The new row needs to be placed in the "Timeout" group.  The current patch 
> puts the row at the end of IO group.  Please insert the new row according to 
> the alphabetical order.
>
> In addition, "morerows" attribute of the following line appears to be 
> increased.
>
>  Timeout
>
> By the way, doesn't this wait event belong to IPC wait event type, because 
> the process is waiting for other conflicting processes to terminate the 
> conflict conditions?  Did you choose Timeout type because 
> max_standby_{archive | streaming}_delay relates to this wait?  I'm not 
> confident which is appropriate, but I'm afraid users can associate this wait 
> with a timeout.

Actually I think that this event belongs to the timeout category,
because we wait until the timeout has finished, the latch being here
to be sure that the process is more responsive in case of a postmaster
death.

> (2) standby.c
> Do we need to specify WL_LATCH_SET?  Who can set the latch?  Do the backends 
> who ended the conflict set the latch?

This makes the process able to react on SIGHUP. That's useful for
responsiveness.

> (3) standby.c
> +   if (rc & WL_LATCH_SET)
> +   ResetLatch(MyLatch);
> +
> +   /* emergency bailout if postmaster has died */
> +   if (rc & WL_POSTMASTER_DEATH)
> +   proc_exit(1);
>
> I thought the child processes have to terminate as soon as postmaster 
> vanishes.  So, it would be better for the order of the two if statements 
> above to be reversed.  proc_exit() could be exit(), as some children do in 
> postmaster/*.c.

OK, reversed this order.

Attached is v4.
-- 
Michael


standby-delay-latch-v4.patch
Description: Binary data

-- 
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] Allow interrupts on waiting standby

[Tsunakawa, Takayuki]

 (4) standby.c
> The latch is not reset when the wait timed out.  The next WaitLatch() would
> return immediately.

Sorry, let me withdraw this.  This is my misunderstanding.

OTOH, when is the latch reset before the wait?  Is there an assumption that 
MyLatch has been in reset state since it was initialized?
Is it safe to use MyLatch here, not the special latch like something used in 
xlog.c?


Regards
Takayuki Tsunakawa


-- 
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] Allow interrupts on waiting standby

[Tsunakawa, Takayuki]

Hi, Michael,


From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> What do you think about the updated version attached?

I reviewed this patch.  Here are some comments and questions:


(1) monitoring.sgml
The new row needs to be placed in the "Timeout" group.  The current patch puts 
the row at the end of IO group.  Please insert the new row according to the 
alphabetical order.

In addition, "morerows" attribute of the following line appears to be increased.

 Timeout

By the way, doesn't this wait event belong to IPC wait event type, because the 
process is waiting for other conflicting processes to terminate the conflict 
conditions?  Did you choose Timeout type because max_standby_{archive | 
streaming}_delay relates to this wait?  I'm not confident which is appropriate, 
but I'm afraid users can associate this wait with a timeout.


(2) standby.c
Do we need to specify WL_LATCH_SET?  Who can set the latch?  Do the backends 
who ended the conflict set the latch?


(3) standby.c
+   if (rc & WL_LATCH_SET)
+   ResetLatch(MyLatch);
+
+   /* emergency bailout if postmaster has died */
+   if (rc & WL_POSTMASTER_DEATH)
+   proc_exit(1);

I thought the child processes have to terminate as soon as postmaster vanishes. 
 So, it would be better for the order of the two if statements above to be 
reversed.  proc_exit() could be exit(), as some children do in postmaster/*.c.



(4) standby.c
The latch is not reset when the wait timed out.  The next WaitLatch() would 
return immediately.


Regards
Takayuki Tsunakawa



-- 
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] Allow interrupts on waiting standby

[Michael Paquier]

On Sun, Mar 19, 2017 at 5:14 AM, Tom Lane  wrote:
> Couple of thoughts on this patch ---

Thanks!

> 1. Shouldn't WaitExceedsMaxStandbyDelay's CHECK_FOR_INTERRUPTS be moved to
> after the WaitLatch call?  Not much point in being woken immediately by
> an interrupt if you're not going to respond.
>
> 2. Is it OK to ResetLatch here?  If the only possible latch event in this
> process is interrupt requests, then I think WaitLatch, then ResetLatch,
> then CHECK_FOR_INTERRUPTS is OK; but otherwise it seems like you risk
> discarding events that need to be serviced later.

Right, I have switched to WaitLatch(), ResetLatch() and then
CHECK_FOR_INTERRUPTS().

> 3. In the same vein, if we're going to check WL_POSTMASTER_DEATH, should
> there be a test for that and immediate exit(1) here?

OK, if the postmaster has died, there is not much recovery conflict
needed anyway.

> 4. I'd be inclined to increase the sleep interval only if we did time out,
> not if we were awakened by some other event.

OK, that makes sense.

> 5. The comment about maximum sleep length needs some work.  At first
> glance you might think that without the motivation of preventing long
> uninterruptible sleeps, we might as well allow the sleep length to grow
> well past 1s.  I think that'd be bad, because we want to wake up
> reasonably soon after the xact(s) we're waiting for commit.  But neither
> the original text nor the proposed replacement mention this.

OK, I did some work on this comment.

What do you think about the updated version attached?
-- 
Michael


standby-delay-latch-v3.patch
Description: Binary data

-- 
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] Allow interrupts on waiting standby

[Tom Lane]

Michael Paquier  writes:
> Both things are fixed in the new version attached. I have added as
> well this patch to the next commit fest:
> https://commitfest.postgresql.org/13/977/

Couple of thoughts on this patch ---

1. Shouldn't WaitExceedsMaxStandbyDelay's CHECK_FOR_INTERRUPTS be moved to
after the WaitLatch call?  Not much point in being woken immediately by
an interrupt if you're not going to respond.

2. Is it OK to ResetLatch here?  If the only possible latch event in this
process is interrupt requests, then I think WaitLatch, then ResetLatch,
then CHECK_FOR_INTERRUPTS is OK; but otherwise it seems like you risk
discarding events that need to be serviced later.

3. In the same vein, if we're going to check WL_POSTMASTER_DEATH, should
there be a test for that and immediate exit(1) here?

4. I'd be inclined to increase the sleep interval only if we did time out,
not if we were awakened by some other event.

5. The comment about maximum sleep length needs some work.  At first
glance you might think that without the motivation of preventing long
uninterruptible sleeps, we might as well allow the sleep length to grow
well past 1s.  I think that'd be bad, because we want to wake up
reasonably soon after the xact(s) we're waiting for commit.  But neither
the original text nor the proposed replacement mention this.

regards, tom lane


-- 
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] Allow interrupts on waiting standby

[Michael Paquier]

On Wed, Mar 8, 2017 at 5:45 AM, Peter Eisentraut
 wrote:
> On 1/30/17 20:34, Michael Paquier wrote:
>> Both things are fixed in the new version attached. I have added as
>> well this patch to the next commit fest:
>> https://commitfest.postgresql.org/13/977/
>
> I'm not clear now what this patch is intending to accomplish, seeing
> that the original issue has already been fixed.

This makes ResolveRecoveryConflictWithVirtualXIDs() more responsive if
the process is signaled, as the wait in pg_usleep can go up to 1s if
things remain stuck for a long time.
-- 
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] Allow interrupts on waiting standby

[Peter Eisentraut]

On 1/30/17 20:34, Michael Paquier wrote:
> On Fri, Jan 27, 2017 at 10:17 PM, Michael Paquier
>  wrote:
>> Two things I forgot in this patch:
>> - documentation for the new wait event
>> - the string for the wait event or this would show up as "???" in
>> pg_stat_activity.
>> There are no default clauses in the pgstat_get_wait_* routines so my
>> compiler is actually complaining...
> 
> Both things are fixed in the new version attached. I have added as
> well this patch to the next commit fest:
> https://commitfest.postgresql.org/13/977/

I'm not clear now what this patch is intending to accomplish, seeing
that the original issue has already been fixed.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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] Allow interrupts on waiting standby

[Michael Paquier]

On Fri, Jan 27, 2017 at 10:17 PM, Michael Paquier
 wrote:
> Two things I forgot in this patch:
> - documentation for the new wait event
> - the string for the wait event or this would show up as "???" in
> pg_stat_activity.
> There are no default clauses in the pgstat_get_wait_* routines so my
> compiler is actually complaining...

Both things are fixed in the new version attached. I have added as
well this patch to the next commit fest:
https://commitfest.postgresql.org/13/977/
-- 
Michael


standby-delay-latch-v2.patch
Description: Binary data

-- 
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] Allow interrupts on waiting standby

[Michael Paquier]

On Sat, Jan 28, 2017 at 2:18 AM, Robert Haas  wrote:
> On Fri, Jan 27, 2017 at 8:17 AM, Michael Paquier
>  wrote:
>> There are no default clauses in the pgstat_get_wait_* routines so my
>> compiler is actually complaining...
>
> That's exactly WHY there are no default clauses there.   :-)

And that's exactly why I missed them! ;p
-- 
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] Allow interrupts on waiting standby

[Robert Haas]

On Fri, Jan 27, 2017 at 8:17 AM, Michael Paquier
 wrote:
> There are no default clauses in the pgstat_get_wait_* routines so my
> compiler is actually complaining...

That's exactly WHY there are no default clauses there.   :-)

-- 
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] Allow interrupts on waiting standby

[Michael Paquier]

On Fri, Jan 27, 2017 at 10:35 AM, Michael Paquier
 wrote:
> On Fri, Jan 27, 2017 at 4:36 AM, Simon Riggs  wrote:
>> On 26 January 2017 at 19:20, Andres Freund  wrote:
>>> On 2017-01-26 12:24:44 -0500, Robert Haas wrote:
 On Thu, Jan 26, 2017 at 7:18 AM, Simon Riggs  wrote:
 > Currently a waiting standby doesn't allow interrupts.
 >
 > Patch implements that.
 >
 > Barring objection, patching today with backpatches.

 "today" is a little quick, but the patch looks fine.  I doubt anyone's
 going to screech too loud about adding a CHECK_FOR_INTERRUPTS() call.
>>>
>>> I don't quite get asking for agreement, and then not waiting as
>>> suggested.  I'm personally fine with going with a CHECK_FOR_INTERRUPTS
>>> for now, but I think it'd better to replace it with a latch.
>>
>> I have waited, so not sure what you mean. Tomorrow is too late.
>
> This gives really little time for any feedback :(
>
>> Replacing with a latch wouldn't be backpatchable, IMHO.
>> I've no problem if you want to work on a deeper fix for future versions.
>
> A deeper fix for HEAD proves to not be that complicated. Please see
> the attached. The other two calls of pg_usleep() in standby.c are
> waiting with 5ms and 10ms, it is not worth switching them to a latch.

Two things I forgot in this patch:
- documentation for the new wait event
- the string for the wait event or this would show up as "???" in
pg_stat_activity.
There are no default clauses in the pgstat_get_wait_* routines so my
compiler is actually complaining...
-- 
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] Allow interrupts on waiting standby

[Michael Paquier]

On Fri, Jan 27, 2017 at 9:23 PM, Simon Riggs  wrote:
> On 27 January 2017 at 01:35, Michael Paquier  
> wrote:
>> On Fri, Jan 27, 2017 at 4:36 AM, Simon Riggs  wrote:
>>> On 26 January 2017 at 19:20, Andres Freund  wrote:
>> A deeper fix for HEAD proves to not be that complicated. Please see
>> the attached. The other two calls of pg_usleep() in standby.c are
>> waiting with 5ms and 10ms, it is not worth switching them to a latch.
>
> So you think 2 calls to pg_usleep() can stay; my opinion is 3 can stay.

This patch replaces one call of pg_usleep() where the wait can go up
to 1s, this is largely noticeable by the user. The two others sleep
for a maximum of 10ms. Even if a latch is used the code path is going
to exit immediately anyway. That's why you added a call to
CHECK_FOR_INTERRUPTS only here, no?

> I'm not clear why this particular call is worthy, while dozens of
> calls in other modules remain unchanged. This seems like a code issue
> rather than anything to do with Hot Standby in particular, so it
> should be another thread.

Some should switch to Latch.

> Doesn't seem important compared to other
> things for this release I should work on.

That's your call.

> Please add to the next CF so it gets proper review.

Sure.
-- 
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] Allow interrupts on waiting standby

[Simon Riggs]

On 27 January 2017 at 01:35, Michael Paquier  wrote:
> On Fri, Jan 27, 2017 at 4:36 AM, Simon Riggs  wrote:
>> On 26 January 2017 at 19:20, Andres Freund  wrote:

>>> I'm personally fine with going with a CHECK_FOR_INTERRUPTS
>>> for now, but I think it'd better to replace it with a latch.

>> Replacing with a latch wouldn't be backpatchable, IMHO.
>> I've no problem if you want to work on a deeper fix for future versions.
>
> A deeper fix for HEAD proves to not be that complicated. Please see
> the attached. The other two calls of pg_usleep() in standby.c are
> waiting with 5ms and 10ms, it is not worth switching them to a latch.

So you think 2 calls to pg_usleep() can stay; my opinion is 3 can stay.

I'm not clear why this particular call is worthy, while dozens of
calls in other modules remain unchanged. This seems like a code issue
rather than anything to do with Hot Standby in particular, so it
should be another thread. Doesn't seem important compared to other
things for this release I should work on.

Please add to the next CF so it gets proper review.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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] Allow interrupts on waiting standby

[Simon Riggs]

On 26 January 2017 at 20:36, Alvaro Herrera  wrote:
> Simon Riggs wrote:
>> On 26 January 2017 at 19:20, Andres Freund  wrote:
>
>> > I'm personally fine with going with a CHECK_FOR_INTERRUPTS
>> > for now, but I think it'd better to replace it with a latch.
>>
>> I have waited, so not sure what you mean. Tomorrow is too late.
>>
>> Replacing with a latch wouldn't be backpatchable, IMHO.
>
> Hmm, you pushed this to the master branch as commit e8ee3d6b859a, but I
> see no backpatch.

There appeared to be a complaint at my speed, so I slowed down.

On reading that there are no objections to this being backpatched, I
will do that now.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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] Allow interrupts on waiting standby

[Michael Paquier]

On Fri, Jan 27, 2017 at 4:36 AM, Simon Riggs  wrote:
> On 26 January 2017 at 19:20, Andres Freund  wrote:
>> On 2017-01-26 12:24:44 -0500, Robert Haas wrote:
>>> On Thu, Jan 26, 2017 at 7:18 AM, Simon Riggs  wrote:
>>> > Currently a waiting standby doesn't allow interrupts.
>>> >
>>> > Patch implements that.
>>> >
>>> > Barring objection, patching today with backpatches.
>>>
>>> "today" is a little quick, but the patch looks fine.  I doubt anyone's
>>> going to screech too loud about adding a CHECK_FOR_INTERRUPTS() call.
>>
>> I don't quite get asking for agreement, and then not waiting as
>> suggested.  I'm personally fine with going with a CHECK_FOR_INTERRUPTS
>> for now, but I think it'd better to replace it with a latch.
>
> I have waited, so not sure what you mean. Tomorrow is too late.

This gives really little time for any feedback :(

> Replacing with a latch wouldn't be backpatchable, IMHO.
> I've no problem if you want to work on a deeper fix for future versions.

A deeper fix for HEAD proves to not be that complicated. Please see
the attached. The other two calls of pg_usleep() in standby.c are
waiting with 5ms and 10ms, it is not worth switching them to a latch.
-- 
Michael


standby-delay-latch.patch
Description: Binary data

-- 
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] Allow interrupts on waiting standby

[Alvaro Herrera]

Simon Riggs wrote:
> On 26 January 2017 at 19:20, Andres Freund  wrote:

> > I'm personally fine with going with a CHECK_FOR_INTERRUPTS
> > for now, but I think it'd better to replace it with a latch.
> 
> I have waited, so not sure what you mean. Tomorrow is too late.
> 
> Replacing with a latch wouldn't be backpatchable, IMHO.

Hmm, you pushed this to the master branch as commit e8ee3d6b859a, but I
see no backpatch.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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] Allow interrupts on waiting standby

[Stephen Frost]

* Andres Freund (and...@anarazel.de) wrote:
> On 2017-01-26 19:36:11 +, Simon Riggs wrote:
> > Tomorrow is too late.
> 
> Huh? We're not wrapping today/tomorrow, are we?  If I missed something
> and we are, then sure, it makes sense to push ahead.

I haven't seen anyone suggest that we're changing from the regular
release cycle, which would mean that we wrap on Monday, Feb 6th, for
release on Feb 9th.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Allow interrupts on waiting standby

[Andres Freund]

On 2017-01-26 19:36:11 +, Simon Riggs wrote:
> On 26 January 2017 at 19:20, Andres Freund  wrote:
> > On 2017-01-26 12:24:44 -0500, Robert Haas wrote:
> >> On Thu, Jan 26, 2017 at 7:18 AM, Simon Riggs  wrote:
> >> > Currently a waiting standby doesn't allow interrupts.
> >> >
> >> > Patch implements that.
> >> >
> >> > Barring objection, patching today with backpatches.
> >>
> >> "today" is a little quick, but the patch looks fine.  I doubt anyone's
> >> going to screech too loud about adding a CHECK_FOR_INTERRUPTS() call.
> >
> > I don't quite get asking for agreement, and then not waiting as
> > suggested.  I'm personally fine with going with a CHECK_FOR_INTERRUPTS
> > for now, but I think it'd better to replace it with a latch.
> 
> I have waited, so not sure what you mean.

Well, Robert today said >> "today" is a little quick <<.


> Tomorrow is too late.

Huh? We're not wrapping today/tomorrow, are we?  If I missed something
and we are, then sure, it makes sense to push ahead.


> Replacing with a latch wouldn't be backpatchable, IMHO.

Hm, don't quite see why - isn't it just like three lines?


Andres


-- 
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] Allow interrupts on waiting standby

[Simon Riggs]

On 26 January 2017 at 19:20, Andres Freund  wrote:
> On 2017-01-26 12:24:44 -0500, Robert Haas wrote:
>> On Thu, Jan 26, 2017 at 7:18 AM, Simon Riggs  wrote:
>> > Currently a waiting standby doesn't allow interrupts.
>> >
>> > Patch implements that.
>> >
>> > Barring objection, patching today with backpatches.
>>
>> "today" is a little quick, but the patch looks fine.  I doubt anyone's
>> going to screech too loud about adding a CHECK_FOR_INTERRUPTS() call.
>
> I don't quite get asking for agreement, and then not waiting as
> suggested.  I'm personally fine with going with a CHECK_FOR_INTERRUPTS
> for now, but I think it'd better to replace it with a latch.

I have waited, so not sure what you mean. Tomorrow is too late.

Replacing with a latch wouldn't be backpatchable, IMHO.

I've no problem if you want to work on a deeper fix for future versions.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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] Allow interrupts on waiting standby

[Andres Freund]

On 2017-01-26 12:24:44 -0500, Robert Haas wrote:
> On Thu, Jan 26, 2017 at 7:18 AM, Simon Riggs  wrote:
> > Currently a waiting standby doesn't allow interrupts.
> >
> > Patch implements that.
> >
> > Barring objection, patching today with backpatches.
> 
> "today" is a little quick, but the patch looks fine.  I doubt anyone's
> going to screech too loud about adding a CHECK_FOR_INTERRUPTS() call.

I don't quite get asking for agreement, and then not waiting as
suggested.  I'm personally fine with going with a CHECK_FOR_INTERRUPTS
for now, but I think it'd better to replace it with a latch.

Andres


-- 
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] Allow interrupts on waiting standby

[Robert Haas]

On Thu, Jan 26, 2017 at 7:18 AM, Simon Riggs  wrote:
> Currently a waiting standby doesn't allow interrupts.
>
> Patch implements that.
>
> Barring objection, patching today with backpatches.

"today" is a little quick, but the patch looks fine.  I doubt anyone's
going to screech too loud about adding a CHECK_FOR_INTERRUPTS() call.

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

[HACKERS] Allow interrupts on waiting standby

[Simon Riggs]

Currently a waiting standby doesn't allow interrupts.

Patch implements that.

Barring objection, patching today with backpatches.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


interrupt_waiting_standby.v1.patch
Description: Binary data

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