Thread: Use WaitLatch for {pre, post}_auth_delay instead of pg_usleep
Hi,
As suggested in [1], starting a new thread for discussing $subject
separately. {pre, post}_auth_delay waiting logic currently uses
pg_usleep which can't detect postmaster death. So, there are chances
that some of the backends still stay in the system even when a
postmaster crashes (for whatever reasons it may be). Please have a
look at the attached patch that does $subject. I pulled out some of
the comments from the other thread related to the $subject, [2], [3],
[4], [5].
[1] - https://www.postgresql.org/message-id/YOv8Yxd5zrbr3k%2BH%40paquier.xyz
[2] - https://www.postgresql.org/message-id/162764.1624892517%40sss.pgh.pa.us
[3] - https://www.postgresql.org/message-id/20210705.145251.462698229911576780.horikyota.ntt%40gmail.com
[4] - https://www.postgresql.org/message-id/flat/20210705155553.GD20766%40tamriel.snowman.net
[5] - https://www.postgresql.org/message-id/YOOnlP4NtWVzfsyb%40paquier.xyz
Regards,
Bharath Rupireddy.
Attachment
On Mon, Jul 12, 2021 at 9:26 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> Hi,
>
> As suggested in [1], starting a new thread for discussing $subject
> separately. {pre, post}_auth_delay waiting logic currently uses
> pg_usleep which can't detect postmaster death. So, there are chances
> that some of the backends still stay in the system even when a
> postmaster crashes (for whatever reasons it may be). Please have a
> look at the attached patch that does $subject. I pulled out some of
> the comments from the other thread related to the $subject, [2], [3],
> [4], [5].
>
> [1] - https://www.postgresql.org/message-id/YOv8Yxd5zrbr3k%2BH%40paquier.xyz
> [2] - https://www.postgresql.org/message-id/162764.1624892517%40sss.pgh.pa.us
> [3] - https://www.postgresql.org/message-id/20210705.145251.462698229911576780.horikyota.ntt%40gmail.com
> [4] - https://www.postgresql.org/message-id/flat/20210705155553.GD20766%40tamriel.snowman.net
> [5] - https://www.postgresql.org/message-id/YOOnlP4NtWVzfsyb%40paquier.xyz
I added this to the commitfest - https://commitfest.postgresql.org/34/3255/
Regards,
Bharath Rupireddy.
On 7/12/21, 9:00 AM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:
> As suggested in [1], starting a new thread for discussing $subject
> separately. {pre, post}_auth_delay waiting logic currently uses
> pg_usleep which can't detect postmaster death. So, there are chances
> that some of the backends still stay in the system even when a
> postmaster crashes (for whatever reasons it may be). Please have a
> look at the attached patch that does $subject. I pulled out some of
> the comments from the other thread related to the $subject, [2], [3],
> [4], [5].
+ <row>
+ <entry><literal>PostAuthDelay</literal></entry>
+ <entry>Waiting on connection startup after authentication to allow attach
+ from a debugger.</entry>
+ </row>
+ <row>
+ <entry><literal>PreAuthDelay</literal></entry>
+ <entry>Waiting on connection startup before authentication to allow
+ attach from a debugger.</entry>
+ </row>
I would suggest changing "attach from a debugger" to "attaching with a
debugger."
if (PreAuthDelay > 0)
- pg_usleep(PreAuthDelay * 1000000L);
+ {
+ /*
+ * Do not use WL_LATCH_SET during backend initialization because the
+ * MyLatch may point to shared latch later.
+ */
+ (void) WaitLatch(MyLatch,
+ WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
+ PreAuthDelay * 1000L,
+ WAIT_EVENT_PRE_AUTH_DELAY);
+ }
IIUC you want to use the same set of flags as PostAuthDelay for
PreAuthDelay, but the stated reason in this comment for leaving out
WL_LATCH_SET suggests otherwise. It's not clear to me why the latch
possibly pointing to a shared latch in the future is an issue. Should
this instead say that we leave out WL_LATCH_SET for consistency with
PostAuthDelay?
Nathan
On Fri, Jul 23, 2021 at 4:40 AM Bossart, Nathan <bossartn@amazon.com> wrote:
> I would suggest changing "attach from a debugger" to "attaching with a
> debugger."
Thanks. IMO, the following looks better:
<entry>Waiting on connection startup before authentication to allow
attaching a debugger to the process.</entry>
<entry>Waiting on connection startup after authentication to allow
attaching a debugger to the process.</entry>
> IIUC you want to use the same set of flags as PostAuthDelay for
> PreAuthDelay, but the stated reason in this comment for leaving out
> WL_LATCH_SET suggests otherwise. It's not clear to me why the latch
> possibly pointing to a shared latch in the future is an issue. Should
> this instead say that we leave out WL_LATCH_SET for consistency with
> PostAuthDelay?
If WL_LATCH_SET is used for PostAuthDelay, the waiting doesn't happen
because the MyLatch which is a shared latch would be set by
SwitchToSharedLatch. More details at [1].
If WL_LATCH_SET is used for PreAuthDelay, actually there's no problem
because MyLatch is still not initialized properly in BackendInitialize
when waiting for PreAuthDelay, it still points to local latch, but
later gets pointed to shared latch and gets set SwitchToSharedLatch.
But relying on MyLatch there seems to me somewhat relying on an
uninitialized variable. More details at [1].
For PreAuthDelay, with the comment I wanted to say that the MyLatch is
not the correct one we would want to wait for. Since there is no
problem in using it there, I changed the comment to following:
/*
* Let's not use WL_LATCH_SET for PreAuthDelay to be consistent with
* PostAuthDelay.
*/
[1] - https://www.postgresql.org/message-id/flat/CALj2ACVF8AZi1bK8oH-Qoz3tYVpqFuzxcDRPdF-3p5BvF6GTxA%40mail.gmail.com
Regards,
Bharath Rupireddy.
Attachment
On 7/24/21, 9:16 AM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:
> On Fri, Jul 23, 2021 at 4:40 AM Bossart, Nathan <bossartn@amazon.com> wrote:
>> I would suggest changing "attach from a debugger" to "attaching with a
>> debugger."
>
> Thanks. IMO, the following looks better:
> <entry>Waiting on connection startup before authentication to allow
> attaching a debugger to the process.</entry>
> <entry>Waiting on connection startup after authentication to allow
> attaching a debugger to the process.</entry>
Your phrasing looks good to me.
>> IIUC you want to use the same set of flags as PostAuthDelay for
>> PreAuthDelay, but the stated reason in this comment for leaving out
>> WL_LATCH_SET suggests otherwise. It's not clear to me why the latch
>> possibly pointing to a shared latch in the future is an issue. Should
>> this instead say that we leave out WL_LATCH_SET for consistency with
>> PostAuthDelay?
>
> If WL_LATCH_SET is used for PostAuthDelay, the waiting doesn't happen
> because the MyLatch which is a shared latch would be set by
> SwitchToSharedLatch. More details at [1].
> If WL_LATCH_SET is used for PreAuthDelay, actually there's no problem
> because MyLatch is still not initialized properly in BackendInitialize
> when waiting for PreAuthDelay, it still points to local latch, but
> later gets pointed to shared latch and gets set SwitchToSharedLatch.
> But relying on MyLatch there seems to me somewhat relying on an
> uninitialized variable. More details at [1].
>
> For PreAuthDelay, with the comment I wanted to say that the MyLatch is
> not the correct one we would want to wait for. Since there is no
> problem in using it there, I changed the comment to following:
> /*
> * Let's not use WL_LATCH_SET for PreAuthDelay to be consistent with
> * PostAuthDelay.
> */
How about we elaborate a bit?
WL_LATCH_SET is not used for consistency with PostAuthDelay.
MyLatch isn't fully initialized for the backend at this point,
anyway.
+ /*
+ * PostAuthDelay will not get applied, if WL_LATCH_SET is used. This
+ * is because the latch could have been set initially.
+ */
I would suggest the following:
If WL_LATCH_SET is used, PostAuthDelay may not be applied,
since the latch might already be set.
Otherwise, this patch looks good and could probably be marked ready-
for-committer.
Nathan
On Mon, Jul 26, 2021 at 11:03 PM Bossart, Nathan <bossartn@amazon.com> wrote: > > For PreAuthDelay, with the comment I wanted to say that the MyLatch is > > not the correct one we would want to wait for. Since there is no > > problem in using it there, I changed the comment to following: > > /* > > * Let's not use WL_LATCH_SET for PreAuthDelay to be consistent with > > * PostAuthDelay. > > */ > > How about we elaborate a bit? > > WL_LATCH_SET is not used for consistency with PostAuthDelay. > MyLatch isn't fully initialized for the backend at this point, > anyway. +1. > + /* > + * PostAuthDelay will not get applied, if WL_LATCH_SET is used. This > + * is because the latch could have been set initially. > + */ > > I would suggest the following: > > If WL_LATCH_SET is used, PostAuthDelay may not be applied, > since the latch might already be set. +1. > Otherwise, this patch looks good and could probably be marked ready- > for-committer. PSA v3 patch. Regards, Bharath Rupireddy.
Attachment
On 7/26/21, 10:34 PM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote: > PSA v3 patch. LGTM. The pre/post_auth_delay parameters seem to work as intended, and they are responsive to postmaster crashes. I didn't find any examples of calling WaitLatch() without WL_LATCH_SET, but the function appears to have support for that. (In fact, it just sets the latch variable to NULL in that case, so perhaps we should replace MyLatch with NULL in the patch.) I do see that WaitLatchOrSocket() is sometimes called without WL_LATCH_SET, though. I am marking this patch as ready-for-committer. Nathan
Hello. At Tue, 27 Jul 2021 11:04:09 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in > On Mon, Jul 26, 2021 at 11:03 PM Bossart, Nathan <bossartn@amazon.com> wrote: > PSA v3 patch. I have some comments. - No harm, but it's pointless to feed MyLatch to WaitLatch when WL_LATCH_SET is not set (or rather misleading). - It seems that the additional wait-event is effectively useless for most of the processes. Considering that this feature is for debugging purpose, it'd be better to use ps display instead (or additionally) if we want to see the wait event anywhere. The events of autovacuum workers can be seen in pg_stat_activity properly. For client-backends, that state cannot be seen in pg_stat_activity. That seems inevitable since backends aren't allocated a PGPROC entry yet at that time. (So the wait event is set to local memory as a safety measure in this case.) On the other hand, I find it inconvenient that the ps display is shown as just "postgres" while in that state. I think we can show "postgres: preauth waiting" or something. (It is shown as "postgres: username dbname [conn] initializing" while PostAuthDelay) Background workers behave the same way to client backends for the same reason to the above. We might be able to *fix* that but I'm not sure it's worth doing that only for this specific case. Autovacuum launcher is seen in pg_stat_activity but clients cannot start connection before autovac launcher starts unless unless process startup time is largely fluctuated. So the status is effectively useless in regard to the process. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Wed, Jul 28, 2021 at 8:12 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > Hello. > > At Tue, 27 Jul 2021 11:04:09 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in > > On Mon, Jul 26, 2021 at 11:03 PM Bossart, Nathan <bossartn@amazon.com> wrote: > > PSA v3 patch. > > I have some comments. > > - No harm, but it's pointless to feed MyLatch to WaitLatch when > WL_LATCH_SET is not set (or rather misleading). +1. I can send NULL to WaitLatch. > - It seems that the additional wait-event is effectively useless for > most of the processes. Considering that this feature is for debugging > purpose, it'd be better to use ps display instead (or additionally) > if we want to see the wait event anywhere. Hm. That's a good idea to show up in the ps display. > The events of autovacuum workers can be seen in pg_stat_activity properly. > > For client-backends, that state cannot be seen in > pg_stat_activity. That seems inevitable since backends aren't > allocated a PGPROC entry yet at that time. (So the wait event is set > to local memory as a safety measure in this case.) On the other hand, > I find it inconvenient that the ps display is shown as just "postgres" > while in that state. I think we can show "postgres: preauth waiting" > or something. (It is shown as "postgres: username dbname [conn] > initializing" while PostAuthDelay) Hm. Is n't it better to show something like below in the ps display? for pre_auth_delay: "postgres: pre auth delay" for post_auth_delay: "postgres: <<existing message>> post auth delay" But, I'm not sure whether this ps display thing will add any value to the end user who always can't see the ps display. So, how about having both i.e. ps display (useful for pre auth delay cases) and wait event (useful for post auth delay)? Regards, Bharath Rupireddy.
Justin Pryzby <pryzby@telsasoft.com> writes:
> On Wed, Jul 28, 2021 at 09:10:35PM +0530, Bharath Rupireddy wrote:
>> Hm. That's a good idea to show up in the ps display.
> Keep in mind that ps is apparently so expensive under windows that the GUC
> defaults to off.
> The admin can leave the ps display off, but I wonder if it's of any concern
> that something so expensive can be caused by an unauthenticated connection.
I'm detecting a certain amount of lily-gilding here. Neither of these
delays are meant for anything except debugging purposes, and nobody as
far as I've heard has ever expressed great concern about identifying
which process they need to attach to for that purpose. So I think it
is a *complete* waste of time to add any cycles to connection startup
to make these delays more visible.
I follow the idea of using WaitLatch to ensure that the delays are
interruptible by postmaster signals, but even that isn't worth a
lot given the expected use of these things. I don't see a need to
expend any extra effort on wait-reporting.
regards, tom lane
On 7/28/21, 11:32 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote: > I'm detecting a certain amount of lily-gilding here. Neither of these > delays are meant for anything except debugging purposes, and nobody as > far as I've heard has ever expressed great concern about identifying > which process they need to attach to for that purpose. So I think it > is a *complete* waste of time to add any cycles to connection startup > to make these delays more visible. > > I follow the idea of using WaitLatch to ensure that the delays are > interruptible by postmaster signals, but even that isn't worth a > lot given the expected use of these things. I don't see a need to > expend any extra effort on wait-reporting. +1. The proposed patch doesn't make the delay visibility any worse than what's already there. Nathan
On Wed, Jul 28, 2021 at 08:28:12PM +0000, Bossart, Nathan wrote: > On 7/28/21, 11:32 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote: >> I follow the idea of using WaitLatch to ensure that the delays are >> interruptible by postmaster signals, but even that isn't worth a >> lot given the expected use of these things. I don't see a need to >> expend any extra effort on wait-reporting. > > +1. The proposed patch doesn't make the delay visibility any worse > than what's already there. Agreed to just drop the patch (my opinion about this patch is unchanged). Not to mention that wait events are not available at SQL level at this stage yet. -- Michael
Attachment
At Thu, 29 Jul 2021 09:52:08 +0900, Michael Paquier <michael@paquier.xyz> wrote in > On Wed, Jul 28, 2021 at 08:28:12PM +0000, Bossart, Nathan wrote: > > On 7/28/21, 11:32 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote: > >> I follow the idea of using WaitLatch to ensure that the delays are > >> interruptible by postmaster signals, but even that isn't worth a > >> lot given the expected use of these things. I don't see a need to > >> expend any extra effort on wait-reporting. > > > > +1. The proposed patch doesn't make the delay visibility any worse > > than what's already there. > > Agreed to just drop the patch (my opinion about this patch is > unchanged). Not to mention that wait events are not available at SQL > level at this stage yet. I'm +1 to not adding wait event stuff at all. So the only advantage this patch would offer is interruptivity. I vote +-0.0 for adding that interruptivity (+1.0 from the previous opinion of mine:p). regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On 7/29/21, 12:59 AM, "Kyotaro Horiguchi" <horikyota.ntt@gmail.com> wrote: > At Thu, 29 Jul 2021 09:52:08 +0900, Michael Paquier <michael@paquier.xyz> wrote in >> On Wed, Jul 28, 2021 at 08:28:12PM +0000, Bossart, Nathan wrote: >> > On 7/28/21, 11:32 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote: >> >> I follow the idea of using WaitLatch to ensure that the delays are >> >> interruptible by postmaster signals, but even that isn't worth a >> >> lot given the expected use of these things. I don't see a need to >> >> expend any extra effort on wait-reporting. >> > >> > +1. The proposed patch doesn't make the delay visibility any worse >> > than what's already there. >> >> Agreed to just drop the patch (my opinion about this patch is >> unchanged). Not to mention that wait events are not available at SQL >> level at this stage yet. > > I'm +1 to not adding wait event stuff at all. So the only advantage > this patch would offer is interruptivity. I vote +-0.0 for adding that > interruptivity (+1.0 from the previous opinion of mine:p). I'm still in favor of moving to WaitLatch() for pre/post_auth_delay, but I don't think we should worry about the wait reporting stuff. The patch doesn't add a tremendous amount of complexity, it improves the behavior on postmaster crashes, and it follows the best practice described in pgsleep.c of using WaitLatch() for long sleeps. Nathan