subject:"\[HACKERS\] \[PATCH\] lock_timeout and common SIGALRM framework"


2012-07-11 21:47 keltezéssel, Tom Lane írta:

Boszormenyi Zoltan z...@cybertec.at writes:

Attached are the refreshed patches. InitializeTimeouts() can be called
twice and PGSemaphoreTimedLock() returns bool now. This saves
two calls to get_timeout_indicator().

I'm starting to look at this patch now.  There are a number of cosmetic
things I don't care for, the biggest one being the placement of
timeout.c under storage/lmgr/.  That seems an entirely random place,
since the functionality provided has got nothing to do with storage
let alone locks.  I'm inclined to think that utils/misc/ is about
the best option in the existing backend directory hierarchy.  Anybody
object to that, or have a better idea?


Good idea, storage/lmgr/timeout.c was chosen simply because
it was born out of files living there.


Another thing that needs some discussion is the handling of
InitializeTimeouts.  As designed, I think it's completely unsafe,
the reason being that if a process using timeouts forks off another
one, the child will inherit the parent's timeout reasons and be unable
to reset them.  Right now this might not be such a big problem because
the postmaster doesn't need any timeouts, but what if it does in the
future?  So I think we should drop the base_timeouts_initialized
protection, and that means we need a pretty consistent scheme for
where to call InitializeTimeouts.  But we already have the same issue
with respect to on_proc_exit callbacks, so we can just add
InitializeTimeouts calls in the same places as on_exit_reset().

Comments?

I'll work up a revised patch and post it.

regards, tom lane




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


--
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] [PATCH] lock_timeout and common SIGALRM framework


2012-07-11 22:59 keltezéssel, Tom Lane írta:

I wrote:

I'm starting to look at this patch now.

After reading this further, I think that the sched_next option is a
bad idea and we should get rid of it.  AFAICT, what it is meant to do
is (if !sched_next) automatically do disable_all_timeouts(true) if
the particular timeout happens to fire.  But there is no reason the
timeout's callback function couldn't do that; and doing it in the
callback is more flexible since you could have logic about whether to do
it or not, rather than freezing the decision at RegisterTimeout time.
Moreover, it does not seem to me to be a particularly good idea to
encourage timeouts to have such behavior, anyway.  Each time we add
another timeout we'd have to look to see if it's still sane for each
existing timeout to use !sched_next.  It would likely be better, in
most cases, for individual callbacks to explicitly disable any other
individual timeout reasons that should no longer be fired.


+1


I am also underwhelmed by the timeout_start callback function concept.


It was generalized out of static TimestampTz timeout_start_time;
in proc.c which is valid if deadlock_timeout is activated. It is used
in ProcSleep() for logging the time difference between the time when
the timeout was activated and now at several places in that function.


In the first place, that's broken enable_timeout, which incorrectly
assumes that the value it gets must be now (see its schedule_alarm
call).


You're right, it must be fixed.


   In the second place, it seems fairly likely that callers of
get_timeout_start would likewise want the clock time at which the
timeout was enabled, not the timeout_start reference time.  (If they
did want the latter, why couldn't they get it from wherever the callback
function had gotten it?)  I'm inclined to propose that we drop the
timeout_start concept and instead provide two functions for scheduling
interrupts:

enable_timeout_after(TimeoutName tn, int delay_ms);
enable_timeout_at(TimeoutName tn, TimestampTz fin_time);

where you use the former if you want the standard GetCurrentTimestamp +
n msec calculation, but if you want the stop time calculated in some
other way, you calculate it yourself and use the second function.

regards, tom lane




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


--
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] [PATCH] lock_timeout and common SIGALRM framework


2012-07-12 19:05 keltezéssel, Tom Lane írta:

Here is a revised version of the timeout-infrastructure patch.
I whacked it around quite a bit, notably:

* I decided that the most convenient way to handle the initialization
issue was to combine establishment of the signal handler with resetting
of the per-process variables.  So handle_sig_alarm is no longer global,
and InitializeTimeouts is called at the places where we used to do
pqsignal(SIGALRM, handle_sig_alarm);.  I believe this is correct
because any subprocess that was intending to use SIGALRM must have
called that before establishing any timeouts.


