Date: Fri, 29 Jan 2021 12:40:18 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@lists.postgresql.org,
	Jaime Casanova <jaime.casanova@2ndQuadrant.com>
Subject: Re: LogwrtResult contended spinlock
Message-ID: <20210129154018.GA7723@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CE5FB7B4-BD5B-40C1-A915-0AE770812C3D@anarazel.de>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

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:

typedef struct XLogwrtAtomic
{
	pg_atomic_uint64	Write;		/* last byte + 1 of write position */
	pg_atomic_uint64	Flush;		/* last byte + 1 of flush position */
} XLogwrtAtomic;

this one, with atomics, would be used for the XLogCtl struct members
LogwrtRqst and LogwrtResult, and are always accessed using atomic ops.
On the other hand we would have

typedef struct XLogwrt
{
	XLogRecPtr	Write;		/* last byte + 1 of write position */
	XLogRecPtr	Flush;		/* last byte + 1 of flush position */
} XLogwrt;

to be used for the local-memory-only LogwrtResult, using normal
assignment.

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.

Thoughts?

-- 
Álvaro Herrera       Valdivia, Chile