Subject: Re: Hot Standby on git
From: Simon Riggs <simon@2ndQuadrant.com>
To: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
In-Reply-To: <4ACA6EDE.4020407@enterprisedb.com>
References: <1253958559.4449.833.camel@ebony.2ndQuadrant>
	<4ABF4522.6060601@enterprisedb.com>
	<1254743455.4691.153.camel@ebony.2ndQuadrant>
	<4ACA6EDE.4020407@enterprisedb.com>
Content-Type: text/plain
Date: Tue, 06 Oct 2009 08:19:32 +0100
Message-Id: <1254813572.26302.38.camel@ebony.2ndQuadrant>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit


On Tue, 2009-10-06 at 01:10 +0300, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > We discussed briefly your change 
> > 0011-Replace-per-proc-counters-of-loggable-locks-with-per.patch.
> > 
> > I don't see how that helps at all. The objective of lock counters was to
> > know if we can skip acquiring an LWlock on all lock partitions. This
> > change keeps the lock counters yet acquires the locks we were trying to
> > avoid. This change needs some justification since it is not a bug fix.
> 
> Well, the original code was buggy. 

One bug was found, yes, but the submitted changes are not just a bug
fix, they alter the whole basic meaning of that code.

> But more to the point, it's a lot
> simpler this way, I don't see any reason why the counters should be
> per-process, meaning that they need to be exposed in the pgproc structs
> or procarray.c.

There was a very clear reason for doing it that way. By putting the
counters on the PGPROC structs it allows us to check the counts while we
are performing the main sweep of the procarray *and* if the count is
zero it allows us to *completely avoid* taking the locks. By making the
lock counters private to each backend the counters themselves do not
need to be protected by a lock when updated, and the fetch can be done
atomically, as we do with xid.

With respect, I have explained this on list at least twice previously,
as well as in code comments.

> The point is to avoid the seqscan of the lock hash table. I presumed
> that's the expensive part in GetRunningTransactionLocks().

> Tom Lane wrote:
> > [ scratches head ... ]  Why is hot standby messing with this sort of
> > thing at all?  It sounds like a performance optimization that should
> > be considered separately, and *later*.
> 
> Yeah, I too considered just ripping it out. Simon is worried that
> locking all the lock partitions and scanning the locks table can take a
> long time. We do that in the master, while holding both ProcArrayLock
> and XidGenLock in exclusive mode (hmm, why is shared not enough?), so
> there is some grounds for worry. OTOH, it's only done once per checkpoint.

I could live with ripping it out, but what we have now doesn't make
sense, to me.

-- 
 Simon Riggs           www.2ndQuadrant.com