Use WaitLatch for {pre, post}_auth_delay instead of pg_usleep

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] - /messages/by-id/YOv8Yxd5zrbr3k+H@paquier.xyz
[2] - /messages/by-id/162764.1624892517@sss.pgh.pa.us
[3] - /messages/by-id/20210705.145251.462698229911576780.horikyota.ntt@gmail.com
[4] - /messages/by-id/20210705155553.GD20766@tamriel.snowman.net
[5] - /messages/by-id/YOOnlP4NtWVzfsyb@paquier.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]/messages/by-id/CALj2ACVF8AZi1bK8oH-Qoz3tYVpqFuzxcDRPdF-3p5BvF6GTxA@mail.gmail.com.
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]/messages/by-id/CALj2ACVF8AZi1bK8oH-Qoz3tYVpqFuzxcDRPdF-3p5BvF6GTxA@mail.gmail.com.

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]: /messages/by-id/CALj2ACVF8AZi1bK8oH-Qoz3tYVpqFuzxcDRPdF-3p5BvF6GTxA@mail.gmail.com

Regards,
Bharath Rupireddy.

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.

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.

On Wed, Jul 28, 2021 at 09:10:35PM +0530, Bharath Rupireddy wrote:

On Wed, Jul 28, 2021 at 8:12 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

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

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.

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

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