Feedback-ID: ia2694551:Fastmail
Date: Tue, 2 Jul 2024 13:55:49 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Andres Freund <andres@anarazel.de>
Cc: Alexander Lakhin <exclusion@gmail.com>, 
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>, Jeff Davis <pgsql@j-davis.com>, 
	pgsql-hackers@lists.postgresql.org, Jaime Casanova <jcasanov@systemguards.com.ec>
Subject: Re: LogwrtResult contended spinlock
Message-ID: <202407021155.s3f4knnte6gv@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <202407011912.diw4rf4rgd76@alvherre.pgsql>
Archived-At: <https://www.postgresql.org/message-id/202407021155.s3f4knnte6gv%40alvherre.pgsql>
Precedence: bulk

On 2024-Jul-01, Alvaro Herrera wrote:

> On 2024-Jul-01, Andres Freund wrote:
> 
> > On 2024-07-01 11:10:24 +0200, Alvaro Herrera wrote:
> > > In the meantime I noticed that pg_attribute_aligned() is not supported
> > > in every platform/compiler, so for safety sake I think it's better to go
> > > with what we do for PGAlignedBlock: use a union with a double member.
> > > That should be 8-byte aligned on x86 as well, unless I misunderstand.
> > 
> > If a platform wants to support 8 byte atomics, it better provides a way to
> > make variables 8 bytes aligned. We already rely on that, actually. See use of
> > pg_attribute_aligned in e.g. src/include/port/atomics/generic-msvc.h

I noticed that all definitions have the value struct member named "value", so I
gave this a quick try, and hit another issue: the "expected" argument of
pg_atomic_compare_exchange_u64_impl() is not volatile, which
pg_atomic_uint64.value is, so we get this warning:

In file included from /pgsql/source/master/src/include/storage/lwlock.h:21,
                 from /pgsql/source/master/src/include/storage/lock.h:23,
                 from /pgsql/source/master/src/include/access/twophase.h:20,
                 from /pgsql/source/master/src/backend/access/transam/xlog.c:57:
/pgsql/source/master/src/include/port/atomics.h: In function ‘pg_atomic_monotonic_advance_u64’:
/pgsql/source/master/src/include/port/atomics.h:600:62: warning: passing argument 2 of ‘pg_atomic_compare_exchange_u64_impl’ discards ‘volatile’ qualifier from pointer target type [-Wdiscarded-qualifiers]
  600 |                 if (pg_atomic_compare_exchange_u64_impl(ptr, &currval.value, target_))
      |                                                              ^~~~~~~~~~~~~~
In file included from /pgsql/source/master/src/include/port/atomics.h:69:
/pgsql/source/master/src/include/port/atomics/arch-x86.h:206:81: note: expected ‘uint64 *’ {aka ‘long unsigned int *’} but argument is of type ‘volatile uint64 *’ {aka ‘volatile long unsigned int *’}
  206 |                                                                         uint64 *expected, uint64 newval)
      |                                                                         ~~~~~~~~^~~~~~~~

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Most hackers will be perfectly comfortable conceptualizing users as entropy
 sources, so let's move on."                               (Nathaniel Smith)
      https://mail.gnu.org/archive/html/monotone-devel/2007-01/msg00080.html