2013-02-23 02:55 keltezéssel, Stephen Frost írta:
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..?
Yes, is was until you came up with your review... ;-)
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.
Since moving to GIT, this expectation is obsolete. All PG hackers
became comfortable with the unified diff format, see references
from the list. A lot of hackers post "git diff" patches which cannot
produce context diff, either.
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.
That's not strictly true but it's not widely used either:
$ find . -type f | xargs grep weight | grep heavy
./monitoring.sgml: <entry>Probe that fires when a request for a heavyweight lock (lmgr
lock)
./monitoring.sgml: <entry>Probe that fires when a request for a heavyweight lock (lmgr
lock)
- 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 ...
OK.
- 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?
OK, suggest names for the 2 GUCS, please.
- 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
Right.
- Not a big fan of this:
+ * See notes in PGSemaphoreLock.
Why? Copy&paste the *long* comment (a different one in each *_sema.c)
or omitting the comments is better? Suggest a better comment, and
it will be included.
- 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.
A linked list might be better, actually I like it.
- 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..?
Well, my code is the only user for this macro but it's cleaner
than explicitly doing
#ifdef HAVE_INT64_TIMESTAMP
fin_time = timeout_start + delay_ms * (int64)1000;
#else
fin_time = timeout_start + delay_ms / 1000000.0;
#endif
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
--
----------------------------------
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
