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

2013-02-23 02:55 keltezéssel, Stephen Frost írta:
> 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..?

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/