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

Zoltán,

* Boszormenyi Zoltan (zb@cybertec.at) wrote:
> 2013-02-23 02:55 keltezéssel, Stephen Frost írta:
> >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.

No, it isn't.  Patches posted to the list should be in context diff
format (and uncompressed unless it's too large) for easier reading.
That avoids having to download it, apply it to a git tree and then
create the diff ourselves.  Indeed, the move to git had impact on this
at all.

> 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.

It's quite possible to create a context diff from git- indeed, it's
documented on the http://wiki.postgresql.org/wiki/Working_with_Git
portion of the wiki, specifically to help people understand how to take
the changes they've made in their git forks and create context diffs to
post to the mailing lists.  The preference for context diffs is also
still documented at http://wiki.postgresql.org/wiki/Submitting_a_Patch.

And, to be honest, I'm not sure how familiar you are with the history of
PG in this area, but I've been through all of it from the pre-git CVS
days, through the migration to git, to the exclusive use of git.  I feel
quite confident that the preference for context diffs hasn't changed.

> 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)

Interesting.  That didn't show up in the searches that I was doing
through the web interface, though it does now.  If we're going to use
that term, we should define it in the lock documentation.  If not, then
we should avoid it everywhere.  I'm fine with either.

> >- 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.

lock_timeout_wait and lock_timeout_stmt_wait ?  Though I've been really
wondering to myself as to if we need both of these options as well as
statement_timeout.  Perhaps you can explain the use case for each
option and how they're distinct from each other?  Indeed, the use-case
that I'm envisioning is focused around "don't wait more than 'X' for the
relation-level locks" which would allow you to distinguish queries that
are, most likely anyway, making progress, from those which have been
caught behind a lock.  That would match your 'per-statement' lock
timeout concept, iiuc, though I think it might be more simply
implemented.

> >- 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.

How about, for starters, there's more than one PGSemaphoreLock?  Second,
as you'll note in posix_sema.c, it's useful to say more than just "look
over there".  I'm not suggesting a mass copy/paste, but more than 4
words would be valuable.

> >- Do we really need TimestampTzPlusMicroseconds..?
>
> Well, my code is the only user for this macro but it's cleaner
> than explicitly doing

To be honest, I was really wondering why TimestampTzPlusMilliseconds
isn't sufficient, not suggesting that we litter the code with #ifdef's.
Thanks,
    Stephen