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

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