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

Zoltán,

* Zoltán Böszörményi (zb@cybertec.at) wrote:
> The patch now passed "make check" in both cases.

Is v29 the latest version of this patch..?

Looking through the patch, I've noticed a couple of things:

First off, it's not in context diff format, which is the PG standard for
patch submission.  Next, the documentation has a few issues:

- "Heavy-weight" should really be defined in terms of specific lock types or modes.  We don't use the 'heavyweight'
termanywhere else in the documentation that I've found. 

- I'd reword this:
 Abort any statement that tries to acquire a heavy-weight lock on rows, pages, tables, indices or other objects and the
lock(s)has to wait more than the specified number of milliseconds. 
 as:
 Abort any statement which waits longer than the specified number of milliseconds while attempting to acquire a lock on
...

- I don't particularly like "lock_timeout_option", for a couple of reasons.  First is simply the name is terrible, but
beyondthat, it strikes me that wanting to set both a 'per-lock timeout' and a 'overall waiting-for-locks timeout' at
thesame time would be a reasonable use-case.  If we're going to have 2 GUCs and we're going to support each of those
options,why not just let the user specify values for each? 

- This is a bit disingenuous:
 If <literal>NOWAIT</> option is not specified and <varname>lock_timeout</varname> is set and the lock or statement
(dependingon <varname>lock_timeout_option</varname>) needs to wait more than the specified value in milliseconds, the
commandreports an error after timing out, rather than waiting indefinitely. 
 The SELECT would simply continue to wait until the lock is available. That's a bit more specific than 'indefinitely'.
Also,we might add a sentence about statement_timeout as well, if we're going to document what can happen if you don't
useNOWAIT with your SELECT-FOR-UPDATE. Should we add documentation to the other commands that wait for locks? 

- Looks like this was ended mid-thought...:

+ * Lock a semaphore (decrement count), blocking if count would be < 0
+ * until a timeout triggers. Returns true if

- Not a big fan of this:

+    * See notes in PGSemaphoreLock.

- I'm not thrilled with the new API for defining the timeouts. Particularly, I believe the more common convention for
passingaround arrays of structures is to have an empty array at the end, which avoids having to remember to update the
#of entries every time it gets changed.  Of course, another option would be to use our existing linked list
implementationand its helper macros such as our foreach() construct. 

- As I've mentioned in other places/times, comments should be about why we're doing something, not what we're doing-
thecode tells you that. As such, comments like this really aren't great: /* Assert request is sane */ /* Now re-enable
thetimer, if necessary. */ 

- Do we really need TimestampTzPlusMicroseconds..?

In general, I like this feature and a number of things above are pretty
small issues.  The main questions, imv, are if we really need both
'options', and, if so, how they should work, and the API for defining
timers.
Thanks,
    Stephen