Re: pgstattuple: Use streaming read API in pgstatindex functions

Hi,

On Thu, Oct 16, 2025 at 6:32 PM wenhui qiu <qiuwenhuifx(at)gmail(dot)com> 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 <xunengzhou(at)gmail(dot)com> wrote:
>>
>> Hi Kato-san,
>>
>> Thanks for looking into this.
>>
>> On Thu, Oct 16, 2025 at 4:21 PM Shinya Kato <shinya11(dot)kato(at)gmail(dot)com> wrote:
>> >
>> > Hi,
>> >
>> > On Wed, Oct 15, 2025 at 10:25 AM Xuneng Zhou <xunengzhou(at)gmail(dot)com> 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