Message-ID: <5e3a2a809388bc01b66bf7fa692eaac5622acc0a.camel@j-davis.com>
Subject: Re: LogwrtResult contended spinlock
From: Jeff Davis <pgsql@j-davis.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, Tom Lane
 <tgl@sss.pgh.pa.us>, Andres Freund <andres@anarazel.de>, 
 pgsql-hackers@lists.postgresql.org, Jaime Casanova
 <jcasanov@systemguards.com.ec>
Date: Thu, 04 Apr 2024 15:50:58 -0700
In-Reply-To: <202404041745.jzrrw3ffyac7@alvherre.pgsql>
References: <202404041745.jzrrw3ffyac7@alvherre.pgsql>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu2 
MIME-Version: 1.0
Archived-At: <https://www.postgresql.org/message-id/5e3a2a809388bc01b66bf7fa692eaac5622acc0a.camel%40j-davis.com>
Precedence: bulk

On Thu, 2024-04-04 at 19:45 +0200, Alvaro Herrera wrote:
> 1. Using pg_atomic_write_membarrier_u64 is useless and it imposes
> mora
> barriers than we actually need.=C2=A0 So I switched back to
> pg_atomic_write_u64 and add one barrier between the two writes.=C2=A0 Sam=
e
> for reads.

+1.

This looks correct to me. Just before the writes there's a spinlock,
which acts as a full barrier; and just afterwards, the function returns
and the WALWriteLock is released, again acting as a full barrier. The
write barrier in between enforces the Write >=3D Flush invariant.

> 2. Using monotonic_advance for Write and Flush is useless.

+1.

> 3. Testing the invariant that the Copy pointer cannot be 0 is
> useless,
> because we initialize that pointer to EndOfLog during StartupXLOG.
> So, removed.

+1.

> 4. If we're not modifying any callers of WALReadFromBuffers(), then
> AFAICS the added check that the request is not past the Copy pointer
> is
> useless.=C2=A0 In a quick look at that code, I think we only try to read
> data
> that's been flushed, not written, so the stricter check that we don't
> read data that hasn't been Copied does nothing.

Bharath has indicated that he may call WALReadFromBuffers() in an
extension, so I believe some error checking is appropriate there.

> =C2=A0 (Honestly I'm not sure
> that we want XLogSendPhysical to be reading data that has not been
> written, or do we?)

Not yet, but there has been some discussion[1][2] about future work to
allow replicating data before it's been flushed locally.

> =C2=A0 Please argue why we need this patch.

I'm not sure what you mean by "this patch"?

> 5. The existing weird useless-looking block at the end of XLogWrite
> is
> there because we used to have it to declare a volatile pointer to
> XLogCtl (cf.=C2=A0 commit 6ba4ecbf477e).=C2=A0 That's no longer needed, s=
o we
> could remove it.=C2=A0 Or we could leave it alone (just because it's
> ancient
> and it doesn't hurt anything), but there's no reason to have the new
> invariant-testing block inside the same block.=C2=A0 So I added another
> weird
> useless-looking block, except that this one does have two variable
> declaration at its top.

That didn't bother me, but it could be cleaned up a bit in a later
patch.

> 6. In a few places, we read both Write and Flush to only use one of
> them.=C2=A0 This is wasteful and we could dial this back to reading only
> the
> one we need.=C2=A0 Andres suggested as much in [1].=C2=A0 I didn't touch =
this
> in
> the current patch, and I don't necessarily think we need to address
> it
> right now.=C2=A0 Addressing this should probably done similar to what I
> posted in [2]'s 0002.

I agree that it should be a separate patch. I haven't thought about the
consequences of making them fully independent -- I think that means we
give up the invariant that Copy >=3D Write >=3D Flush?


Regarding the patches themselves, 0001 looks good to me.

For 0002, did you consider having pg_atomic_monotonic_advance_u64()
return the currval?
=09
Regards,
	Jeff Davis

[1]
https://www.postgresql.org/message-id/CALj2ACV6rS%2B7iZx5%2BoAvyXJaN4AG-djA=
QeM1mrM%3DYSDkVrUs7g%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/20230125211540.zylu74dj2uuh3k7w%40awo=
rk3.anarazel.de
[3]
https://www.postgresql.org/message-id/CALj2ACW65mqn6Ukv57SqDTMzAJgd1N_AdQtD=
gy%2BgMDqu6v618Q%40mail.gmail.com