On Mon, Jun 4, 2012 at 9:20 AM, Ants Aasma <ants@cybertec.at> wrote:
> On Mon, Jun 4, 2012 at 5:12 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> Note sure about the rest of this patch, but this part is definitely bogus:
>>
>> +#if !defined(pg_atomic_fetch_and_set)
>> +#define pg_atomic_fetch_and_set(dst, src, value) \
>> + do { S_LOCK(&dummy_spinlock); \
>> + dst = src; \
>> + src = value; \
>> + S_UNLOCK(&dummy_spinlock); } while (0)
>> +#endif
>>
>> Locking a dummy backend-local spinlock doesn't provide atomicity
>> across multiple processes.
>
> Right, of course. I don't know why I assumed that dummy_spinlock would
> be global. In any case, this is very WIP and doesn't even aspire to be
> portable yet. The main point was to see if there's any significant
> performance to be gained by this method.
yeah -- those are fallback routines in case the compiler primitives
don't exist. I think I understand what Ants is doing here: he's
reducing the coverage of the free list lock to only cover actually
popping a buffer off the free list; it no longer covers the clock
sweep. That's a massive win if it works. In order to get away with
that he had to decompose all manipulations from the clock sweep to the
Strategy to thread safe atomic operations.
What happens (in the very unlikely, but possible case?) if another
backend races to the buffer you've pointed at with 'victim'? It looks
like multiple backends share the clock sweep now, but don't you need
to need an extra test to ensure it's still a candidate victim buffer?
merlin