Hi, On Thu, Oct 16, 2025 at 6:32 PM wenhui qiu <[email protected]> wrote: > > HI Xuneng Zhou > > > - /* Unlock and release buffer */ > > - LockBuffer(buffer, BUFFER_LOCK_UNLOCK); > > - ReleaseBuffer(buffer); > > + UnlockReleaseBuffer(buffer); > > } > The previous suggestion to keep it was based on the fact that the original > code already had a similar comment. > In fact, the code itself is quite easy to understand. My earlier email was > simply following the style of the existing code when making the suggestion. > If anyone thinks the comment is unnecessary, it can certainly be removed. > > On Thu, Oct 16, 2025 at 5:39 PM Xuneng Zhou <[email protected]> wrote: >> >> Hi Kato-san, >> >> Thanks for looking into this. >> >> On Thu, Oct 16, 2025 at 4:21 PM Shinya Kato <[email protected]> wrote: >> > >> > Hi, >> > >> > On Wed, Oct 15, 2025 at 10:25 AM Xuneng Zhou <[email protected]> wrote: >> >> >> >> Hi, >> >> >> >> Here’s the updated summary report(cold cache, fragmented index), now >> >> including results for the streaming I/O + io_uring configuration. >> >> >> > >> > Thank you for the additional tests. I can see the image on Gmail, but I >> > cannot on pgsql-hackers archive [0], so it might be a good idea to attach >> > it and not to paste it on the body. >> >> Please see the attachment. >> >> > >> > >> > I saw the patch and have a few minor comments. >> > >> > + p.current_blocknum = 1; >> > >> > To improve readability, how about using the following, which is consistent >> > with nbtree.c [1]? >> > p.current_blocknum = BTREE_METAPAGE + 1; >> > >> > Similarly, for hash index: >> > p.current_blocknum = HASH_METAPAGE + 1; >> >> This is more readable. I made the replacements. >> >> > >> > + /* Unlock and release buffer */ >> > UnlockReleaseBuffer(buf); >> > >> > I think this comment is redundant since the function name >> > UnlockReleaseBuffer is self-explanatory. I suggest omitting it from >> > pgstathashindex and removing the existing one from pgstatindex_impl. >> >> UnlockReleaseBuffer seems clearer and simpler than the original >> >> > LockBuffer(buffer, BUFFER_LOCK_UNLOCK); >> > ReleaseBuffer(buffer); >> >> So the comment might be less meaningful for UnlockReleaseBuffer. I >> removed it as you suggested.
It might be worthwhile to benchmark this patch on a machine with non-SSD storage. However, I don’t currently have access to one, and still, I plan to mark patch v5 as Ready for Committer. Best, Xuneng
