Re: LWLock Queue Jumping

On Sat, Aug 29, 2009 at 4:02 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

>
> On Fri, 2009-08-28 at 14:44 -0700, Jeff Janes wrote:
>
> > I'd previously implemented this just by copying and pasting and making
> > some changes, perhaps not the most desirable way but I thought adding
> > another parameter to all existing invocations would be a bit
> > excessive.
>
> That's the way I would implement it also, but would call it
> LWLockAcquireWithPriority() so that it's purpose is clear, rather than
> refer to its implementation, which may change.

Yes, good idea. But thinking about it as a patch to be applied, rather than
a proof of concept, I think the best solution would be to add a third
argument (boolean Priority) to LWLockAquire, and hunt down all existing
invocations and change them to include false as the extra argument. Copying
160 lines of code to change 4 of them in the copy is temporarily easier, but
not a good solution for the long term.

> > I've tested it fairly thoroughly,
>
> Please send the tested patch, if this isn't it. What tests were made?

I'd have a hard time coming up with the full origianl patch, as my changes
for files other than lwlock.c were blown away by parallel efforts and an
rsync to the repo. The above was just an exploratory tool, not proposed as
an actual patch to be applied to HEAD. If we want to add a parameter to the
existing LWLockAcquire, I'll work on coming up with a tested patch for
that. My testing was to run the normal regression test (which often failed
to detect my early buggy implementations), then load testing with pgbench
(which always (that I know of) found them when -c > 1) and a custom Perl
script I use. Since WALWriteLock is heavily used and contended under
pgbench -c 20, and lwlock is agnostic to the exact identity of the
underlying lock, I think this test was pretty thorough for the
implementation. But not of course for starvation issues, which would have
to be tested on a case by case basis when a specific Acquire invocation is
changed to be high priority.

If you have ideas for other tests to do, or corner cases that are likely to
be overlooked by my tests, I'll try to work tests for them in too.

>
>
> > in the context of using it in AdvanceXLInsertBuffer for acquiring the
> > WALWriteLock.
>
> Apologies if you'd already suggested that recently. I read a few of your
> posts but not all of them.

> I don't think WALWriteLock from AdvanceXLInsertBuffer is an important
> area, but I don't see any harm from it either.

I had not mentioned it before. The change helped by ~50% or so when
wal_buffers was undersized (kept at the default setting) but did not help
significantly when wal_buffers was generously sized. I didn't think we
would be interested in introducing a new locking procedure just to optimize
performance for a poorly configured server. But if we are to introduce this
method for other reasons, I think it should be used for
AdvanceXLInsertBuffer as well.

Cheers,

Jeff