Re: Lockless StrategyGetBuffer() clock sweep

On 2014-10-30 07:55:08 -0400, Robert Haas wrote:
> On Wed, Oct 29, 2014 at 3:09 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >> But if it is, then how about
> >> adding a flag that is 4 bytes wide or less alongside bgwriterLatch,
> >> and just checking the flag instead of checking bgwriterLatch itself?
> >
> > Yea, that'd be nicer. I didn't do it because it made the patch slightly
> > more invasive... The complexity really is only needed because we're not
> > guaranteed that 64bit reads are atomic. Although we actually can be
> > sure, because there's no platform with nonatomic intptr_t reads - so
> > 64bit platforms actually *do* have atomic 64bit reads/writes.
> >
> > So if we just have a integer 'setBgwriterLatch' somewhere we're good. We
> > don't even need to take a lock to set it. Afaics the worst that can
> > happen is that several processes set the latch...
>
> OK, that design is fine with me.

There's a slight problem with this, namely restarts of bgwriter. If it
crashes the reference to the relevant latch will currently be reset via
StrategyNotifyBgWriter(). In reality that's not a problem because
sizeof(void*) writes are always atomic, but currently we don't assume
that. We'd sometimes write to a old latch, but that's harmless because
they're never deallocated.

So I can see several solutions right now:
1) Redefine our requirements so that aligned sizeof(void*) writes are
always atomic. That doesn't affect any currently supported platform
and won't ever affect any future platform either. E.g. linux has had
that requirement for a long time.
2) Use a explicitly defined latch for the bgworker instead of using the
PGPROC->procLatch. That way it never has to be reset and all
SetLatch()s will eventually go to the right process.
3) Continue requiring the spinlock when setting the latch.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services