Re: LogwrtResult contended spinlock

From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
To: Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>
Cc: Jeff Davis <pgsql(at)j-davis(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Andres Freund <andres(at)anarazel(dot)de>, pgsql-hackers(at)lists(dot)postgresql(dot)org, Jaime Casanova <jcasanov(at)systemguards(dot)com(dot)ec>
Subject: Re: LogwrtResult contended spinlock
Date: 2024-04-03 11:19:40
Message-ID: 202404031119.cd2kugjk2vho@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Thanks for keeping this moving forward. I gave your proposed patches a
look. One thing I didn't like much is that we're adding a new member
(Copy) to XLogwrtAtomic -- but this struct is supposed to be a mirror of
XLogwrtResult for use with atomic access. Since this new member is not
added to XLogwrtResult (because it's not needed there), the whole idea
of there being symmetry between those two structs crumbles down.
Because we later stop using struct-assign anyway, meaning we no longer
need the structs to match, we can instead spell out the members in
XLogCtl and call it a day.

So what I do in the attached 0001 is stop using the XLogwrtResult struct
in XLogCtl and replace it with separate Write and Flush values, and add
the macro XLogUpdateLocalLogwrtResult() that copies the values of Write
and Flush from the shared XLogCtl to the local variable given as macro
argument. (I also added our idiomatic do {} while(0) to the macro
definition, for safety). The new members are XLogCtl->logWriteResult
and XLogCtl->logFlushResult and as of 0001 are just XLogRecPtr, so
essentially identical semantics as the previous code. No atomic access

0002 then adds pg_atomic_monotonic_advance_u64. (I don't add the _u32
variant, because I don't think it's a great idea to add dead code. If
later we see a need for it we can put it in.) It also changes the two
new members to be atomics, changes the macro to use atomic read, and
XLogWrite now uses monotonic increment. A couple of other places can
move the macro calls to occur outside the spinlock. Also, XLogWrite
gains the invariant checking that involves Write and Flush.

Finally, 0003 adds the Copy pointer to XLogCtl alongside Write and
Flush, and updates WALReadFromBuffers to test that instead of the Write
pointer, and adds in XLogWrite the invariant checks that involve the
Copy pointer.

I haven't rerun Bharath test loop yet; will do so shortly.

Álvaro Herrera PostgreSQL Developer —
"You're _really_ hosed if the person doing the hiring doesn't understand
relational systems: you end up with a whole raft of programmers, none of
whom has had a Date with the clue stick." (Andrew Sullivan)

Attachment Content-Type Size
v15-0001-split-XLogCtl-LogwrtResult-into-separate-struct-.patch text/x-diff 7.4 KB
v15-0002-Make-XLogCtl-log-Write-Flush-Result-accessible-w.patch text/x-diff 8.3 KB
v15-0003-Add-Copy-pointer-to-track-data-copied-to-WAL-buf.patch text/x-diff 4.0 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Ranier Vilela 2024-04-03 11:36:47 Re: Fix out-of-bounds in the function PQescapeinternal (src/interfaces/libpq/fe-exec.c)
Previous Message Amit Kapila 2024-04-03 10:49:46 Re: Introduce XID age and inactive timeout based replication slot invalidation