Horton

Thread

Commits

  1. Don't cast off_t to 32-bit type for output, bug fix

  1. truncating casts of pgoff_t

    Peter Eisentraut <peter@eisentraut.org> — 2026-06-22T07:56:55Z

    I found a couple of places where a pgoff_t value, typically a signed 
64-bit integer, is cast to a type with a smaller range.  I noticed this 
specifically in error messages, so it's mostly cosmetic.  Also, files 
with a size where this would matter are not expected in many places, but 
I suspect that the case in basebackup_server.c in the attached patch can 
actually happen.

The fix is to use the %lld print format and a cast to long long int, 
which is the style already in most places.

In passing, I also converted a few places that used %llu to print 
pgoff_t to also use %lld instead.  This is mostly for consistency, but 
it also feels more robust in case a negative value ever sneaks in.

  2. Re: truncating casts of pgoff_t

    Heikki Linnakangas <hlinnaka@iki.fi> — 2026-06-22T08:55:45Z

    On 22/06/2026 10:56, Peter Eisentraut wrote:
> I found a couple of places where a pgoff_t value, typically a signed 64- 
> bit integer, is cast to a type with a smaller range.  I noticed this 
> specifically in error messages, so it's mostly cosmetic.  Also, files 
> with a size where this would matter are not expected in many places, but 
> I suspect that the case in basebackup_server.c in the attached patch can 
> actually happen.
> 
> The fix is to use the %lld print format and a cast to long long int, 
> which is the style already in most places.
> 
> In passing, I also converted a few places that used %llu to print 
> pgoff_t to also use %lld instead.  This is mostly for consistency, but 
> it also feels more robust in case a negative value ever sneaks in.

+1

In SendTimeLineHistory():

> 	/* XXX might truncate histfilelen */
> 	Assert(histfilelen <= UINT32_MAX);
> 	pq_sendint32(&buf, histfilelen);	/* col2 len */
> 
> 	bytesleft = histfilelen;
> 	while (bytesleft > 0)
> 	{
> 		PGAlignedBlock rbuf;
> 		int			nread;
> 
> 		pgstat_report_wait_start(WAIT_EVENT_WALSENDER_TIMELINE_HISTORY_READ);
> 		nread = read(fd, rbuf.data, sizeof(rbuf));
> 		pgstat_report_wait_end();
> 		if (nread < 0)
> 			ereport(ERROR,
> 					(errcode_for_file_access(),
> 					 errmsg("could not read file \"%s\": %m",
> 							path)));
> 		else if (nread == 0)
> 			ereport(ERROR,
> 					(errcode(ERRCODE_DATA_CORRUPTED),
> 					 errmsg("could not read file \"%s\": read %d of %zu",
> 							path, nread, (Size) bytesleft)));
> 
> 		pq_sendbytes(&buf, rbuf.data, nread);
> 		bytesleft -= nread;
> 	}

Not that it makes much difference, but I'd suggest "if (histfilelen > 
UINT32_MAX) elog(ERROR, ...)" here instead of an Assert. This isn't 
performance critical and a better error message is always nice if 
something weird happens. (I think on non-assertion builds, you'd get 
"out of memory" error while trying to increase the send buffer).

Not new with this patch, but I noticed that if the file increases in 
size while we're reading it for some reason, we would read beyond the 
originally calculated length. It really shouldn't happen, but it'd be 
good to add an "nread <= bytesleft" check here, for the sake of robustness.

- Heikki

  3. Re: truncating casts of pgoff_t

    Chao Li <li.evan.chao@gmail.com> — 2026-06-22T09:29:10Z

    
> On Jun 22, 2026, at 15:56, Peter Eisentraut <peter@eisentraut.org> wrote:
> 
> I found a couple of places where a pgoff_t value, typically a signed 64-bit integer, is cast to a type with a smaller range.  I noticed this specifically in error messages, so it's mostly cosmetic.  Also, files with a size where this would matter are not expected in many places, but I suspect that the case in basebackup_server.c in the attached patch can actually happen.
> 
> The fix is to use the %lld print format and a cast to long long int, which is the style already in most places.
> 
> In passing, I also converted a few places that used %llu to print pgoff_t to also use %lld instead.  This is mostly for consistency, but it also feels more robust in case a negative value ever sneaks in.
> <0001-Don-t-cast-pgoff_t-to-possibly-32-bit-types-for-outp.patch><0002-Print-off_t-pgoff_t-consistently-as-lld.patch>

I have small comment:

walsender.c
```
+	/* XXX might truncate histfilelen */
+	Assert(histfilelen <= UINT32_MAX);
```

histfilelen is returned from lseek(), that’s a runtime behavior. I’m afraid Assert is weak here. If we really worry about the truncate, should we use elog(ERROR).

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

  4. Re: truncating casts of pgoff_t

    Peter Eisentraut <peter@eisentraut.org> — 2026-07-07T09:59:29Z

    On 22.06.26 10:55, Heikki Linnakangas wrote:
> Not that it makes much difference, but I'd suggest "if (histfilelen > 
> UINT32_MAX) elog(ERROR, ...)" here instead of an Assert. This isn't 
> performance critical and a better error message is always nice if 
> something weird happens. (I think on non-assertion builds, you'd get 
> "out of memory" error while trying to increase the send buffer).

Ok, committed with an elog instead.

> Not new with this patch, but I noticed that if the file increases in 
> size while we're reading it for some reason, we would read beyond the 
> originally calculated length. It really shouldn't happen, but it'd be 
> good to add an "nread <= bytesleft" check here, for the sake of robustness.

Yes, this is being addressed in the thread "clean up size_t/ssize_t use 
with POSIX file system APIs".