On Thu, Oct 15, 2026 at 2:09 PM Heikki Linnakangas <[email protected]> wrote:
>
> On 07/10/2026 12:55, Jakub Wartak wrote:
> > On Tue, Oct 6, 2026 at 5:42 PM Heikki Linnakangas <[email protected]> wrote:
> >
> >> I think that comment is wrong. A short read is possible for other
> >> reasons than a truncated file, e.g. if the read is interrupted by a
> >> signal. In the past, we've made that same assumption that you never get
> >> a short read on a BLCKSZ-sized read in other places too, but it was
> >> always questionable. Thomas Munro fixed it for smgrread() in commit
> >> 4908c58720.
> >>
> >> I think we should now fix that in read_file_data_into_buffer(), too.
> >> Make it retry, so that it never returns fewer bytes than requested
> >> except for EOF. With a larger buffer, a short read becomes more likely.
> >
> > OK, I see, thanks, so I've tried this in the attached 0004.
>
> Ok, looks good as far as it goes. But looking at basebackup_read_file(),
> I think there's a race condition with partial_read_ok == false. This
> could happen:
>
> 0. read_file_data_into_buffer() observes a checksum failure thanks to a
> torn write, and retries reading a single BLCKSZ block.
> 1. basebackup_read_file() is called with partial_read_ok == false,
> nbytes=BLCKSZ.
> 2. It reads the first 4096 bytes of the block. (e.g. because of
> SIMULATE_SHORT_READ, or EINTR)
> 3. The file is truncated
> 4. basebackup_read_file() makes another pg_pread() call to read the rest
> of the block. It returns 0, because the file was truncated
> 5. basebackup_read_file() throws a partial read error: "could not read
> file \"%s\": read %zu of %zu".
>
> If the truncation happened earlier, before the first read, or later,
> after the whole block has been read, you don't get the error.
Hi Heikki,
So the 0001 is the previous one, and and 0002 is about fixing that
SIMULATE_SHORT_READ/EINTR situation above (separate just to ease the review).
I suspect we should never assume that basebackup_read_file() with
partial_read_ok==false is fine, so it was my starting point, so:
- reread_cnt = basebackup_read_file(..., partial_read_ok=false)
- if (reread_cnt == 0)
+ reread_cnt = basebackup_read_file(..., partial_read_ok=true)
+ if (reread_cnt != BLCKSZ)
and that was it's only user, so it's gone now.
But there's more and it deserves some explanation: when running above
WIP-fix to that reread race condition and simulating EINTR/short reads
and backing up things things in parallel all the time and having this
setup:
1. #define SIMULATE_SHORT_READ
2. INSERT INTO t SELECT generate_series(1, 10000); \watch 0.0001
3. TRUNCATE t; \watch 0.0001 -- it kept on rotating relfilenode a lot
4. optional: strace in parallel (even more EINTR and stuff)
5. while : ; do rm -rf /tmp/full1 && install/bin/pg_basebackup -c fast \
-h 127.0.0.1 -D /tmp/full1 && install/bin/pg_verifybackup /tmp/full1; \
done
I've started receving haywire stuff like:
WARNING: could not verify checksum in file "./base/5/157555", block 32:
read buffer size 167936 and page size 8192 differ
DETAIL: bytes_done=262144 filestat.st_size=3629056 fstat_rc=0 fstat_size=0
Please notice the 2nd fstat report size of 0 (truncation) vs 3629056, so I
had to fix that further, so that's why I landed with is_relfile and inside
read_file_data_into_buffer() there's now:
if (is_relfile && cnt < requested && (cnt % BLCKSZ) != 0)
hopefully comment explains the rationale.
The errdetail() is still there and I would like to keep it , but maybe
some better form or shape or maybe better leave it as separate DEBUG1
to still being able to troubleshoot this code (it's hard).
A thorough review would be highly advisable, as this is like walking on thin
ice and a lot of smart people complained about this code in the past.
-J.
From fb3953a3f57933db433164fa7c8504aeceec2bb3 Mon Sep 17 00:00:00 2001
From: Jakub Wartak <[email protected]>
Date: Thu, 29 Oct 2026 09:34:50 +0100
Subject: [PATCH v29102026 2/2] Fix re-reading and short reads races. Outcome
of testing with -DSIMULATE_SHORT_READS.
---
src/backend/backup/basebackup.c | 68 +++++++++++++++++++++++----------
1 file changed, 47 insertions(+), 21 deletions(-)
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c
index 83da382e4e8..25914c5b1d3 100644
--- a/src/backend/backup/basebackup.c
+++ b/src/backend/backup/basebackup.c
@@ -107,6 +107,7 @@ static off_t read_file_data_into_buffer(bbsink *sink,
const char *readfilename, int fd,
off_t offset, size_t length,
BlockNumber blkno,
+ bool is_relfile,
bool verify_checksum,
int *checksum_failures);
static void push_to_sink(bbsink *sink, pg_checksum_context *checksum_ctx,
@@ -128,7 +129,7 @@ static void perform_base_backup(basebackup_options *opt, bbsink *sink,
static void parse_basebackup_options(List *options, basebackup_options *opt);
static int compareWalFileNames(const ListCell *a, const ListCell *b);
static ssize_t basebackup_read_file(int fd, char *buf, size_t nbytes, off_t offset,
- const char *filename, bool partial_read_ok);
+ const char *filename);
/* Was the backup currently in-progress initiated in recovery mode? */
static bool backup_started_in_recovery = false;
@@ -594,7 +595,7 @@ perform_base_backup(basebackup_options *opt, bbsink *sink,
while ((cnt = basebackup_read_file(fd, sink->bbs_buffer,
Min(sink->bbs_buffer_length,
wal_segment_size - len),
- len, pathbuf, true)) > 0)
+ len, pathbuf)) > 0)
{
CheckXLogRemoved(segno, tli);
bbsink_archive_contents(sink, cnt);
@@ -1742,6 +1743,7 @@ sendFile(bbsink *sink, const char *readfilename, const char *tarfilename,
cnt = read_file_data_into_buffer(sink, readfilename, fd,
bytes_done, remaining,
blkno + segno * RELSEG_SIZE,
+ RelFileNumberIsValid(relfilenumber),
verify_checksum,
&checksum_failures);
}
@@ -1764,6 +1766,7 @@ sendFile(bbsink *sink, const char *readfilename, const char *tarfilename,
relative_blkno * BLCKSZ,
BLCKSZ,
relative_blkno + segno * RELSEG_SIZE,
+ true,
verify_checksum,
&checksum_failures);
@@ -1771,7 +1774,7 @@ sendFile(bbsink *sink, const char *readfilename, const char *tarfilename,
* read_file_data_into_buffer() retries short reads, so if we got
* fewer bytes than requested we must have hit EOF (which means
* that the relation is being truncated). Ultimately, it should be
- * truncated to a multiple of BLCKSZ, since this path should only
+ * truncated to a multiple of BLCKSZ, since this path should only
* be reached for relation files, but we might transiently observe
* an intermediate value.
*
@@ -1791,11 +1794,22 @@ sendFile(bbsink *sink, const char *readfilename, const char *tarfilename,
backup_checksums_verifiable(sink->bbs_state->startptr) &&
(cnt % BLCKSZ != 0))
{
+ /* Gain more insight into the state of file now */
+ struct stat fstatdbg;
+ int fstatrc = fstat(fd, &fstatdbg);
+
ereport(WARNING,
(errmsg("could not verify checksum in file \"%s\", block "
"%u: read buffer size %d and page size %d "
"differ",
- readfilename, blkno, (int) cnt, BLCKSZ)));
+ readfilename, blkno, (int) cnt, BLCKSZ),
+ errdetail("bytes_done=%lld filestat_size=%lld "
+ "fstat_rc=%d fstat_size=%lld",
+ (long long) bytes_done,
+ (long long) filestat.st_size,
+ fstatrc,
+ (long long) (fstatrc == 0 ? fstatdbg.st_size : -1))));
+
verify_checksum = false;
}
@@ -1885,6 +1899,8 @@ sendFile(bbsink *sink, const char *readfilename, const char *tarfilename,
* 'blkno' is the block number of the first page in the bbsink's buffer
* relative to the start of the relation.
*
+ * 'is_relfile' determines if this is a relation file.
+ *
* 'verify_checksum' determines if the user has asked to verify checksums, but
* since data checksums can be disabled, or become disabled, we need to check
* state before verifying individual pages. If we do this, we'll update
@@ -1893,16 +1909,30 @@ sendFile(bbsink *sink, const char *readfilename, const char *tarfilename,
static off_t
read_file_data_into_buffer(bbsink *sink, const char *readfilename, int fd,
off_t offset, size_t length, BlockNumber blkno,
- bool verify_checksum, int *checksum_failures)
+ bool is_relfile, bool verify_checksum,
+ int *checksum_failures)
{
off_t cnt;
+ size_t requested = Min(sink->bbs_buffer_length, length);
int i;
char *page;
/* Try to read some more data. */
- cnt = basebackup_read_file(fd, sink->bbs_buffer,
- Min(sink->bbs_buffer_length, length),
- offset, readfilename, true);
+ cnt = basebackup_read_file(fd, sink->bbs_buffer, requested,
+ offset, readfilename);
+
+ /*
+ * Short read of relations: if we got less than we asked for, we hit EOF, so
+ * the file is being truncated concurrently. For relation files truncation
+ * happens in whole blocks, but apparently the OS might copy data page by
+ * OS page size (4kB), so our big read happening with the truncation can
+ * end in the middle of a block. Treat that partial block as truncated as
+ * WAL replay will fix this anyway.
+ *
+ * For non-relation there's no WAL replay, so we just use what we have.
+ */
+ if (is_relfile && cnt < requested && (cnt % BLCKSZ) != 0)
+ cnt -= cnt % BLCKSZ;
/* Can't verify checksums if read length is not a multiple of BLCKSZ. */
if (!verify_checksum || (cnt % BLCKSZ) != 0)
@@ -1952,13 +1982,18 @@ read_file_data_into_buffer(bbsink *sink, const char *readfilename, int fd,
reread_cnt =
basebackup_read_file(fd, sink->bbs_buffer + BLCKSZ * i,
BLCKSZ, offset + BLCKSZ * i,
- readfilename, false);
- if (reread_cnt == 0)
+ readfilename);
+ if (reread_cnt != BLCKSZ)
{
/*
* If we hit end-of-file, a concurrent truncation must have
* occurred, so reduce cnt to reflect only the blocks already
* processed and break out of this loop.
+ *
+ * In unlikely situation where reread ended up getting only part of
+ * the block, we classify this as truncation too (We drop that block
+ * and everything after it instead of raising read X out Y bytes and
+ * aborting the backup).
*/
cnt = BLCKSZ * i;
break;
@@ -2209,16 +2244,13 @@ convert_link_to_directory(const char *pathbuf, struct stat *statbuf)
/*
* Read some data from a file, setting a wait event and reporting any error
* encountered. Short reads are retried until either all requested bytes have
- * been read or EOF is hit ( potentially due to concurrent truncation).
- *
- * If partial_read_ok is false, also report an error if the number of bytes
- * read is not equal to the number of bytes requested.
+ * been read or EOF is hit (potentially due to concurrent truncation).
*
* Returns the number of bytes read.
*/
static ssize_t
basebackup_read_file(int fd, char *buf, size_t nbytes, off_t offset,
- const char *filename, bool partial_read_ok)
+ const char *filename)
{
ssize_t transferred = 0;
@@ -2254,11 +2286,5 @@ basebackup_read_file(int fd, char *buf, size_t nbytes, off_t offset,
transferred += rc;
}
- if (!partial_read_ok && transferred > 0 && transferred != nbytes)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not read file \"%s\": read %zu of %zu",
- filename, transferred, nbytes)));
-
return transferred;
}
--
2.43.0
From ea416741a9693792ccb60094c89ed78155f096c5 Mon Sep 17 00:00:00 2001
From: Jakub Wartak <[email protected]>
Date: Wed, 7 Oct 2026 10:36:21 +0200
Subject: [PATCH v29102026 1/2] basebackup: bump SINK_BUFFER_LENGTH to 256kB
By increasing SINK_BUFFER_LENGTH size to be closer to modern L2 CPU cache
sizes, we are also reducing frequency of the pread() system call which
also helps avoids performance penalties (and is visible as one of the
top functions when sending backups). In isolated performance runs where
pg_basebackup client did not have write it's backup to disk, this gives
~180% of the baseline (3.1GB/s -> 5.5GB/s).
Along the way try to improve the code for handling short reads in basebackup's
basebackup_read_file() as higher buffer size could translate to higher risk
of pread() interruptions.
Author: Jakub Wartak <[email protected]>
Reviewed-by: Heikki Linnakangas <[email protected]>
Discussion: https://postgr.es/m/CAKZiRmwwW-hDc3B6ERJB%2BpaX7RNSBcQLheq1KdsTf42cGuRvuA%40mail.gmail.com
---
src/backend/backup/basebackup.c | 69 ++++++++++++++++--------
src/backend/backup/basebackup_progress.c | 2 +-
2 files changed, 49 insertions(+), 22 deletions(-)
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c
index f8c3520e5b8..83da382e4e8 100644
--- a/src/backend/backup/basebackup.c
+++ b/src/backend/backup/basebackup.c
@@ -62,7 +62,7 @@
* NB: The buffer size is required to be a multiple of the system block
* size, so use that value instead if it's bigger than our preference.
*/
-#define SINK_BUFFER_LENGTH Max(32768, BLCKSZ)
+#define SINK_BUFFER_LENGTH Max(256 * 1024, BLCKSZ)
typedef struct
{
@@ -1768,11 +1768,12 @@ sendFile(bbsink *sink, const char *readfilename, const char *tarfilename,
&checksum_failures);
/*
- * If we get a partial read, that must mean that the relation is
- * being truncated. Ultimately, it should be truncated to a
- * multiple of BLCKSZ, since this path should only be reached for
- * relation files, but we might transiently observe an
- * intermediate value.
+ * read_file_data_into_buffer() retries short reads, so if we got
+ * fewer bytes than requested we must have hit EOF (which means
+ * that the relation is being truncated). Ultimately, it should be
+ * truncated to a multiple of BLCKSZ, since this path should only
+ * be reached for relation files, but we might transiently observe
+ * an intermediate value.
*
* It should be fine to treat this just as if the entire block had
* been truncated away - i.e. fill this and all later blocks with
@@ -1877,8 +1878,9 @@ sendFile(bbsink *sink, const char *readfilename, const char *tarfilename,
* 'offset' is the file offset from which we should begin to read, and
* 'length' is the amount of data that should be read. The actual amount
* of data read will be less than the requested amount if the bbsink's
- * buffer isn't big enough to hold it all, or if the underlying file has
- * been truncated. The return value is the number of bytes actually read.
+ * buffer isn't big enough to hold it all, or if we hit EOF, which means
+ * that the underlying file has been truncated. The return value is the
+ * number of bytes actually read.
*
* 'blkno' is the block number of the first page in the bbsink's buffer
* relative to the start of the relation.
@@ -2206,7 +2208,8 @@ convert_link_to_directory(const char *pathbuf, struct stat *statbuf)
/*
* Read some data from a file, setting a wait event and reporting any error
- * encountered.
+ * encountered. Short reads are retried until either all requested bytes have
+ * been read or EOF is hit ( potentially due to concurrent truncation).
*
* If partial_read_ok is false, also report an error if the number of bytes
* read is not equal to the number of bytes requested.
@@ -2217,21 +2220,45 @@ static ssize_t
basebackup_read_file(int fd, char *buf, size_t nbytes, off_t offset,
const char *filename, bool partial_read_ok)
{
- ssize_t rc;
+ ssize_t transferred = 0;
- pgstat_report_wait_start(WAIT_EVENT_BASEBACKUP_READ);
- rc = pg_pread(fd, buf, nbytes, offset);
- pgstat_report_wait_end();
+ /*
+ * Loop to continue after a short read. Short reads are more likely to
+ * show up with large read requests such as ours due to e.g. signals. This
+ * tries to mirror what mdreadv() does.
+ */
+ while (transferred < nbytes)
+ {
+ ssize_t rc;
- if (rc < 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not read file \"%s\": %m", filename)));
- if (!partial_read_ok && rc > 0 && rc != nbytes)
+ pgstat_report_wait_start(WAIT_EVENT_BASEBACKUP_READ);
+ rc = pg_pread(fd, buf + transferred, nbytes - transferred,
+ offset + transferred);
+ pgstat_report_wait_end();
+
+#ifdef SIMULATE_SHORT_READ
+ rc = Min(rc, 4096);
+#endif
+ if (rc < 0 && errno == EINTR)
+ continue;
+
+ if (rc < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not read file \"%s\": %m", filename)));
+
+ /* EOF */
+ if (rc == 0)
+ break;
+
+ transferred += rc;
+ }
+
+ if (!partial_read_ok && transferred > 0 && transferred != nbytes)
ereport(ERROR,
(errcode_for_file_access(),
- errmsg("could not read file \"%s\": read %zd of %zu",
- filename, rc, nbytes)));
+ errmsg("could not read file \"%s\": read %zu of %zu",
+ filename, transferred, nbytes)));
- return rc;
+ return transferred;
}
diff --git a/src/backend/backup/basebackup_progress.c b/src/backend/backup/basebackup_progress.c
index f74459181d0..1d8b24dfdc9 100644
--- a/src/backend/backup/basebackup_progress.c
+++ b/src/backend/backup/basebackup_progress.c
@@ -63,7 +63,7 @@ bbsink_progress_new(bbsink *next, bool estimate_backup_size, bool incremental)
Assert(next != NULL);
- sink = palloc0_object(bbsink);
+ sink = palloc_aligned(sizeof(bbsink), PG_IO_ALIGN_SIZE, MCXT_ALLOC_ZERO);
*((const bbsink_ops **) &sink->bbs_ops) = &bbsink_progress_ops;
sink->bbs_next = next;
--
2.43.0