OK.


* BTW, doing that exposed the fact that walsender processes were failing
to establish a SIGALRM signal handler, which is a pre-existing bug,
because they run a normal authentication transaction during InitPostgres
and hence need to be able to cope with deadlock and statement timeouts.
I will do something about back-patching a fix for that.


Wow, my work uncovered a pre-existing bug in PostgreSQL. :-)


* I ended up putting the RegisterTimeout calls for DEADLOCK_TIMEOUT
and STATEMENT_TIMEOUT into InitPostgres, ensuring that they'd get
done in walsender and autovacuum processes.  I'm not totally satisfied
with that, but for sure it didn't work to only establish them in
regular backends.

* I didn't like the TimeoutName nomenclature, because to me name
suggests that the value is a string, not just an enum.  So I renamed
that to TimeoutId.


OK.


* I whacked around the logic in timeout.c a fair amount, because it
had race conditions if SIGALRM happened while enabling or disabling
a timeout.  I believe the attached coding is safe, but I'm not totally
happy with it from a performance standpoint, because it will do two
setitimer calls (a disable and then a re-enable) in many cases where
the old code did only one.


Disabling deadlock timeout, a.k.a. disable_sig_alarm(false) in
the original code called setitimer() twice if statement timeout
was still in effect, it was done in CheckStatementTimeout().
Considering this, I think there is no performance problem with
the new code you came up with.


I think what we ought to do is go ahead and apply this, so that we
can have the API nailed down, and then we can revisit the internals
of timeout.c to see if we can't get the performance back up.
It's clearly a much cleaner design than the old spaghetti logic in
proc.c, so I think we ought to go ahead with this independently of
whether the second patch gets accepted.


There is one tiny bit you might have broken. You wrote previously:


I am also underwhelmed by the timeout_start callback function concept.
In the first place, that's broken enable_timeout, which incorrectly
assumes that the value it gets must be now (see its schedule_alarm
call).  In the second place, it seems fairly likely that callers of
get_timeout_start would likewise want the clock time at which the
timeout was enabled, not the timeout_start reference time.  (If they
did want the latter, why couldn't they get it from wherever the callback
function had gotten it?)  I'm inclined to propose that we drop the
timeout_start concept and instead provide two functions for scheduling
interrupts:

enable_timeout_after(TimeoutName tn, int delay_ms);
enable_timeout_at(TimeoutName tn, TimestampTz fin_time);

where you use the former if you want the standard GetCurrentTimestamp +
n msec calculation, but if you want the stop time calculated in some
other way, you calculate it yourself and use the second function.


It's okay, but you haven't really followed it with STATEMENT_TIMEOUT:

-8--8--8--8--8-
*** 2396,2404 
/* Set statement timeout running, if any */
/* NB: this mustn't be enabled until we are within an xact */
if (StatementTimeout  0)
!   enable_sig_alarm(StatementTimeout, true);
else
!   cancel_from_timeout = false;

xact_started = true;
}
--- 2397,2405 
/* Set statement timeout running, if any */
/* NB: this mustn't be enabled until we are within an xact */
if (StatementTimeout  0)
!   enable_timeout_after(STATEMENT_TIMEOUT, 
StatementTimeout);
else
!   disable_timeout(STATEMENT_TIMEOUT, false);

xact_started = true;
}
-8--8--8--8--8-

It means that StatementTimeout losts its precision. It would trigger
in the future counting from now instead of counting from
GetCurrentStatementStartTimestamp(). It should be:

enable_timeout_at(STATEMENT_TIMEOUT,
TimestampTzPlusMilliseconds(GetCurrentStatementStartTimestamp(), 
StatementTimeout));


I haven't really looked at the second patch yet, but at minimum that
will need some rebasing to match the API tweaks here.


Yes, I will do that.

Thanks for your

Re: [HACKERS] [PATCH] lock_timeout and common SIGALRM framework


2012-07-13 22:32 keltezéssel, Boszormenyi Zoltan írta:

2012-07-12 19:05 keltezéssel, Tom Lane írta:


