testing HS/SR - 1 vs 2 performance
in [PgSql]
|
Prev: pg_ctl stop -m immediate on the primary server inflatessequences
Next: [HACKERS] non-reproducible failure of random test on HEAD
From: Simon Riggs on 21 Apr 2010 09:31 On Wed, 2010-04-21 at 15:20 +0300, Heikki Linnakangas wrote: > Simon Riggs wrote: > > On Sun, 2010-04-18 at 08:24 +0100, Simon Riggs wrote: > >> On Sat, 2010-04-17 at 18:52 -0400, Tom Lane wrote: > >>> Simon Riggs <simon(a)2ndQuadrant.com> writes: > >>>> What I'm not clear on is why you've used a spinlock everywhere when only > >>>> weak-memory thang CPUs are a problem. Why not have a weak-memory-protect > >>>> macro that does does nada when the hardware already protects us? (i.e. a > >>>> spinlock only for the hardware that needs it). > >>> Well, we could certainly consider that, if we had enough places where > >>> there was a demonstrable benefit from it. I couldn't measure any real > >>> slowdown from adding a spinlock in that sinval code, so I didn't propose > >>> doing so at the time --- and I'm pretty dubious that this code is > >>> sufficiently performance-critical to justify the work, either. > >> OK, I'll put a spinlock around access to the head of the array. > > > > v2 patch attached > > The locking seems overly complex to me. > Looking at > KnownAssignedXidsAdd(), isn't it possible for two processes to call it > concurrently in exclusive_lock==false mode and get the same 'head' > value, and step on each others toes? I guess KnownAssignedXidsAdd() is > only called by the startup process, but it seems like an accident > waiting to happen. Not at all. That assumption is also used elsewhere, so it is safe to use here. > Spinlocks are fast, if you have to add an if-branch to decide whether to > acquire it, I suspect you've lost any performance gain to be had > already. I think you're misreading the code. One caller already holds exclusive lock, one does not. The if test is to determine whether to acquire the lock or not. > Let's keep it simple. And acquiring ProcArrayLock in exclusive > mode while adding entries to the array seems OK to me as well. It only > needs to be held very briefly, and getting this correct and keeping it > simple is much more important at this point than squeezing out every > possible CPU cycle from the system. I don't understand what you're saying: you say I'm wasting a few cycles on an if test and should change that, but at the same time you say I shouldn't worry about a few cycles. > Just require that the caller holds ProcArrayLock in exclusive mode when > calling an operation that modifies the array, and in shared mode when > reading it. The point of the code you're discussing is to remove the exclusive lock requests, not to save a few cycles. Spinlocks are fast, as you say. Exclusive lock requests block under a heavy load of shared lock holders, we know that already. It is worth removing the contention that can occur by minimising the number of exclusive locks required. This patch shows how and I don't see any reason from what you have said to avoid committing it. I'm willing to hear some sound reasons, if any exist. -- Simon Riggs www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Simon Riggs on 21 Apr 2010 09:37 On Wed, 2010-04-21 at 15:27 +0300, Heikki Linnakangas wrote: > Given the discussion about the cyclic nature of XIDs, it would be good > to add an assertion that when a new XID is added to the array, it is > > a) larger than the biggest value already in the array > (TransactionIdFollows(new, head)), and > b) not too far from the smallest value in the array to confuse binary > search (TransactionIdFollows(new, tail)). We discussed this in November. You convinced me it isn't possible for older xids to stay in the standby because anti-wraparound vacuums would conflict and kick them out. The primary can't run with wrapped xids and neither can the standby. I think that is correct. Adding an assertion isn't going to do much because it's unlikely anybody is going to be running for 2^31 transactions with asserts enabled. Worrying about things like this seems strange when real and negative behaviours are right in our faces elsewhere. Performance and scalability are real world concerns. -- Simon Riggs www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Robert Haas on 21 Apr 2010 09:51 On Wed, Apr 21, 2010 at 9:37 AM, Simon Riggs <simon(a)2ndquadrant.com> wrote: > On Wed, 2010-04-21 at 15:27 +0300, Heikki Linnakangas wrote: > >> Given the discussion about the cyclic nature of XIDs, it would be good >> to add an assertion that when a new XID is added to the array, it is >> >> a) larger than the biggest value already in the array >> (TransactionIdFollows(new, head)), and >> b) not too far from the smallest value in the array to confuse binary >> search (TransactionIdFollows(new, tail)). > > We discussed this in November. You convinced me it isn't possible for > older xids to stay in the standby because anti-wraparound vacuums would > conflict and kick them out. The primary can't run with wrapped xids and > neither can the standby. I think that is correct. > > Adding an assertion isn't going to do much because it's unlikely anybody > is going to be running for 2^31 transactions with asserts enabled. > > Worrying about things like this seems strange when real and negative > behaviours are right in our faces elsewhere. Performance and scalability > are real world concerns. I think the assert is a good idea. If there's no real problem here, the assert won't trip. It's just a safety precaution. ....Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Robert Haas on 21 Apr 2010 10:11 On Wed, Apr 21, 2010 at 8:20 AM, Heikki Linnakangas <heikki.linnakangas(a)enterprisedb.com> wrote: > The locking seems overly complex to me. I tend to agree. ! /* ! * Callers must hold either ProcArrayLock in Exclusive mode or ! * ProcArrayLock in Shared mode *and* known_assigned_xids_lck ! * to update these values. ! */ I'm not convinced that this is either (a) correct or (b) performant. ....Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Simon Riggs on 21 Apr 2010 10:12
On Wed, 2010-04-21 at 09:51 -0400, Robert Haas wrote: > > > > Adding an assertion isn't going to do much because it's unlikely anybody > > is going to be running for 2^31 transactions with asserts enabled. > > > I think the assert is a good idea. If there's no real problem here, > the assert won't trip. It's just a safety precaution. If you believe that, then I think you should add this to all the other places in the current server where that assumption is made without assertion being added. As a safety precaution. -- Simon Riggs www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |