Re: Improve WALRead() to suck data directly from WAL buffers when possible
От | Bharath Rupireddy |
---|---|
Тема | Re: Improve WALRead() to suck data directly from WAL buffers when possible |
Дата | |
Msg-id | CALj2ACUTHE77R35-FiUT=TxJw=6Qmrc+6JVo1F=+AnyiL6pjOQ@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Improve WALRead() to suck data directly from WAL buffers when possible (Dilip Kumar <dilipbalaut@gmail.com>) |
Ответы |
Re: Improve WALRead() to suck data directly from WAL buffers when possible
(Dilip Kumar <dilipbalaut@gmail.com>)
|
Список | pgsql-hackers |
On Tue, Feb 7, 2023 at 4:12 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Mon, Dec 26, 2022 at 2:20 PM Bharath Rupireddy > <bharath.rupireddyforpostgres@gmail.com> wrote: > > I have gone through this patch, I have some comments (mostly cosmetic > and comments) Thanks a lot for reviewing. > From the above comments, it appears that we are reading from the exact > pointer we are interested to read, but actually, we are reading > the complete page. I think this comment needs to be fixed and we can > also explain why we read the complete page here. I modified it. Please see the attached v3 patch. > Generally, we use the name 'recptr' to represent XLogRecPtr type of > variable, but in your case, it is actually data at that recptr, so > better use some other name like 'buf' or 'buffer'. Changed it to use 'data' as it seemed more appropriate than just a buffer to not confuse with the WAL buffer page. > 3. > + if ((recptr + nbytes) <= (page + XLOG_BLCKSZ)) > + { > + } > + else if ((recptr + nbytes) > (page + XLOG_BLCKSZ)) > + { > > Why do you have this 'else if ((recptr + nbytes) > (page + > XLOG_BLCKSZ))' check in the else part? why it is not directly else > without a condition in 'if'? Changed. > I think we do not need 2 separate variables 'count' and '*read_bytes', > just one variable for input/output is sufficient. The original value > can always be stored in some temp variable > instead of the function argument. We could do that, but for the sake of readability and not cluttering the API, I kept it as-is. Besides addressing the above review comments, I've made some more changes - 1) I optimized the patch a bit by removing an extra memcpy. Up until v2 patch, the entire WAL buffer page is returned and the caller takes what is wanted from it. This adds an extra memcpy, so I changed it to avoid extra memcpy and just copy what is wanted. 2) I improved the comments. I can also do a few other things, but before working on them, I'd like to hear from others: 1. A separate wait event (WAIT_EVENT_WAL_READ_FROM_BUFFERS) for reading from WAL buffers - right now, WAIT_EVENT_WAL_READ is being used both for reading from WAL buffers and WAL files. Given the fact that we won't wait for a lock or do a time-taking task while reading from buffers, it seems unnecessary. 2. A separate TAP test for verifying that the WAL is actually read from WAL buffers - right now, existing tests for recovery, subscription, pg_walinspect already cover the code, see [1]. However, if needed, I can add a separate TAP test. 3. Use the oldest initialized WAL buffer page to quickly tell if the given LSN is present in WAL buffers without taking any lock - right now, WALBufMappingLock is acquired to do so. While this doesn't seem to impact much, it's good to optimize it away. But, the oldest initialized WAL buffer page isn't tracked, so I've put up a patch and sent in another thread [2]. Irrespective of [2], we are still good with what we have in this patch. [1] recovery tests: PATCHED: WAL buffers hit - 14759, misses - 3371 subscription tests: PATCHED: WAL buffers hit - 1972, misses - 32616 pg_walinspect tests: PATCHED: WAL buffers hit - 8, misses - 8 [2] https://www.postgresql.org/message-id/CALj2ACVgi6LirgLDZh%3DFdfdvGvKAD%3D%3DWTOSWcQy%3DAtNgPDVnKw%40mail.gmail.com -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Вложения
В списке pgsql-hackers по дате отправления: