Re: LWLock Queue Jumping - Mailing list pgsql-hackers
| From | Jeff Janes |
|---|---|
| Subject | Re: LWLock Queue Jumping |
| Date | |
| Msg-id | f67928030908291157w78071ae0gcb599aa7a5c26f4b@mail.gmail.com Whole thread Raw |
| In response to | Re: LWLock Queue Jumping (Simon Riggs <simon@2ndQuadrant.com>) |
| List | pgsql-hackers |
On Sat, Aug 29, 2009 at 4:02 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
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'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.
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
That's the way I would implement it also, but would call it
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.
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.
Please send the tested patch, if this isn't it. What tests were made?
> I've tested it fairly thoroughly,
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.
Apologies if you'd already suggested that recently. I read a few of your
> in the context of using it in AdvanceXLInsertBuffer for acquiring the
> WALWriteLock.
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
pgsql-hackers by date: