Re: LogwrtResult contended spinlock
От | Bharath Rupireddy |
---|---|
Тема | Re: LogwrtResult contended spinlock |
Дата | |
Msg-id | CALj2ACW0K4UQY6s4xK6hjASJuoPfk=v3ZaKTpojL7Xz+RiJkAA@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: LogwrtResult contended spinlock (Alvaro Herrera <alvherre@alvh.no-ip.org>) |
Ответы |
Re: LogwrtResult contended spinlock
|
Список | pgsql-hackers |
On Wed, Apr 3, 2024 at 4:49 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > 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. Hm, I have no objection to having separate variables in XLogCtl. > 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 > yet! +1. > 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. I'm fine with not having the 32 bit variant of pg_atomic_monotonic_advance. However, a recent commit bd5132db5 added both 32 and 64 bit versions of pg_atomic_read_membarrier even though 32 bit isn't being used. > 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. +1. The attached patches look good to me. Also, I'm fine with renaming XLogUpdateLocalLogwrtResult() to RefreshXLogWriteResult(). > I haven't rerun Bharath test loop yet; will do so shortly. I ran it a 100 times [1] on top of all the 3 patches, it looks fine. [1] for i in {1..100}; do make check PROVE_TESTS="t/027_stream_regress.pl"; if [ $? -ne 0 ]; then echo "The command failed on iteration $i"; break; fi; done ./configure --prefix=$PWD/pg17/ --enable-debug --enable-tap-tests --enable-cassert CC=/usr/bin/clang-14 > install.log && make -j 8 install > install.log 2>&1 & -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Matthias van de MeentДата:
Сообщение: Re: Detoasting optionally to make Explain-Analyze less misleading