| From: | Peter Eisentraut <peter(at)eisentraut(dot)org> |
|---|---|
| To: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
| Subject: | faulty error handling around pgstat_count_io_op_time() |
| Date: | 2026-06-12 16:01:45 |
| Message-ID: | 0db864e6-4477-4eba-b2be-d3523cc86564@eisentraut.org |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
There are several places where the return value of pg_pread() or
pg_pwrite() is passed directly as the byte count to
pgstat_count_io_op_time(). The bytes argument of
pgstat_count_io_op_time() is of type uint64, and so error returns of -1
are going to passed as UINT64_MAX and added as such to the internal
statistics.
In the attached patch, I have marked up those places.
I think the correction here would be to move the
pgstat_count_io_op_time() calls to after the error returns are handled.
This is effectively how most other code already behaves. For example,
most smgr calls don't return on error, so you don't get a chance to make
any pgstat calls afterwards. It's only the open-coded places where we
can even do that.
However, XLogPageRead() even goes out of its way to make an explicit
pgstat_count_io_op_time() call in the error branch. I suppose this
could be useful to record short reads, but a) this particular instance
is still faulty regarding -1, and b) other places don't do that. So
it's a bit unclear what the preferred behavior on error should be.
An alternative would be to call pgstat_count_io_op_time() with like
Max(byteswritten, 0), but that seems kind of ugly.
Another alternative would be to change the bytes argument of
pgstat_count_io_op_time() to ssize_t. POSIX file system operations
can't operate on sizes larger than ssize_t, so this type should be
sufficient. And then error returns could be handled centrally in
pgstat_count_io_op_time(). (Record them, don't record them, or even
count errors separately, etc.)
Thoughts?
| Attachment | Content-Type | Size |
|---|---|---|
| 0001-Mark-up-faulty-error-handling-around-pgstat_count_io.patch | text/plain | 2.5 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Peter Geoghegan | 2026-06-12 16:10:39 | Re: s/pg_attribute_always_inline/pg_always_inline/? |
| Previous Message | Haoyu Huang | 2026-06-12 15:51:16 | Re: Better shared data structure management and resizable shared data structures |