Re: LogwrtResult contended spinlock

Hi,


On 2021-01-29 12:40:18 -0300, Alvaro Herrera wrote:
> On 2020-Aug-31, Andres Freund wrote:
> 
> > Wouldn't the better fix here be to allow reading of individual members without a lock? E.g. by wrapping each in a 64bit atomic.
> 
> So I've been playing with this and I'm annoyed about having two
> datatypes to represent Write/Flush positions:
> 
> typedef struct XLogwrtRqst
> {
> 	XLogRecPtr	Write;			/* last byte + 1 to write out */
> 	XLogRecPtr	Flush;			/* last byte + 1 to flush */
> } XLogwrtRqst;
> 
> typedef struct XLogwrtResult
> {
> 	XLogRecPtr	Write;			/* last byte + 1 written out */
> 	XLogRecPtr	Flush;			/* last byte + 1 flushed */
> } XLogwrtResult;
> 
> Don't they look, um, quite similar?  I am strongly tempted to remove
> that distinction, since it seems quite pointless, and introduce a
> different one:

Their spelling drives me nuts. Like, one is *Rqst, the other *Result?
Comeon.


> Now, I do wonder if there's a point in keeping LogwrtResult as a local
> variable at all; maybe since the ones in shared memory are going to use
> unlocked access, we don't need it anymore?  I'd prefer to defer that
> decision to after this patch is done, since ISTM that it'd merit more
> careful benchmarking.

I think doing that might be a bit harmful - we update LogwrtResult
fairly granularly in XLogWrite(). Doing that in those small steps in
shared memory will increase the likelihood of cache misses in other
backends.

Greetings,

Andres Freund