I haven't really looked at the second patch yet, but at minimum that
will need some rebasing to match the API tweaks here.


Yes, I will do that.


While doing it, I discovered another bug you introduced.
enable_timeout_after(..., 0); would set an alarm instead of ignoring it.
Try SET deadlock_timeout = 0;

Same for enable_timeout_at(..., fin_time): if fin_time points to the past,
it enables a huge timeout that wouldn't possibly trigger for short
transactions but it's a bug nevertheless.



Thanks for your review and work.

Best regards,
Zoltán Böszörményi




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


--
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] [PATCH] lock_timeout and common SIGALRM framework

Boszormenyi Zoltan z...@cybertec.at writes:
 It means that StatementTimeout losts its precision. It would trigger
 in the future counting from now instead of counting from
 GetCurrentStatementStartTimestamp().

That is, in fact, better not worse.  Note the comment in the existing
code:

 * Begin statement-level timeout
 *
 * Note that we compute statement_fin_time with reference to the
 * statement_timestamp, but apply the specified delay without any
 * correction; that is, we ignore whatever time has elapsed since
 * statement_timestamp was set.  In the normal case only a small
 * interval will have elapsed and so this doesn't matter, but there
 * are corner cases (involving multi-statement query strings with
 * embedded COMMIT or ROLLBACK) where we might re-initialize the
 * statement timeout long after initial receipt of the message. In
 * such cases the enforcement of the statement timeout will be a bit
 * inconsistent.  This annoyance is judged not worth the cost of
 * performing an additional gettimeofday() here.

That is, measuring from GetCurrentStatementStartTimestamp is a hack to
save one gettimeofday call, at the cost of making the timeout less
accurate, sometimes significantly so.  In the new model there isn't any
good way to duplicate this kluge (in particular, there's no point in
using enable_timeout_at, because that will still make a gettimeofday
call).  That doesn't bother me too much.  I'd rather try to buy back
whatever performance was lost by seeing if we can reduce the number of
setitimer calls.

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] [PATCH] lock_timeout and common SIGALRM framework

Boszormenyi Zoltan z...@cybertec.at writes:
 While doing it, I discovered another bug you introduced.
 enable_timeout_after(..., 0); would set an alarm instead of ignoring it.
 Try SET deadlock_timeout = 0;

Hm.  I don't think it's a bug for enable_timeout_after(..., 0) to cause
a timeout ... but we'll have to change the calling code.  Thanks for
catching that.

 Same for enable_timeout_at(..., fin_time): if fin_time points to the past,
 it enables a huge timeout

No, it should cause an immediate interrupt, or at least after 1
microsecond.  Look at TimestampDifference.

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] [PATCH] lock_timeout and common SIGALRM framework


2012-07-13 23:51 keltezéssel, Tom Lane írta:

Boszormenyi Zoltan z...@cybertec.at writes:

While doing it, I discovered another bug you introduced.
enable_timeout_after(..., 0); would set an alarm instead of ignoring it.
Try SET deadlock_timeout = 0;

Hm.  I don't think it's a bug for enable_timeout_after(..., 0) to cause
a timeout ... but we'll have to change the calling code.  Thanks for
catching that.


You're welcome. This caused a segfault in my second patch,
the code didn't expect enable_timeout_after(..., 0);
to set up a timer.

So, the calling code should check for the value and not call
enable_timeout_*() when it shouldn't, right? It's making the code
more obvious for the casual reader, I agree it's better that way.

Will you post a new version with callers checking their *Timeout settings
or commit it with this change? I can then post a new second patch.

Regarding the lock_timeout functionality: the patch can be reduced to
about half of its current size and it would be a lot less intrusive if the
LockAcquire() callers don't need to report the individual object types
and names or OIDs. Do you prefer the verbose ereport()s or a
generic one about lock timeout triggered in ProcSleep()?


Same for enable_timeout_at(..., fin_time): if fin_time points to the past,
it enables a huge timeout

No, it should cause an immediate interrupt, or at least after 1
microsecond.  Look at TimestampDifference.


Okay.

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


--
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] [PATCH] lock_timeout and common SIGALRM framework

