Feedback-ID: ia2694551:Fastmail
Date: Mon, 1 Jul 2024 19:04:36 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Alexander Lakhin <exclusion@gmail.com>,
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
 Andres Freund <andres@anarazel.de>,
	Jeff Davis <pgsql@j-davis.com>, pgsql-hackers@lists.postgresql.org,
	Jaime Casanova <jcasanov@systemguards.com.ec>
Subject: Re: LogwrtResult contended spinlock
Message-ID: <202407011704.ujh2z5vtmx5d@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="utchk7geklzvbrhf"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <2741228.1719845958@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/202407011704.ujh2z5vtmx5d%40alvherre.pgsql>
Precedence: bulk


--utchk7geklzvbrhf
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2024-Jul-01, Tom Lane wrote:

> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > Maybe we can do something like this,
> 
> > +#if MAXIMUM_ALIGNOF >= 8
> >  	uint64		currval;
> 
> This should probably be testing the alignment of int64 specifically,
> rather than assuming that MAXIMUM_ALIGNOF applies to it.  At least
> historically, there have been platforms where MAXIMUM_ALIGNOF is
> determined by float quantities and integer quantities are different.

OK, so it's
#if ALIGNOF_LONG_LONG_INT >= 8

Alexander, can you please confirm whether this works for you?

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"La virtud es el justo medio entre dos defectos" (Aristóteles)

--utchk7geklzvbrhf
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="v4-0001-Fix-alignment-in-pg_atomic_monotonic_advance_u64.patch"

From 65e9e813859fec436805b1ada7be9339f8ecd63e Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Mon, 1 Jul 2024 16:44:22 +0200
Subject: [PATCH v4] Fix alignment in pg_atomic_monotonic_advance_u64

Unsurprisingly, it's possible for Windows x86 to have 8-byte atomics but
4-byte-aligned long long ints.  This breaks an assertion in recently
introduced pg_atomic_monotonic_advance_u64 with commit f3ff7bf83bce.
Try to fix that blindly by adding pg_attribute_aligned(8).  Unfortunately
not all compilers support that, so it needs to be protected by #ifdef;
fortunately not all compilers _need_ it.  Hopefully the combinations we
now support cover all interesting cases.

Reported-by: Alexander Lakhin <exclusion@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/36796438-a718-cf9b-2071-b2c1b947c1b5@gmail.com
---
 src/include/port/atomics.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/src/include/port/atomics.h b/src/include/port/atomics.h
index 78987f3154..eeb47dee30 100644
--- a/src/include/port/atomics.h
+++ b/src/include/port/atomics.h
@@ -580,7 +580,21 @@ pg_atomic_sub_fetch_u64(volatile pg_atomic_uint64 *ptr, int64 sub_)
 static inline uint64
 pg_atomic_monotonic_advance_u64(volatile pg_atomic_uint64 *ptr, uint64 target_)
 {
+	/*
+	 * When using actual (not simulated) atomics, the target variable for
+	 * pg_atomic_compare_exchange_u64 must have suitable alignment.  This
+	 * occurs naturally on platforms where long long int is 8-byte aligned. On
+	 * others, we must explicitly coerce the compiler into applying suitable
+	 * alignment, or abort the compile if we cannot.  When using simulated
+	 * atomics, the alignment doesn't matter.
+	 */
+#if ALIGNOF_LONG_LONG_INT >= 8 || defined(PG_HAVE_ATOMIC_U64_SIMULATION)
 	uint64		currval;
+#elif defined(pg_attribute_aligned)
+	pg_attribute_aligned(8) uint64 currval;
+#else
+#error "Must have pg_attribute_aligned or simulated atomics on this platform"
+#endif
 
 #ifndef PG_HAVE_ATOMIC_U64_SIMULATION
 	AssertPointerAlignment(ptr, 8);
-- 
2.39.2


--utchk7geklzvbrhf--