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

2013-02-24 16:14 keltezéssel, Boszormenyi Zoltan írta:
> 2013-02-24 15:03 keltezéssel, Stephen Frost írta:
>> * Boszormenyi Zoltan (zb@cybertec.at) wrote:
>>> 2013-02-24 03:23 keltezéssel, Stephen Frost írta:
>>>> No, it isn't.  Patches posted to the list should be in context diff
>>>> format (and uncompressed unless it's too large) for easier reading.
>>>> That avoids having to download it, apply it to a git tree and then
>>>> create the diff ourselves.  Indeed, the move to git had impact on this
>>>> at all.
>>> I remembered this mail from The Master(tm):
>>> http://www.postgresql.org/message-id/21555.1339866293@sss.pgh.pa.us
>> Which matches exactly what I was saying- context diff provides easier
>> readind for review purposes, which is presumably what you were looking
>> to have happen when you posted this patch to the mailing list. And it's
>> still the documented expectation and project policy, regardless of any
>> individual's email.  If we're going to change it, then the
>> documentation, et al, should be updated as well.
>>
>> For my 2c, I still prefer context diffs posted to the mailing lists,
>> but would also like to see more people posting pull requests. That
>> doesn't make it project policy though.
>>
>>> So, more than halfof the recently posted patches come
>>> directly from "git diff". The preference has changed.
>> No, more people have ignored the project policy than not- that doesn't
>> say anything about the preference or what the policy is.
>>
>>>> lock_timeout_wait and lock_timeout_stmt_wait ?
>>> Hm. How about without the "_wait" suffix?
>>> Or lock_timeout vs statement_lock_timeout?
>> I could live without the _wait suffix, but it occurs to me that none of
>> these really indicate that statement_lock_timeout is an accumulated
>> timeout.  Perhaps it should say 'total lock wait timeout' or similar?
>>
>>> statement_timeout is the legacy behaviour, it should be kept.
>>> It's behaviour is well understood: the statement finishes under
>>> the specified time or it throws an error. The problem from the
>>> application point of view is that the error can happen while
>>> the tuples are being transferred to the client, so the recordset
>>> can be cut in half.
>>>
>>> "lock_timeout_stmt" (or lock_timeout_option = per_statement)
>>> is somewhat extending the statement_timeout as only the
>>> time waiting on locks are counted, so SELECT FOR UPDATE/etc.
>>> may throw an error but if all rows are collected already, or
>>> plain SELECT is run, the application gets them all.
>>> This seems to follow the Microsoft SQL Server semantics:
>>> http://msdn.microsoft.com/en-us/library/ms189470.aspx
>> The documentation at that link appears to match what 'lock_timeout'
>> would do.  Note that it says 'a lock' here: "When a wait for a lock
>> exceeds the time-out value, an error is returned.", or have you tested
>> the actual behavior and seen it treat this value as an accumulated
>> time across all lock waits for an entire statement?
>
> (Note to myself: don't read with headache.)
> I may have misread the MSDN link.
>
>>
>>> The per-lock lock_timeout was the result of a big Informix
>>> project ported to PostgreSQL, this follows Informix semantics:
>>> http://publib.boulder.ibm.com/infocenter/idshelp/v10/index.jsp?topic=/com.ibm.sqls.doc/sqls788.htm
>>>
>> I agree that the Informix command looks to be per-lock, but based on my
>> simple reading of both documentation links, I think they're actually the
>> same behavior and neither is about an accumulated time.
>
> You seem to be right.
>
>>> Because Timestamp[Tz] is microsecond resolution, and the timeout
>>> GUCs are in milliseconds. Adding up time differences (and rounding
>>> or truncating them to millisecond in the process) would make the
>>> per-statement lock timeout lose precision...
>> Removing the accumulation-based option would fix that.. :D
>
> To call out the wrath of Tom? No, thanks. :-D
> IIRC, he was the one who didn't like the per-lock behaviour
> the first time he looked at this patch in an earlier form
> back in 2010/2011 and he proposed this use case instead.
> If not him, then someone else. I got the idea from this list...

Another question just popped up. Now, that    bool enable_multiple_timeouts(List *timeouts);
exists, do we really need the singular versions?

Since the "timeout after N msec" have the per-lock and per-stmt
versions, enable_timeout_after() gained a new argument to
distinguish between the two cases and every occurrences of
this function happen to just use "0" here. The only usage of the
per-stmt variant is used with enable_multiple_timeouts().

Wouldn't it be better to have a single    bool enable_timeouts(List *timeouts);
instead?


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