Re: Switch buffile.c/h to use pgoff_t

From: Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Bryan Green <dbryan(dot)green(at)gmail(dot)com>
Subject: Re: Switch buffile.c/h to use pgoff_t
Date: 2025-12-25 02:40:02
Message-ID: CAEoWx2kwZBdsg6Gxck13jPEXvga2aH5fN8__ewgZ-hHwB1XHKA@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Thu, Dec 25, 2025 at 7:42 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:

> On Wed, Dec 24, 2025 at 05:01:57PM +0800, Chao Li wrote:
> > Make sense, bytestowrite is not a file offset. So, in the current code,
> > availbytes is not a file offset either, but it is defined as pgoff_t,
> which
> > has the same confusion, right? Also bytestowrite is casted to pgoff_t,
> it's
> > the same confusion again.
>
> Yeah, actually this suggestion makes more sense. availbytes is a
> computation made of a maximal size and an offset, so defining it as an
> offset from the start is kind of weird.
>
> Now I think that your suggested set of changes could become more
> consistent with a few more changes. For example, what about pos and
> nbytes in BufFile? While ssize_t is more consistent with FileRead()
> and FileWrite(), this code is written to care about signedness while
> ssize_t has a stricter range per posix, hence could int64 be a better
> choice for the whole interface? int64 is already what we use for
> BufFileSize(), which is due to the limit of MAX_PHYSICAL_FILESIZE of
> course.
> --
> Michael
>

Yeah, int64 feels a better fit.

WRT. changing pos and nbytes in BufFile to int64, I agree that also makes
sense. BufFile is a private struct in buffile.c, though it's indirectly
exposed by:
```
typedef struct BufFile BufFile;
```
None of the callers accesses its fields directly. Changing the two fields
from int to int64 will bump the structure size 16 bytes, but likely there
is not a case where numerous BufFile objects need to be created, thus
runtime impact should be minimal.

In attached v3, I have applied int64 to the 2 struct fields and
corresponding local variables. I ran a clean build, no warning was
introduced, and "make check" passed.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Attachment Content-Type Size
v3-0001-Refactor-BufFile-buffer-fields-to-eliminate-odd-t.patch application/octet-stream 2.6 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Zshengbin91 2025-12-25 03:05:15 improve comments on CreateFunction returnsSet
Previous Message Michael Paquier 2025-12-25 01:50:16 Re: Resetting recovery target parameters in pg_createsubscriber