| 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 |
| 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 |