Horton

Re: pgstattuple: Use streaming read API in pgstatindex functions

wenhui qiu <qiuwenhuifx@gmail.com>

View thread
From: wenhui qiu <qiuwenhuifx@gmail.com>
To: Xuneng Zhou <xunengzhou@gmail.com>
Cc: Shinya Kato <shinya11.kato@gmail.com>, pgsql-hackers <pgsql-hackers@lists.postgresql.org>, Nazir Bilal Yavuz <byavuz81@gmail.com>, Thomas Munro <thomas.munro@gmail.com>
Date: 2025-10-16T10:32:32Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Refactor logical worker synchronization code into a separate file.

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@gmail.com> wrote:

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