2012-07-13 Thread Alvaro Herrera


Excerpts from Boszormenyi Zoltan's message of vie jul 13 18:11:27 -0400 2012:

 Regarding the lock_timeout functionality: the patch can be reduced to
 about half of its current size and it would be a lot less intrusive if the
 LockAcquire() callers don't need to report the individual object types
 and names or OIDs. Do you prefer the verbose ereport()s or a
 generic one about lock timeout triggered in ProcSleep()?

For what it's worth, I would appreciate it if you would post the lock
timeout patch for the upcoming commitfest.  This one is already almost a
month long now.  That way we can close this CF item soon and concentrate
on the remaining patches.  This one has received its fair share of
committer attention already, ISTM.

-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

-- 
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] [PATCH] lock_timeout and common SIGALRM framework

Boszormenyi Zoltan z...@cybertec.at writes:
 Try SET deadlock_timeout = 0;

Actually, when I try that I get

ERROR:  0 is outside the valid range for parameter deadlock_timeout (1 .. 
2147483647)

So I don't see any bug here.  The places that are unconditionally doing
enable_timeout_after(..., DeadlockTimeout); are perfectly fine.  The
only place that might need an if-test has already got one:

  if (StatementTimeout  0)
! enable_timeout_after(STATEMENT_TIMEOUT, StatementTimeout);
  else
! disable_timeout(STATEMENT_TIMEOUT, false);


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] [PATCH] lock_timeout and common SIGALRM framework

Alvaro Herrera alvhe...@commandprompt.com writes:
 For what it's worth, I would appreciate it if you would post the lock
 timeout patch for the upcoming commitfest.

+1.  I think it's reasonable to get the infrastructure patch in now,
but we are running out of time in this commitfest (and there's still
a lot of patches that haven't been reviewed at all).

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] [PATCH] lock_timeout and common SIGALRM framework

2012-07-12 Thread Tom Lane

Here is a revised version of the timeout-infrastructure patch.
I whacked it around quite a bit, notably:

* I decided that the most convenient way to handle the initialization
issue was to combine establishment of the signal handler with resetting
of the per-process variables.  So handle_sig_alarm is no longer global,
and InitializeTimeouts is called at the places where we used to do
pqsignal(SIGALRM, handle_sig_alarm);.  I believe this is correct
because any subprocess that was intending to use SIGALRM must have
called that before establishing any timeouts.

* BTW, doing that exposed the fact that walsender processes were failing
to establish a SIGALRM signal handler, which is a pre-existing bug,
because they run a normal authentication transaction during InitPostgres
and hence need to be able to cope with deadlock and statement timeouts.
I will do something about back-patching a fix for that.

* I ended up putting the RegisterTimeout calls for DEADLOCK_TIMEOUT
and STATEMENT_TIMEOUT into InitPostgres, ensuring that they'd get
done in walsender and autovacuum processes.  I'm not totally satisfied
with that, but for sure it didn't work to only establish them in
regular backends.

* I didn't like the TimeoutName nomenclature, because to me name
suggests that the value is a string, not just an enum.  So I renamed
that to TimeoutId.

* I whacked around the logic in timeout.c a fair amount, because it
had race conditions if SIGALRM happened while enabling or disabling
a timeout.  I believe the attached coding is safe, but I'm not totally
happy with it from a performance standpoint, because it will do two
setitimer calls (a disable and then a re-enable) in many cases where
the old code did only one.

I think what we ought to do is go ahead and apply this, so that we
can have the API nailed down, and then we can revisit the internals
of timeout.c to see if we can't get the performance back up.
It's clearly a much cleaner design than the old spaghetti logic in
proc.c, so I think we ought to go ahead with this independently of
whether the second patch gets accepted.

I haven't really looked at the second patch yet, but at minimum that
will need some rebasing to match the API tweaks here.

regards, tom lane



binwIymnjnW5K.bin
Description: 1-timeout-framework-v16.patch.gz

-- 
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] [PATCH] lock_timeout and common SIGALRM framework

2012-07-11 Thread Tom Lane

