From: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | 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: | 2022-03-22 18:58:34 |
Message-ID: | 202203221858.vx3ssvehtkfj@alvherre.pgsql |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
So I've been wondering about this block at the bottom of XLogWrite:
/*
* Make sure that the shared 'request' values do not fall behind the
* 'result' values. This is not absolutely essential, but it saves some
* code in a couple of places.
*/
{
SpinLockAcquire(&XLogCtl->info_lck);
if (XLogCtl->LogwrtRqst.Write < LogwrtResult.Write)
XLogCtl->LogwrtRqst.Write = LogwrtResult.Write;
if (XLogCtl->LogwrtRqst.Flush < LogwrtResult.Flush)
XLogCtl->LogwrtRqst.Flush = LogwrtResult.Flush;
SpinLockRelease(&XLogCtl->info_lck);
}
I just noticed that my 0001 makes the comment a lie: it is now quite
possible that 'result' is advanced beyond 'request'. Before the patch
that never happened because they were both advanced in the region locked
by the spinlock.
I think we could still maintain this promise if we just moved this
entire block before the first pg_atomic_monotonic_advance_u64 setting
XLogCtl->LogwrtResult.Write. Or we could halve the whole block, and put
one acquire/test/set/release stanza before each monotonic increase of
the corresponding variable.
However, I wonder if this is still necessary. This code was added in
4d14fe0048c (March 2001) and while everything else was quite different
back then, this hasn't changed at all. I can't quite figure out what
are those "couple of places" that would need additional code if this
block is just removed. I tried running the tests (including
wal_consistency_checking), and nothing breaks. Reading the code
surrounding the other accesses of XLogCtl->LogwrtRqst, there's nothing
that looks to me like it depends on these values not lagging behind
LogwrtResult.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"If you have nothing to say, maybe you need just the right tool to help you
not say it." (New York Times, about Microsoft PowerPoint)
From | Date | Subject | |
---|---|---|---|
Next Message | Andres Freund | 2022-03-22 19:01:36 | Re: LockAcquireExtended() dontWait vs weaker lock levels than already held |
Previous Message | Robert Haas | 2022-03-22 18:20:55 | Re: LockAcquireExtended() dontWait vs weaker lock levels than already held |