Re: [PATCH] lock_timeout and common SIGALRM framework

2012-06-19 19:19 keltezéssel, Alvaro Herrera írta:
> Excerpts from Boszormenyi Zoltan's message of mar jun 19 12:44:04 -0400 2012:
>
>> OK, all 4 Check* functions are now moved back into proc.c,
>> nothing outside of timeout.c touches anything in it. New patches
>> are attached.
> Yeah, I like this one better, thanks.

Thanks. It seems I am on the right track then. :-)

> It seems to me that the "check" functions are no longer "check" anymore,
> right?  I mean, they don't check whether the time has expired.  It can
> be argued that CheckDeadLock is well named, because it does check
> whether there is a deadlock before doing anything else; but
> CheckStandbyTimeout is no longer a check -- it just sends a signal.
> Do we need to rename these functions?

Well, maybe. How about *Function() instead of Check*()?

> Why are you using the deadlock timeout for authentication?

Because it originally also used the deadlock timeout.
I agree that it was abusing code that's written for something else.

>    Wouldn't it
> make more sense to have a separate TimeoutName, just to keep things
> clean?

Yes, sure. Can you tell me a way to test authentication timeout?
"pre_auth_delay" is not good for testing it.

Changing authentication_timeout to 1sec both in the GUC list and the initial value of it in
postmaster.c plus adding this code below after enabling the auth timeout didn't help.
---8<------8<------8<------8<---
if (AuthenticationTimeout > 0)
     pg_usleep((AuthenticationTimeout + 2) * 1000000L);
---8<------8<------8<------8<---

I got this despite authentication_timeout being 1 second:

---8<------8<------8<------8<---
============== removing existing temp installation    ==============
============== creating temporary installation        ==============
============== initializing database system           ==============
============== starting postmaster                    ==============

pg_regress: postmaster did not respond within 60 seconds
Examine .../postgresql.1/src/test/regress/log/postmaster.log for the reason
---8<------8<------8<------8<---

Anyway, it doesn't seem to me that the code after enabling auth timeout
actually uses anything from the timeout code but the side effect of
a signal interrupting read() on the client connection socket and logs
"incomplete startup packet" or "invalid length of startup packet".
If that's true, then it can use its own (dummy) timeout and I modified
postmaster.c and timeout.c accordingly. It survives "make check".

> The "NB:" comment here doesn't seem to be useful anymore:
> + /*****************************************************************************
> +  * Init, Destroy and Check functions for different timeouts.
> +  *
> +  * NB: all Check* functions are run inside a signal handler, so be very wary
> +  * about what is done in them or in called routines.
> +  *****************************************************************************/

I removed it.

> In base_timeouts you don't initialize fin_time for any of the members.
> This is probably unimportant but then why initialize start_time?

The answer is "out of habit" and "leftover".
But now the checks are only done for active timeouts,
I think neither of these are needed to be initialized now.
Thanks for spotting it. It survives "make check".

Attached are the new patches with these changes.

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
      http://www.postgresql.at/