Boszormenyi Zoltan z...@cybertec.at writes:
 Attached are the refreshed patches. InitializeTimeouts() can be called
 twice and PGSemaphoreTimedLock() returns bool now. This saves
 two calls to get_timeout_indicator().

I'm starting to look at this patch now.  There are a number of cosmetic
things I don't care for, the biggest one being the placement of
timeout.c under storage/lmgr/.  That seems an entirely random place,
since the functionality provided has got nothing to do with storage
let alone locks.  I'm inclined to think that utils/misc/ is about
the best option in the existing backend directory hierarchy.  Anybody
object to that, or have a better idea?

Another thing that needs some discussion is the handling of
InitializeTimeouts.  As designed, I think it's completely unsafe,
the reason being that if a process using timeouts forks off another
one, the child will inherit the parent's timeout reasons and be unable
to reset them.  Right now this might not be such a big problem because
the postmaster doesn't need any timeouts, but what if it does in the
future?  So I think we should drop the base_timeouts_initialized
protection, and that means we need a pretty consistent scheme for
where to call InitializeTimeouts.  But we already have the same issue
with respect to on_proc_exit callbacks, so we can just add
InitializeTimeouts calls in the same places as on_exit_reset().

Comments?

I'll work up a revised patch and post it.

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] [PATCH] lock_timeout and common SIGALRM framework

2012-07-11 Thread Alvaro Herrera


Excerpts from Tom Lane's message of mié jul 11 15:47:47 -0400 2012:
 
 Boszormenyi Zoltan z...@cybertec.at writes:
  Attached are the refreshed patches. InitializeTimeouts() can be called
  twice and PGSemaphoreTimedLock() returns bool now. This saves
  two calls to get_timeout_indicator().
 
 I'm starting to look at this patch now.  There are a number of cosmetic
 things I don't care for, the biggest one being the placement of
 timeout.c under storage/lmgr/.  That seems an entirely random place,
 since the functionality provided has got nothing to do with storage
 let alone locks.  I'm inclined to think that utils/misc/ is about
 the best option in the existing backend directory hierarchy.  Anybody
 object to that, or have a better idea?

I agree with the proposed new location.

 Another thing that needs some discussion is the handling of
 InitializeTimeouts.  As designed, I think it's completely unsafe,
 the reason being that if a process using timeouts forks off another
 one, the child will inherit the parent's timeout reasons and be unable
 to reset them.  Right now this might not be such a big problem because
 the postmaster doesn't need any timeouts, but what if it does in the
 future?  So I think we should drop the base_timeouts_initialized
 protection, and that means we need a pretty consistent scheme for
 where to call InitializeTimeouts.  But we already have the same issue
 with respect to on_proc_exit callbacks, so we can just add
 InitializeTimeouts calls in the same places as on_exit_reset().

I do agree that InitializeTimeouts is not optimally placed.  We
discussed this upthread.

Some of the calls of on_exit_reset() are placed in code that's about to
die.  Surely we don't need InitializeTimeouts() then.  Maybe we should
have another routine, say InitializeProcess (noting we already
InitProcess so maybe some name would be good), that calls both
on_exit_reset and InitializeTimeouts.

-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

-- 
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] [PATCH] lock_timeout and common SIGALRM framework

2012-07-11 Thread Tom Lane

I wrote:
 I'm starting to look at this patch now.

After reading this further, I think that the sched_next option is a
bad idea and we should get rid of it.  AFAICT, what it is meant to do
is (if !sched_next) automatically do disable_all_timeouts(true) if
the particular timeout happens to fire.  But there is no reason the
timeout's callback function couldn't do that; and doing it in the
callback is more flexible since you could have logic about whether to do
it or not, rather than freezing the decision at RegisterTimeout time.
Moreover, it does not seem to me to be a particularly good idea to
encourage timeouts to have such behavior, anyway.  Each time we add
another timeout we'd have to look to see if it's still sane for each
existing timeout to use !sched_next.  It would likely be better, in
most cases, for individual callbacks to explicitly disable any other
individual timeout reasons that should no longer be fired.

