Re: Lockless StrategyGetBuffer() clock sweep

On Mon, Dec 8, 2014 at 2:51 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> 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.

Maybe you could store the pgprocno instead of the PROC *.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company