Re: Strange Windows problem, lock_timeout test request

2013-02-23 02:55 keltezéssel, Stephen Frost írta:
> Zoltán,
>
> * Zoltán Böszörményi (zb(at)cybertec(dot)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/