Re: Strange Windows problem, lock_timeout test request - Mailing list pgsql-hackers

2013-02-27 04:06 keltezéssel, Stephen Frost írta:
> Zoltan,
>
> * Boszormenyi Zoltan (zb@cybertec.at) wrote:
>> attached is v30, I hope with everything fixed.
> Making progress, certainly.
>
> Given the hack to the API of enable_timeout_after() and the need for
> timeout_reset_base_time(), I'm just going to voice my objection to the
> "accumulated wait time on locks" portion again.  I still like the idea
> of a timeout for waiting on relation-level locks, as we acquire those
> all up-front and we'd be able to just set a timeout at the appropriate
> point and then release it when we're past acquiring the relation-level
> locks.  Seems like that would be much cleaner.

Yes, it would be cleaner. But this way the total timeout the
statement would wait for locks is N * lock_timeout - <some delta>
in the worst case when all the locks can be acquired just under
the set timeout interval, N being the number of locks that statement
wants to acquire. For some use cases, it's better to have known limit
in the total amount of wait time. But unlike statement_timeout,
with lock_timeout_stmt the statement can still finish after this limit
as it does useful work besides waiting for locks.

> On the other hand, if we're going to go down this route, I'm really
> starting to wonder if it should be the timeout systems responsibility to
> keep track of such accumulated time.

Thinking about it a bit more, I start to agree with it.
It's not likely that any new timeout sources will get added
to proc.c that has anything to do with waiting across multiple locks.From this POV, this accumulated time can be done
byproc.c itself. 
But this makes it necessary to reschedule the timer from the
ProcSleep() loop so it increases the number of setitimer() calls.
But with clever coding, the "it_interval" part of struct itimerval
can be used to decrease the number of setitimer calls, so it may
be balanced out.

Another thought is that there is no need to have an extra function
to set the start time for the timeouts. It can be folded into
enable_timeout_after(), enable_timeout_at() and
enable_multiple_timeouts() and it simplifies the API, too.

Since setitimer() has microsecond resolution, I wonder whether
the timeout.c code shouldn't accept that, too. Especially if we want
to keep the per-statement accumulated version for the lock timeout.
Then time to wait that can be represented using int32 would be
about 35.8 minutes at most, we will need to use int64 if the maximum
number of millisecs is to stay as INT_MAX, which I guess it should.

Comments?

> Other than that..
>
>> - List based enable/disable_multiple_timeouts()
> That looks good, like the use of foreach(), etc, but I don't like how
> you've set up delay_ms as a pointer..?  Looks to be to allow you to
> initialize the TimeoutParams structs early in proc.c..?  Is there
> another reason it needs to be a pointer that I'm missing?  Why not build
> the TimeoutParam strcutures in the if() blocks that check if the GUCs
> are set..?

It's fixed in my tree and I also added an Assert to
enable_multiple_timeouts() to make sure all timeout sources
use either TIMEOUT_AT or TIMEOUT_AFTER. Some more
comments were also fixed. But I would like to send out the
new  patch after the above questions are settled.

I also want to thank you for your comments. It's good to have
a fresh look on this code.

>> - separate per-lock and per-statement lock_timeout variants
>> - modified comments and documentation
> Thanks.
>
>     Stephen


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