I am also underwhelmed by the timeout_start callback function concept.
In the first place, that's broken enable_timeout, which incorrectly
assumes that the value it gets must be now (see its schedule_alarm
call).  In the second place, it seems fairly likely that callers of
get_timeout_start would likewise want the clock time at which the
timeout was enabled, not the timeout_start reference time.  (If they
did want the latter, why couldn't they get it from wherever the callback
function had gotten it?)  I'm inclined to propose that we drop the
timeout_start concept and instead provide two functions for scheduling
interrupts:

enable_timeout_after(TimeoutName tn, int delay_ms);
enable_timeout_at(TimeoutName tn, TimestampTz fin_time);

where you use the former if you want the standard GetCurrentTimestamp +
n msec calculation, but if you want the stop time calculated in some
other way, you calculate it yourself and use the second function.

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] [PATCH] lock_timeout and common SIGALRM framework

2012-07-11 Thread Tom Lane

Alvaro Herrera alvhe...@commandprompt.com writes:
 Excerpts from Tom Lane's message of miÃ© jul 11 15:47:47 -0400 2012:
 ... that means we need a pretty consistent scheme for
 where to call InitializeTimeouts.  But we already have the same issue
 with respect to on_proc_exit callbacks, so we can just add
 InitializeTimeouts calls in the same places as on_exit_reset().

 I do agree that InitializeTimeouts is not optimally placed.  We
 discussed this upthread.

 Some of the calls of on_exit_reset() are placed in code that's about to
 die.  Surely we don't need InitializeTimeouts() then.  Maybe we should
 have another routine, say InitializeProcess (noting we already
 InitProcess so maybe some name would be good), that calls both
 on_exit_reset and InitializeTimeouts.

Yeah, I was wondering about that too, but it seems a bit ad-hoc from a
modularity standpoint.  I gave some consideration to the idea of putting
these calls directly into fork_process(), but we'd have to be very sure
that there would never be a case where it was incorrect to do them after
forking.

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] [PATCH] lock_timeout and common SIGALRM framework

2012-07-04 Thread Alvaro Herrera


Excerpts from Boszormenyi Zoltan's message of mié jul 04 06:32:46 -0400 2012:
 2012-07-04 12:09 keltezéssel, Boszormenyi Zoltan írta:

  You just broke initdb with this cleanup. :-)

Ouch.

  initdb starts postgres --single, that doesn't do BackendInitialize(),
  only PostgresMain(). So, you need InitializeTimeouts() before
  the RegisterTimeout() calls in PostgresMain and the elog(PANIC)
  must not be in InitializeTimeouts() if called twice.
 
 Attached is the fix for this problem. PostgresMain() has a new
 argument: bool single_user. This way, InitializeTimeouts() can
 keep its elog(PANIC) if called twice and postgres --single
 doesn't fail its Assert() in RegisterTimeout().

Hmm.  Maybe it's better to leave InitializeTimeouts to be called twice
after all.  The fix seems a lot uglier than the disease it's curing.

-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

-- 
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] [PATCH] lock_timeout and common SIGALRM framework

2012-07-04 Thread Alvaro Herrera


Excerpts from Boszormenyi Zoltan's message of mié jul 04 07:03:44 -0400 2012:
 
 2012-07-03 23:38 keltezéssel, Alvaro Herrera írta:
 
  I don't understand why PGSemaphoreTimedLock() is not broken.  I mean
  surely you need a bool return to let the caller know whether the
  acquisition succeeded or failed?
 
 Well, this is the same interface PGSemaphoreTryLock() uses.
 By this reasoning, it's also broken.

Eh, no -- as far as I can see, PGSemaphoreTryLock returns a boolean,
which is precisely my point.

-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

-- 
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] [PATCH] lock_timeout and common SIGALRM framework


2012-07-04 17:25 keltezéssel, Alvaro Herrera írta:

Excerpts from Boszormenyi Zoltan's message of mié jul 04 07:03:44 -0400 2012:

2012-07-03 23:38 keltezéssel, Alvaro Herrera írta:

I don't understand why PGSemaphoreTimedLock() is not broken.  I mean
surely you need a bool return to let the caller know whether the
acquisition succeeded or failed?

Well, this is the same interface PGSemaphoreTryLock() uses.
By this reasoning, it's also broken.

