Thread: sinval contention reduction
Patch to reduce the contention on SInvalLock, as discussed here: http://archives.postgresql.org/pgsql-hackers/2007-09/msg00501.php and http://archives.postgresql.org/pgsql-performance/2008-01/msg00023.php For discussion. -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com
Attachment
Simon Riggs <simon@2ndquadrant.com> writes: > Patch to reduce the contention on SInvalLock, as discussed here: > http://archives.postgresql.org/pgsql-hackers/2007-09/msg00501.php > and > http://archives.postgresql.org/pgsql-performance/2008-01/msg00023.php > For discussion. This seems large, complex, and untested (I note in particular a guaranteed-to-fail Assert). I'm also wondering if it will help much, since unless the system is already in trouble, the normal case will be that all backends have absorbed all messages and so they'll all see stateP->nextMsgNum == segP->minMsgNum when they first respond to a signal. Do you have any evidence for performance improvement? regards, tom lane
On Fri, 2008-01-25 at 19:02 -0500, Tom Lane wrote: > Simon Riggs <simon@2ndquadrant.com> writes: > > Patch to reduce the contention on SInvalLock, as discussed here: > > http://archives.postgresql.org/pgsql-hackers/2007-09/msg00501.php > > and > > http://archives.postgresql.org/pgsql-performance/2008-01/msg00023.php > > > For discussion. > > This seems large, complex, and untested (I note in particular a > guaranteed-to-fail Assert). Yes, its for discussion. How would you describe such a patch in the future? I want to be able to differentiate patch status. There's no point polishing it before the discussion. > I'm also wondering if it will help much, > since unless the system is already in trouble, the normal case will be > that all backends have absorbed all messages and so they'll all see > stateP->nextMsgNum == segP->minMsgNum when they first respond to a > signal. Agreed, that's why its not the only condition. In the patch, the message queue is normally cleaned when an insert gets past 50% of queue length, then again at 56%, 62% and 68%. If that doesn't help then we hit the PM signal at 70% as before. We don't attempt to clean the queue after every round of messages by each backend, which causes huge contention as we know. We only clean the queue if a long run of messages is read by the oldest message reader, so when stateP->nextMsgNum == segP->minMsgNum && number of messages read > 25% of queue. So that is only performed by a backend waking up to find it is behind, such as would happen if a PM signal had been issued. > Do you have any evidence for performance improvement? ISTM that it would only be worth testing when we had a rough agreement that we had found a reasonable approach. -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com
Simon Riggs <simon@2ndquadrant.com> writes:
> On Fri, 2008-01-25 at 19:02 -0500, Tom Lane wrote:
>> This seems large, complex, and untested (I note in particular a
>> guaranteed-to-fail Assert).
> Yes, its for discussion. How would you describe such a patch in the
> future? I want to be able to differentiate patch status.
"Completely untested" might be an appropriate description ...
> We only clean the queue if a long run of messages is read by the oldest
> message reader, so when stateP->nextMsgNum == segP->minMsgNum && number
> of messages read > 25% of queue. So that is only performed by a backend
> waking up to find it is behind, such as would happen if a PM signal had
> been issued.
You still haven't explained why that won't happen in parallel within
many backends at once. I really think we need to fix things so that
catchup interrupts occur serially instead of all at once.
>> Do you have any evidence for performance improvement?
> ISTM that it would only be worth testing when we had a rough agreement
> that we had found a reasonable approach.
Well, what I'd like to do for 8.4 is a considerably more invasive
rethink of the signaling logic. I'm not opposed in principle to some
simpler stopgap fix for 8.3, but at this late stage of the cycle there
would have to be a pretty compelling performance-improvement case for
it. So I'm not sure of the point of posting a patch that's not even
ready for performance testing.
regards, tom lane
On Sat, 2008-01-26 at 14:27 -0500, Tom Lane wrote: > Simon Riggs <simon@2ndquadrant.com> writes: > > On Fri, 2008-01-25 at 19:02 -0500, Tom Lane wrote: > >> This seems large, complex, and untested (I note in particular a > >> guaranteed-to-fail Assert). > > > Yes, its for discussion. How would you describe such a patch in the > > future? I want to be able to differentiate patch status. > > "Completely untested" might be an appropriate description ... That wouldn't be true, because it passes make check. If it were true, I'd have said it. Your responses are inappropriate to a patch clearly marked "for discussion", especially when you privately suggested this topic for me to look at and you also know exactly which system I was going to run a performance test on once I had the patch agreed. -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com