Zoltán, * Zoltán Böszörményi (z...@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' term anywhere 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 beyond that, it
strikes me that wanting to set both a 'per-lock timeout' and a
'overall waiting-for-locks timeout' at the same 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
(depending on <varname>lock_timeout_option</varname>) needs to wait
more than the specified value in milliseconds, the command reports
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 use NOWAIT 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 passing around
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 implementation and 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- the code tells you that.
As such, comments like this really aren't great:
/* Assert request is sane */
/* Now re-enable the timer, 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
signature.asc
Description: Digital signature