Eh, no -- as far as I can see, PGSemaphoreTryLock returns a boolean,
which is precisely my point.


You're right. I blame the heat for not being able to properly
read my own code.

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


--
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] [PATCH] lock_timeout and common SIGALRM framework

2012-07-03 23:31 keltezéssel, Alvaro Herrera írta:

Excerpts from Boszormenyi Zoltan's message of vie jun 29 14:30:28 -0400 2012:

Does anyone have a little time to look at the latest timeout framework
with the registration interface and the 2nd patch too? I am at work
until Friday next week, after that I will be on vacation for two weeks.
Just in case there is anything that needs tweaking to make it more
acceptable.

I cleaned up this a bit more and now I think it's ready to commit --
as soon as somebody tests that the standby bits still work.

You just broke initdb with this cleanup. :-)

---8--8--8--8--8--8--8---
$ cat src/test/regress/log/initdb.log
Running in noclean mode. Mistakes will not be cleaned up.
The files belonging to this database system will be owned by user zozo.
This user must also own the server process.

The database cluster will be initialized with locales
COLLATE: hu_HU.utf8
CTYPE:hu_HU.utf8
MESSAGES: C
MONETARY: hu_HU.utf8
NUMERIC: hu_HU.utf8
TIME: hu_HU.utf8
The default database encoding has accordingly been set to UTF8.
The default text search configuration will be set to hungarian.

creating directory
/home/zozo/lock-timeout/9.1/1/postgresql.14/src/test/regress/./tmp_check/data ... ok

creating subdirectories ... ok
selecting default max_connections ... 100
selecting default shared_buffers ... 32MB
creating configuration files ... ok
creating template1 database in
/home/zozo/lock-timeout/9.1/1/postgresql.14/src/test/regress/./tmp_check/data/base/1 ... ok
initializing pg_authid ... TRAP: FailedAssertion(!(base_timeouts_initialized), File:
timeout.c, Line: 217)
sh: line 1: 29872 Aborted (core dumped)
/home/zozo/lock-timeout/9.1/1/postgresql.14/src/test/regress/tmp_check/install/home/zozo/pgc92dev-locktimeout/bin/postgres
--single -F -O -c search_path=pg_catalog -c exit_on_error=true template1 /dev/null

child process exited with exit code 134
initdb: data directory
/home/zozo/lock-timeout/9.1/1/postgresql.14/src/test/regress/./tmp_check/data not
removed at user's request

---8--8--8--8--8--8--8---

initdb starts postgres --single, that doesn't do BackendInitialize(),
only PostgresMain(). So, you need InitializeTimeouts() before
the RegisterTimeout() calls in PostgresMain and the elog(PANIC)
must not be in InitializeTimeouts() if called twice.

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

--
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] [PATCH] lock_timeout and common SIGALRM framework


2012-07-04 12:09 keltezéssel, Boszormenyi Zoltan írta:

2012-07-03 23:31 keltezéssel, Alvaro Herrera írta:

Excerpts from Boszormenyi Zoltan's message of vie jun 29 14:30:28 -0400 2012:


Does anyone have a little time to look at the latest timeout framework
with the registration interface and the 2nd patch too? I am at work
until Friday next week, after that I will be on vacation for two weeks.
Just in case there is anything that needs tweaking to make it more
acceptable.

I cleaned up this a bit more and now I think it's ready to commit --
as soon as somebody tests that the standby bits still work.


You just broke initdb with this cleanup. :-)

---8--8--8--8--8--8--8---
$ cat src/test/regress/log/initdb.log
Running in noclean mode.  Mistakes will not be cleaned up.
The files belonging to this database system will be owned by user zozo.
This user must also own the server process.

The database cluster will be initialized with locales
  COLLATE:  hu_HU.utf8
  CTYPE:hu_HU.utf8
  MESSAGES: C
  MONETARY: hu_HU.utf8
  NUMERIC:  hu_HU.utf8
  TIME: hu_HU.utf8
The default database encoding has accordingly been set to UTF8.
The default text search configuration will be set to hungarian.

