Re: BUG #8470: 9.3 locking/subtransaction performance regression

Bruce Momjian wrote:
> On Mon, Jan  5, 2015 at 02:23:18PM -0300, Alvaro Herrera wrote:
> > I just got around to merging this patch to 9.5 sources.  I'm glad I
> > did it because in the course of doing so I noticed a bug in a recent
> > patch, which led to commit d5e3d1e969d2f65009f718d3100d6565f47f9112
> > (back-patched to 9.3).
> >
> > I'm now more confident in this code and will probably push this a few
> > days, but only to 9.5 at least for now.  It probably won't apply cleanly
> > to 9.3 due to other changes in the area, such as 05315498012530d44cd89a2
> > and df630b0dd5ea2de52972d456f5978a012436115e and others.
>
> Where are we on this?

That's indeed the question.  I gave this a look yesterday and came up
with two patches that "fix" the issue.  (I had to fix one additional bug
in the formulation of the complex patch that I posted in January).  I
leave the details of the bug as exercise for the interested readers
(hint: run the isolation tests).  First some timings.  Ran the script
against unpatched master and each of the patches five times.  Results:

unpatched:
real    0m8.192s
real    0m8.230s
real    0m8.233s
real    0m8.187s
real    0m8.212s

simple patch:
real    0m0.741s
real    0m0.728s
real    0m0.729s
real    0m0.738s
real    0m0.731s

complex patch:
real    0m0.732s
real    0m0.723s
real    0m0.730s
real    0m0.725s
real    0m0.726s

In 9.2 the time is about 0.150s, so the regression is not completely
resolved, but it's a huge improvement.

The main catch of the "simple" formulation of the patch is that we do
the new GetMultiXactIdMembers call with the buffer lock held, which is a
horrible idea from a concurrency point of view; it will make many cases
where the optimization doesn't apply a lot slower.  I think with some
extra contortions we could fix that problem, but it's already quite ugly
that we have a duplicate check for the are-we-already-a-multixact-locker
so I reject the idea that this seemingly simple patch is any good.  I
much prefer the complex formulation, which is what I had to start with,
and makes thing a bit less unclear(*).

There was some traction to the idea of backpatching this, but I'm no
longer on board with that.  If somebody wants to, I would like some
commitment to a huge testing effort.


(*) In a scale 1 to 10 with 10 being most unclear, the original code is
about 12-unclear, and with the patch is 11-unclear.  So it's an
improvement anyhow.

PS: Apologies for unified diff.  This is one case where filterdiff
dropped some hunks from the patch produced by git diff.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services