At Fri, 16 Feb 2024 19:50:00 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in
> On Fri, Feb 16, 2024 at 7:10 AM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> > 1. It's useless to copy the whole page regardless of the 'count'. It's
> > enough to copy only up to the 'count'. The patch looks good in this
> > regard.
>
> Yes, it's not needed to copy the whole page. Per my understanding
> about page transfers between disk and OS page cache - upon OS page
> cache miss, the whole page (of disk block size) gets fetched from disk
> even if we just read 'count' bytes (< disk block size).
Right, but with a possibly-different block size. Anyway that behavior
doesn't affect the result of this change. (Could affect performance
hereafter if it were not the case, though..)
> > 2. Maybe we need a comment that states the page_read callback
> > functions leave garbage bytes beyond the returned count, due to
> > partial copying without clearing the unused portion.
>
> Isn't the comment around page_read callback at
>
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/include/access/xlogreader.h;h=2e9e5f43eb2de1ca9ba81afe76d21357065c61aa;hb=d57b7cc3338e9d9aa1d7c5da1b25a17c5a72dcce#l78
> enough?
>
> "The callback shall return the number of bytes read (never more than
> XLOG_BLCKSZ), or -1 on failure."
Yeah, perhaps I was overly concerned. The removed comment made me
think that someone could add code relying on the incorrect assumption
that the remaining bytes beyond the returned count are cleared out. On
the flip side, SimpleXLogPageRead always reads a whole page and
returns XLOG_BLCKSZ. However, as you know, the returned buffer doesn't
contain random garbage bytes. Therefore, it's safe as long as the
caller doesn't access beyond the returned count. As a result, the
description you pointed out seems to be enough.
After all, the patch looks good to me.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