creating directory 
/home/zozo/lock-timeout/9.1/1/postgresql.14/src/test/regress/./tmp_check/data ... ok

creating subdirectories ... ok
selecting default max_connections ... 100
selecting default shared_buffers ... 32MB
creating configuration files ... ok
creating template1 database in 
/home/zozo/lock-timeout/9.1/1/postgresql.14/src/test/regress/./tmp_check/data/base/1 ... ok
initializing pg_authid ... TRAP: FailedAssertion(!(base_timeouts_initialized), File: 
timeout.c, Line: 217)
sh: line 1: 29872 Aborted (core dumped) 
/home/zozo/lock-timeout/9.1/1/postgresql.14/src/test/regress/tmp_check/install/home/zozo/pgc92dev-locktimeout/bin/postgres 
--single -F -O -c search_path=pg_catalog -c exit_on_error=true template1  /dev/null

child process exited with exit code 134
initdb: data directory 
/home/zozo/lock-timeout/9.1/1/postgresql.14/src/test/regress/./tmp_check/data not 
removed at user's request

---8--8--8--8--8--8--8---

initdb starts postgres --single, that doesn't do BackendInitialize(),
only PostgresMain(). So, you need InitializeTimeouts() before
the RegisterTimeout() calls in PostgresMain and the elog(PANIC)
must not be in InitializeTimeouts() if called twice.




Attached is the fix for this problem. PostgresMain() has a new
argument: bool single_user. This way, InitializeTimeouts() can
keep its elog(PANIC) if called twice and postgres --single
doesn't fail its Assert() in RegisterTimeout().

Comments?

Best regards,
Zoltán Böszörményi

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

diff -durpN postgresql.14.orig/src/backend/main/main.c postgresql.14/src/backend/main/main.c
--- postgresql.14.orig/src/backend/main/main.c	2012-06-26 09:10:21.272759354 +0200
+++ postgresql.14/src/backend/main/main.c	2012-07-04 12:21:58.869037014 +0200
@@ -192,7 +192,7 @@ main(int argc, char *argv[])
 	else if (argc  1  strcmp(argv[1], --describe-config) == 0)
 		GucInfoMain();			/* does not return */
 	else if (argc  1  strcmp(argv[1], --single) == 0)
-		PostgresMain(argc, argv, get_current_username(progname)); /* does not return */
+		PostgresMain(argc, argv, get_current_username(progname), true); /* does not return */
 	else
 		PostmasterMain(argc, argv); /* does not return */
 	abort();		/* should not get here */
diff -durpN postgresql.14.orig/src/backend/postmaster/postmaster.c postgresql.14/src/backend/postmaster/postmaster.c
--- postgresql.14.orig/src/backend/postmaster/postmaster.c	2012-07-04 12:25:09.247183727 +0200
+++ postgresql.14/src/backend/postmaster/postmaster.c	2012-07-04 12:23:22.933543240 +0200
@@ -3626,7 +3626,7 @@ BackendRun(Port *port)
 	 */
 	MemoryContextSwitchTo(TopMemoryContext);
 
-	PostgresMain(ac, av, port-user_name);
+	PostgresMain(ac, av, port-user_name, false);
 }
 
 
diff -durpN postgresql.14.orig/src/backend/tcop/postgres.c postgresql.14/src/backend/tcop/postgres.c
--- postgresql.14.orig/src/backend/tcop/postgres.c	2012-07-04 12:25:09.255183775 +0200
+++ postgresql.14/src/backend/tcop/postgres.c	2012-07-04 12:24:17.685873058 +0200
@@ -3509,7 +3509,7 @@ process_postgres_switches(int argc, char
  * 
  */
 void
-PostgresMain(int argc, char *argv[], const char *username)
+PostgresMain(int argc, char *argv[], const char *username, bool single_user)
 {
 	const char *dbname;
 	int			firstchar;
@@ -3603,8 +3603,11 @@ PostgresMain(int argc, char *argv[], con
 	{
 		/*
 		 * Register timeout sources needed by backend operation.  Note
-		 * that InitializeTimeout was already called by BackendInitialize.
+		 * that

Re: [HACKERS] [PATCH] lock_timeout and common SIGALRM framework