Re: LogwrtResult contended spinlock
От | Bharath Rupireddy |
---|---|
Тема | Re: LogwrtResult contended spinlock |
Дата | |
Msg-id | CALj2ACUBFGuBwtSToDbxmKjJ4EMZS7c5wFu4wou2DjKrFPfCEg@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: LogwrtResult contended spinlock (Jeff Davis <pgsql@j-davis.com>) |
Ответы |
Re: LogwrtResult contended spinlock
|
Список | pgsql-hackers |
On Fri, Apr 5, 2024 at 4:21 AM Jeff Davis <pgsql@j-davis.com> wrote: > > > 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. 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. > > > (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. Right. Although callers of WALReadFromBuffers() in core postgres doesn't need it now (in future, they will), but it allows one to write something up externally - for example, see 0004 patch in https://www.postgresql.org/message-id/CALj2ACWmW+1ZdYE0BD5-4KVLzYGn3=pudomwPHuHmi4YQuOFSg@mail.gmail.com where I implemented a xlogreader page_read callback that just keeps reading the WAL that's fully copied to WAL buffers. > Regarding the patches themselves, 0001 looks good to me. A few comments on 0001: 1. /* * Update local copy of shared XLogCtl->log{Write,Flush}Result + * + * It's critical that Flush always trails Write, so the order of the reads is + * important, as is the barrier. */ #define RefreshXLogWriteResult(_target) \ do { \ - _target.Write = XLogCtl->logWriteResult; \ - _target.Flush = XLogCtl->logFlushResult; \ + _target.Flush = pg_atomic_read_u64(&XLogCtl->logFlushResult); \ + pg_read_barrier(); \ + _target.Write = pg_atomic_read_u64(&XLogCtl->logWriteResult); \ } while (0) Is it "Flush always trails Write" or "Flush always leades Write"? I guess the latter, no? 2. + + pg_atomic_write_u64(&XLogCtl->logWriteResult, LogwrtResult.Write); + pg_write_barrier(); + pg_atomic_write_u64(&XLogCtl->logFlushResult, LogwrtResult.Flush); } Maybe add the reason as to why we had to write logWriteResult first and then logFlushResult, similar to the comment atop RefreshXLogWriteResult? > For 0002, did you consider having pg_atomic_monotonic_advance_u64() > return the currval? +1 for returning currval here. Except for the above comments, the patches look good to me. I've also run the test loop for any assertion failure - 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. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
В списке pgsql-hackers по дате отправления: