Subject: Re: LogwrtResult contended spinlock

From: Alexander Lakhin <[email-masked]>
Date: Sat, 29 Jun 2024 15:00:00 +0300

Hi Alvaro,

Thank you for looking at this!

29.06.2024 13:23, Alvaro Herrera wrote:
>> TRAP: failed Assert("TYPEALIGN(8, (uintptr_t)(&currval)) ==
>> (uintptr_t)(&currval)"), File: "...\src\include\port/atomics.h", Line: 597,
>> PID: 7556
>> child process was terminated by exception 0xC0000409
> Oh.  This is the new assertion in pg_atomic_monotonic_advance_u64() and
> therefore the only possible culprit could be logInsertResult in
> XLogCtlData.  Does it work if you do this?
>
> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
> index 8dcdf5a764..e581488d57 100644
> --- a/src/backend/access/transam/xlog.c
> +++ b/src/backend/access/transam/xlog.c
> @@ -468,7 +468,7 @@ typedef struct XLogCtlData
>   	XLogRecPtr	lastSegSwitchLSN;
>   
>   	/* These are accessed using atomics -- info_lck not needed */
> -	pg_atomic_uint64 logInsertResult;	/* last byte + 1 inserted to buffers */
> +	pg_atomic_uint64 logInsertResult pg_attribute_aligned(8);	/* last byte + 1 inserted to buffers */
>   	pg_atomic_uint64 logWriteResult;	/* last byte + 1 written out */
>   	pg_atomic_uint64 logFlushResult;	/* last byte + 1 flushed */

It doesn't, but the following works for me:
  static inline uint64
  pg_atomic_monotonic_advance_u64(volatile pg_atomic_uint64 *ptr, uint64 target_)
  {
-       uint64          currval;
+       pg_attribute_aligned(8) uint64          currval;

because the failed assertion is:
#ifndef PG_HAVE_ATOMIC_U64_SIMULATION
     AssertPointerAlignment(&currval, 8);
#endif

Best regards,
Alexander

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 1 Jul 2024 11:10:24 +0200

Hello,

>     AssertPointerAlignment(&currval, 8);

Hah, thank you.

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.

BTW if things works after this fix, I suggest you get a buildfarm member
running with this configuration.  Otherwise it's quite likely that we'll
break it again.  Or we could just decide we don't care about this
particular platform ...  but AFAICS the buildfarm does have other 32-bit
animals running.

[ looks at buildfarm ]

Oh!  while looking at Adder's config, I noticed this line:
Checking for alignment of "double" : 4 
So, I do misunderstand doubles.

So my patch is not going to work.  There seems to be nothing that has
alignment 8 there, so I guess we're back to using
pg_attribute_aligned() and abort the compile if that doesn't exist.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"La gente vulgar sólo piensa en pasar el tiempo;
el que tiene talento, en aprovecharlo"
>From 9d240e90f87bf8b53bd5d92b623e33419db64717 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <[email-masked]>
Date: Mon, 1 Jul 2024 10:41:06 +0200
Subject: [PATCH v2] Fix alignment of variable in
 pg_atomic_monotonic_advance_u64

Reported-by: Alexander Lakhin <[email-masked]>
Discussion: https://postgr.es/m/[email-masked]
---
 src/include/port/atomics.h | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/src/include/port/atomics.h b/src/include/port/atomics.h
index 78987f3154..964732e660 100644
--- a/src/include/port/atomics.h
+++ b/src/include/port/atomics.h
@@ -580,30 +580,38 @@ 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_)
 {
-	uint64		currval;
+	/*
+	 * On 32-bit machines, declaring a bare uint64 variable doesn't promise
+	 * the alignment we need, so coerce the compiler this way.
+	 */
+	union
+	{
+		uint64		u64;
+		double		force_align_d;
+	}			currval;
 
 #ifndef PG_HAVE_ATOMIC_U64_SIMULATION
 	AssertPointerAlignment(ptr, 8);
 #endif
 
-	currval = pg_atomic_read_u64_impl(ptr);
-	if (currval >= target_)
+	currval.u64 = pg_atomic_read_u64_impl(ptr);
+	if (currval.u64 >= target_)
 	{
 		pg_memory_barrier();
-		return currval;
+		return currval.u64;
 	}
 
 #ifndef PG_HAVE_ATOMIC_U64_SIMULATION
-	AssertPointerAlignment(&currval, 8);
+	AssertPointerAlignment(&currval.u64, 8);
 #endif
 
-	while (currval < target_)
+	while (currval.u64 < target_)
 	{
-		if (pg_atomic_compare_exchange_u64_impl(ptr, &currval, target_))
+		if (pg_atomic_compare_exchange_u64_impl(ptr, &currval.u64, target_))
 			break;
 	}
 
-	return Max(target_, currval);
+	return Max(target_, currval.u64);
 }
 
 #undef INSIDE_ATOMICS_H
-- 
2.39.2

---

From: Andres Freund <[email-masked]>
Date: Mon, 1 Jul 2024 10:17:03 -0700

Hi,

> 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

> +	 * the alignment we need, so coerce the compiler this way.

I wonder if we should just relax the alignment requirement for currval. It's
crucial that the pointer is atomically aligned (atomic ops across pages are
either forbidden or extremely slow), but it's far from obvious that it's
crucial for comparator value to be aligned.

>  	AssertPointerAlignment(ptr, 8);

What's the point of this assert, btw? This stuff is already asserted in lower
level routines, so it just seems redundant to have it here?

> +	return Max(target_, currval.u64);

What does the Max() actually achieve here? Shouldn't it be impossible to reach
this with  currval < target_?

And why does target_ end in an underscore?

Greetings,

Andres Freund

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 1 Jul 2024 21:12:25 +0200

Hello,

Thanks for your attention here.

> pg_attribute_aligned in e.g. src/include/port/atomics/generic-msvc.h

Well, pg_atomic_uint64 is a struct. Passing pointers to it is fine,
which is what non-platform-specific code does; but because the
declaration of the type is in each platform-specific file, it might not
work to use it directly in generic code.  I didn't actually try, but it
seems a bit of a layering violation.  (I didn't find any place where
the struct is used that way.)

If that works, then I think we could simply declare currval as a
pg_atomic_uint64 and it'd be prettier.

> crucial for comparator value to be aligned.

I'm pretty sure the Microsoft docs I linked to are saying it must be
aligned.

> level routines, so it just seems redundant to have it here?

There are in some of them, but not in pg_atomic_compare_exchange_u64_impl.

> this with  currval < target_?

When two processes are hitting the cmpxchg concurrently, we need to
return the highest value that was written, even if it was the other
process that did it.  The assertions in the calling site are quickly
broken if we don't do this.  I admit this aspect took me by surprise.

> And why does target_ end in an underscore?

Heh, you tell me -- I just copied the style of the functions just above.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"No nos atrevemos a muchas cosas porque son difíciles,
pero son difíciles porque no nos atrevemos a hacerlas" (Séneca)