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