On Wed, Nov 4, 2026 at 11:45 PM Heikki Linnakangas <[email protected]> wrote:
>
> On 29/10/2026 16:16, Jakub Wartak wrote:
> > On Thu, Oct 15, 2026 at 2:09 PM Heikki Linnakangas <[email protected]> wrote:
> >> 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.
>
> These functions are really complex, they work quite differently for
> relation files and others. The responsibility for verifying the
> checksums is supposedly in read_file_data_into_buffer() -- but the
> caller still needs to worry about page boundaries and short reads and
> warn about checksum failure on short read. That's not new with your
> patches, but it's getting worse.
>
> I think this all becomes more clear if we separate the code to handle
> relation files and other files. See attached. The first patch splits
> sendFile() into two: sendRelFile() for relation files, and sendFile()
> for all other files. That duplicates some code, but makes it more
> readable IMHO. sendFile() can call basebackup_read_file() directly, and
> read_[rel]file_data_into_buffer() is only be used for relation files,
> which makes the 'is_relfile' argument unnecessary.
>
> The second patch changes read_relfile_data_into_buffer() to always work
> with full BLCKSZ blocks. That moves the handling of partial reads fully
> inside read_relfile_data_into_buffer(), making the caller simpler.
>
> These two patches are on top of your two patches, because I developed
> them that way, but it would probably make sense to squash them a little.
Hi Heikki,
Wow, thanks, I was afraid to even touch all of that, but yes it makes it
easier to follow. I've stressed it and I've got two test failures [1] due to
that 0003 ("split sendFile() and sendRelFile()"):
- pg_verifybackup/008_untar, ERROR: could not read file "./junk": Bad address
- pg_verifybackup/010_client_untar , could not read COPY data + crash +
"could not opendir"
In both cases it was due to 640kB "junk" non-relation file being read into
256kB buffer and then blowing up with e.g. with EFAULT due pread().
Then I've tried to break the basebackup in parallel with traffic for quite
some time, but I couldn't :) That was with pgbench -s 100 (~2.2GB relation,
so 2 segments), parallel concurrent loops of VACUUMS/TRUNCATE,
-DSIMULATE_SHORT_READ, and the end hash of table was gathered. Then real
restores worked, hash of the table matched and amcheck did not complain.
I cannot think of anything else.
As for code itself:
- one comment was not updated
- I've noticed we got rid of ereport("could not verify checksum in file")
and started to wonder if that's a good thing. Even with the retries one
of the cases is gone now (transistent reads), but I suspect there could
be sick situations where relfilesize % BLCKSZ != 0 (something odd
happened; try echo "123" >> 16384.100, just static file/fs corrupted). It
looks like earlier we would ship that garbage and with this we seem to be
just shiping zeros instead. I'm not after those zeros or data (WAL should
fix that), but the fact that there is such file in such state in first
place (pg_basebackup is also used as healthy DB verification kinda of thing,
it's often the only thing people use to verify readability of their DB
along with checksums-crosscheck). IMHO we should raise more attention to
such scenarios, if for some reason on some fs/platform it is possible to
end up with such traiing bytes I would sleep safer if user got WARNING,
so I've added another patch for Your's consideration.
Other than that, the CI seem to be happy about this v0005 implementation
(GHA does not have capactiy for MacOS runs for the 2nd day in row).
I'm thinking of squashing all of this into one bigger thing (it's kinda
pointless to have commit just for changing the #define SINK_BUFFER_SIZE?)
or do You want to have it refactor first and then just adjust #define?
-J.
[1] - meson test -C build -q --print-errorlogs --suite setup \
--suite postgresql:pg_basebackup \
--suite postgresql:pg_verifybackup \
--suite postgresql:pg_combinebackup
[2] -
https://github.com/jakubwartakEDB/postgres-public-ci/actions/runs/42016809106
From 3baf0464384cb0ea645cc0a140547529c2903d03 Mon Sep 17 00:00:00 2001
From: Jakub Wartak <[email protected]>
Date: Wed, 7 Oct 2026 10:36:21 +0200
Subject: [PATCH v0005_05112026 1/7] 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
From f9f8e18f45836183a88cccdae3ac0970e9a51703 Mon Sep 17 00:00:00 2001
From: Jakub Wartak <[email protected]>
Date: Thu, 5 Nov 2026 09:24:50 +0100
Subject: [PATCH v0005_05112026 5/7] fix buffer size in sendFile() for non-rel
files
---
src/backend/backup/basebackup.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c
index 86cd118a33b..ea2fedecadf 100644
--- a/src/backend/backup/basebackup.c
+++ b/src/backend/backup/basebackup.c
@@ -1641,8 +1641,9 @@ sendFile(bbsink *sink, const char *readfilename, const char *tarfilename,
* Read as many bytes as will fit in the buffer, or however many
* are left to read, whichever is less.
*/
- cnt = basebackup_read_file(fd, sink->bbs_buffer, remaining, bytes_done,
- readfilename);
+ cnt = basebackup_read_file(fd, sink->bbs_buffer,
+ Min(sink->bbs_buffer_length, remaining),
+ bytes_done, readfilename);
Assert(cnt <= remaining);
/*
--
2.43.0
From 83515c12f65c230e5b2db0b2163ca143969bb850 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <[email protected]>
Date: Thu, 5 Nov 2026 00:20:38 +0200
Subject: [PATCH v0005_05112026 4/7] Work block by block
---
src/backend/backup/basebackup.c | 233 ++++++++++++++------------------
1 file changed, 99 insertions(+), 134 deletions(-)
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c
index 90fed53114b..86cd118a33b 100644
--- a/src/backend/backup/basebackup.c
+++ b/src/backend/backup/basebackup.c
@@ -106,12 +106,12 @@ static bool sendRelFile(bbsink *sink, const char *readfilename, const char *tarf
static bool sendFile(bbsink *sink, const char *readfilename, const char *tarfilename,
bool missing_ok, Oid spcoid,
backup_manifest_info *manifest);
-static off_t read_relfile_data_into_buffer(bbsink *sink,
- const char *readfilename, int fd,
- off_t offset, size_t length,
- BlockNumber blkno,
- bool verify_checksum,
- int *checksum_failures);
+static BlockNumber read_relfile_data_into_buffer(bbsink *sink,
+ const char *readfilename,
+ int fd, unsigned segno,
+ BlockNumber start_blk, BlockNumber nblocks,
+ bool verify_checksum,
+ int *checksum_failures);
static void push_to_sink(bbsink *sink, pg_checksum_context *checksum_ctx,
size_t *bytes_done, void *data, size_t length);
static bool backup_checksums_verifiable(XLogRecPtr start_lsn);
@@ -1634,7 +1634,7 @@ sendFile(bbsink *sink, const char *readfilename, const char *tarfilename,
* If we've read the required number of bytes, then it's time to
* stop.
*/
- if (bytes_done >= filestat.st_size)
+ if (remaining == 0)
break;
/*
@@ -1643,6 +1643,7 @@ sendFile(bbsink *sink, const char *readfilename, const char *tarfilename,
*/
cnt = basebackup_read_file(fd, sink->bbs_buffer, remaining, bytes_done,
readfilename);
+ Assert(cnt <= remaining);
/*
* If we hit end-of-file, a concurrent truncation must have occurred.
@@ -1721,13 +1722,13 @@ sendRelFile(bbsink *sink, const char *readfilename, const char *tarfilename,
BlockNumber *incremental_blocks, unsigned truncation_block_length)
{
int fd;
- BlockNumber blkno = 0;
int checksum_failures = 0;
- off_t cnt;
- pgoff_t bytes_done = 0;
- bool verify_checksum = false;
+ size_t bytes_done = 0;
+ BlockNumber total_blocks;
+ BlockNumber blocks_done;
+ bool verify_checksum;
pg_checksum_context checksum_ctx;
- int ibindex = 0;
+ int ibindex;
struct stat filestat = *statbuf;
Assert(RelFileNumberIsValid(relfilenumber));
@@ -1819,6 +1820,12 @@ sendRelFile(bbsink *sink, const char *readfilename, const char *tarfilename,
bytes_done += sizeof(num_incremental_blocks);
bytes_done += sizeof(truncation_block_length);
bytes_done += sizeof(BlockNumber) * num_incremental_blocks;
+
+ /*
+ * Make sure incremental files with block data are properly aligned
+ * (header is a multiple of BLCKSZ, blocks are BLCKSZ too).
+ */
+ Assert((num_incremental_blocks == 0) || (bytes_done % BLCKSZ == 0));
}
/*
@@ -1827,37 +1834,34 @@ sendRelFile(bbsink *sink, const char *readfilename, const char *tarfilename,
* for a base backup we can ignore such extended data. It will be restored
* from WAL.
*/
+ total_blocks = filestat.st_size / BLCKSZ;
+ blocks_done = 0;
+ ibindex = 0;
while (1)
{
+ size_t cnt;
+ BlockNumber start_blk;
+ BlockNumber nblocks;
+ BlockNumber blocks_read;
+
/*
* Determine whether we've read all the data that we need, and if not,
* read some more.
*/
if (incremental_blocks == NULL)
{
- size_t remaining = filestat.st_size - bytes_done;
-
/*
- * If we've read the required number of bytes, then it's time to
- * stop.
+ * Read as many blocks as will fit in the buffer, or however many
+ * are left to read, whichever is less. If we've read the
+ * required number of blocks, then it's time to stop.
*/
- if (bytes_done >= filestat.st_size)
+ start_blk = blocks_done;
+ nblocks = total_blocks - blocks_done;
+ if (nblocks == 0)
break;
-
- /*
- * Read as many bytes as will fit in the buffer, or however many
- * are left to read, whichever is less.
- */
- cnt = read_relfile_data_into_buffer(sink, readfilename, fd,
- bytes_done, remaining,
- blkno + segno * RELSEG_SIZE,
- verify_checksum,
- &checksum_failures);
}
else
{
- BlockNumber relative_blkno;
-
/*
* If we've read all the blocks, then it's time to stop.
*/
@@ -1868,76 +1872,30 @@ sendRelFile(bbsink *sink, const char *readfilename, const char *tarfilename,
* Read just one block, whichever one is the next that we're
* supposed to include.
*/
- relative_blkno = incremental_blocks[ibindex++];
- cnt = read_relfile_data_into_buffer(sink, readfilename, fd,
- relative_blkno * BLCKSZ,
- BLCKSZ,
- relative_blkno + segno * RELSEG_SIZE,
- verify_checksum,
- &checksum_failures);
-
- /*
- * read_relfile_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
- * zeroes. WAL replay will fix things up.
- */
- if (cnt < BLCKSZ)
- break;
+ start_blk = incremental_blocks[ibindex++];
+ nblocks = 1;
}
- /*
- * If the amount of data we were able to read was not a multiple of
- * BLCKSZ, we cannot verify checksums, which are block-level.
- */
- if (verify_checksum &&
- 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),
- 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;
- }
+ blocks_read =
+ read_relfile_data_into_buffer(sink, readfilename,
+ fd, segno,
+ start_blk, nblocks,
+ verify_checksum,
+ &checksum_failures);
/*
* If we hit end-of-file, a concurrent truncation must have occurred.
* That's not an error condition, because WAL replay will fix things
* up.
*/
- if (cnt == 0)
+ if (blocks_read == 0)
break;
- /* Update block number and # of bytes done for next loop iteration. */
- blkno += cnt / BLCKSZ;
+ /* Update # of blocks and bytes done for next loop iteration. */
+ cnt = (size_t) blocks_read * BLCKSZ;
+ blocks_done += blocks_read;
bytes_done += cnt;
- /*
- * Make sure incremental files with block data are properly aligned
- * (header is a multiple of BLCKSZ, blocks are BLCKSZ too).
- */
- Assert(!((incremental_blocks != NULL && num_incremental_blocks > 0) &&
- (bytes_done % BLCKSZ != 0)));
-
/* Archive the data we just read. */
bbsink_archive_contents(sink, cnt);
@@ -1992,66 +1950,75 @@ sendRelFile(bbsink *sink, const char *readfilename, const char *tarfilename,
}
/*
- * Read some more data from the file into the bbsink's buffer, verifying
- * checksums as required.
+ * Read some more data from a relation file into the bbsink's buffer,
+ * verifying checksums as required.
*
- * '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 we hit EOF, which means
- * that the underlying file has been truncated. The return value is the
- * number of bytes actually read.
+ * 'start_blk' is the block number from the beginning of the file from which
+ * we should begin to read, and 'nblocks' is the number of blocks 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 we hit
+ * EOF, which means that the underlying file has been truncated. The return
+ * value is the number of blocks actually read.
*
- * 'blkno' is the block number of the first page in the bbsink's buffer
- * relative to the start of the relation.
+ * 'segno' is the relation segment number, needed to calculate the checksums
+ * correctly.
*
* '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
* *checksum_failures and issue warnings as appropriate.
*/
-static off_t
-read_relfile_data_into_buffer(bbsink *sink, const char *readfilename, int fd,
- off_t offset, size_t length, BlockNumber blkno,
+static BlockNumber
+read_relfile_data_into_buffer(bbsink *sink, const char *readfilename,
+ int fd, unsigned segno,
+ BlockNumber start_blk, BlockNumber nblocks,
bool verify_checksum, int *checksum_failures)
{
- off_t cnt;
- size_t requested = Min(sink->bbs_buffer_length, length);
+ BlockNumber blkno;
+ BlockNumber blocks_read;
+ ssize_t cnt;
int i;
char *page;
+ Assert(sink->bbs_buffer_length >= BLCKSZ);
+ nblocks = Min(sink->bbs_buffer_length / BLCKSZ, nblocks);
+
/* Try to read some more data. */
- cnt = basebackup_read_file(fd, sink->bbs_buffer, requested,
- offset, readfilename);
+ cnt = basebackup_read_file(fd, sink->bbs_buffer,
+ (size_t) nblocks * BLCKSZ,
+ (off_t) start_blk * BLCKSZ,
+ 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.
+ * 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
+ * function is only used for relation files, but because the OS might copy
+ * data in smaller chunks (e.g. OS page size), 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 zeroes. WAL
+ * replay will fix things up.
*/
- if (cnt < requested && (cnt % BLCKSZ) != 0)
- cnt -= cnt % BLCKSZ;
+ blocks_read = (BlockNumber) (cnt / BLCKSZ);
- /* Can't verify checksums if read length is not a multiple of BLCKSZ. */
- if (!verify_checksum || (cnt % BLCKSZ) != 0)
- return cnt;
+ /*
+ * The data checksum state can change at any point, so we need to
+ * re-check.
+ */
+ if (!verify_checksum || !backup_checksums_verifiable(sink->bbs_state->startptr))
+ return blocks_read;
/* Verify checksum for each block. */
- for (i = 0; i < cnt / BLCKSZ; i++)
+ blkno = start_blk + segno * RELSEG_SIZE;
+ for (i = 0; i < blocks_read; i++)
{
- int reread_cnt;
+ ssize_t reread_cnt;
uint16 expected_checksum;
- /*
- * The data checksum state can change at any point, so we need to
- * re-check before each page.
- */
- if (!backup_checksums_verifiable(sink->bbs_state->startptr))
- return cnt;
-
page = sink->bbs_buffer + BLCKSZ * i;
/* If the page is OK, go on to the next one. */
@@ -2079,24 +2046,22 @@ read_relfile_data_into_buffer(bbsink *sink, const char *readfilename, int fd,
* again.
*/
if (!backup_checksums_verifiable(sink->bbs_state->startptr))
- return cnt;
+ break;
reread_cnt =
basebackup_read_file(fd, sink->bbs_buffer + BLCKSZ * i,
- BLCKSZ, offset + BLCKSZ * i,
+ BLCKSZ, (off_t) (start_blk + i) * BLCKSZ,
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.
+ * occurred, so reduce 'blocks_read' 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).
+ * In the unlikely situation where reread ended up getting only
+ * part of the block, we classify this as truncation too.
*/
- cnt = BLCKSZ * i;
+ blocks_read = i;
break;
}
@@ -2121,7 +2086,7 @@ read_relfile_data_into_buffer(bbsink *sink, const char *readfilename, int fd,
"be reported", readfilename)));
}
- return cnt;
+ return blocks_read;
}
/*
--
2.43.0
From 95f9b00bdef645d89638c2a3994f423245d53825 Mon Sep 17 00:00:00 2001
From: Jakub Wartak <[email protected]>
Date: Thu, 29 Oct 2026 09:34:50 +0100
Subject: [PATCH v0005_05112026 2/7] 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 a0731fb26723f8df81c9ac6b51dd7d668f7d6552 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <[email protected]>
Date: Thu, 5 Nov 2026 00:39:38 +0200
Subject: [PATCH v0005_05112026 3/7] split sendFile() and sendRelFile()
---
src/backend/backup/basebackup.c | 261 ++++++++++++++++++++++----------
1 file changed, 181 insertions(+), 80 deletions(-)
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c
index 25914c5b1d3..90fed53114b 100644
--- a/src/backend/backup/basebackup.c
+++ b/src/backend/backup/basebackup.c
@@ -95,21 +95,23 @@ static int64 sendDir(bbsink *sink, const char *path, int basepathlen, bool sizeo
List *tablespaces, bool sendtblspclinks,
backup_manifest_info *manifest, Oid spcoid,
IncrementalBackupInfo *ib);
+static bool sendRelFile(bbsink *sink, const char *readfilename, const char *tarfilename,
+ const struct stat *statbuf, bool missing_ok,
+ Oid dboid, Oid spcoid, RelFileNumber relfilenumber,
+ unsigned segno,
+ backup_manifest_info *manifest,
+ unsigned num_incremental_blocks,
+ BlockNumber *incremental_blocks,
+ unsigned truncation_block_length);
static bool sendFile(bbsink *sink, const char *readfilename, const char *tarfilename,
- const struct stat *statbuf, bool missing_ok,
- Oid dboid, Oid spcoid, RelFileNumber relfilenumber,
- unsigned segno,
- backup_manifest_info *manifest,
- unsigned num_incremental_blocks,
- BlockNumber *incremental_blocks,
- unsigned truncation_block_length);
-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);
+ bool missing_ok, Oid spcoid,
+ backup_manifest_info *manifest);
+static off_t read_relfile_data_into_buffer(bbsink *sink,
+ const char *readfilename, int fd,
+ off_t offset, size_t length,
+ BlockNumber blkno,
+ bool verify_checksum,
+ int *checksum_failures);
static void push_to_sink(bbsink *sink, pg_checksum_context *checksum_ctx,
size_t *bytes_done, void *data, size_t length);
static bool backup_checksums_verifiable(XLogRecPtr start_lsn);
@@ -647,14 +649,7 @@ perform_base_backup(basebackup_options *opt, bbsink *sink,
snprintf(pathbuf, MAXPGPATH, XLOGDIR "/%s", fname);
- if (lstat(pathbuf, &statbuf) != 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not stat file \"%s\": %m", pathbuf)));
-
- sendFile(sink, pathbuf, pathbuf, &statbuf, false,
- InvalidOid, InvalidOid, InvalidRelFileNumber, 0,
- &manifest, 0, NULL, 0);
+ sendFile(sink, pathbuf, pathbuf, false, InvalidOid, &manifest);
/* unconditionally mark file as archived */
StatusFilePath(pathbuf, fname, ".done");
@@ -1545,12 +1540,18 @@ sendDir(bbsink *sink, const char *path, int basepathlen, bool sizeonly,
}
if (!sizeonly)
- sent = sendFile(sink, pathbuf, tarfilename, &statbuf,
- true, dboid, spcoid,
- relfilenumber, segno, manifest,
- num_blocks_required,
- method == BACK_UP_FILE_INCREMENTALLY ? relative_block_numbers : NULL,
- truncation_block_length);
+ {
+ if (isRelationFile)
+ sent = sendRelFile(sink, pathbuf, tarfilename, &statbuf,
+ true, dboid, spcoid,
+ relfilenumber, segno, manifest,
+ num_blocks_required,
+ method == BACK_UP_FILE_INCREMENTALLY ? relative_block_numbers : NULL,
+ truncation_block_length);
+ else
+ sent = sendFile(sink, pathbuf, tarfilename, true,
+ spcoid, manifest);
+ }
if (sent || sizeonly)
{
@@ -1577,10 +1578,129 @@ sendDir(bbsink *sink, const char *path, int basepathlen, bool sizeonly,
}
/*
- * Given the member, write the TAR header & send the file.
+ * Write the TAR header & send the file.
+ *
+ * If 'missing_ok' is true, will not throw an error if the file is not found.
+ *
+ * Returns true if the file was successfully sent, false if 'missing_ok',
+ * and the file did not exist.
+ */
+static bool
+sendFile(bbsink *sink, const char *readfilename, const char *tarfilename,
+ bool missing_ok, Oid spcoid, backup_manifest_info *manifest)
+{
+ int fd;
+ off_t cnt;
+ pgoff_t bytes_done = 0;
+ pg_checksum_context checksum_ctx;
+ struct stat filestat;
+
+ if (pg_checksum_init(&checksum_ctx, manifest->checksum_type) < 0)
+ elog(ERROR, "could not initialize checksum of file \"%s\"",
+ readfilename);
+
+ fd = OpenTransientFile(readfilename, O_RDONLY | PG_BINARY);
+ if (fd < 0)
+ {
+ if (errno == ENOENT && missing_ok)
+ return false;
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not open file \"%s\": %m", readfilename)));
+ }
+
+ /*
+ * Note: Non-relation files may be replaced by a rename(), so even if the
+ * caller had stat data, we should not use it here.
+ */
+ if (fstat(fd, &filestat) != 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not stat file \"%s\": %m", readfilename)));
+
+ _tarWriteHeader(sink, tarfilename, NULL, &filestat, false);
+
+ /*
+ * Loop until we read the amount of data the caller told us to expect. The
+ * file could be longer, if it was extended while we were sending it, but
+ * for a base backup we can ignore such extended data. It will be restored
+ * from WAL.
+ */
+ while (1)
+ {
+ size_t remaining = filestat.st_size - bytes_done;
+
+ /*
+ * If we've read the required number of bytes, then it's time to
+ * stop.
+ */
+ if (bytes_done >= filestat.st_size)
+ break;
+
+ /*
+ * Read as many bytes as will fit in the buffer, or however many
+ * are left to read, whichever is less.
+ */
+ cnt = basebackup_read_file(fd, sink->bbs_buffer, remaining, bytes_done,
+ readfilename);
+
+ /*
+ * If we hit end-of-file, a concurrent truncation must have occurred.
+ * That's not an error condition, because WAL replay will fix things
+ * up.
+ */
+ if (cnt == 0)
+ break;
+
+ /* Update # of bytes done for next loop iteration. */
+ bytes_done += cnt;
+
+ /* Archive the data we just read. */
+ bbsink_archive_contents(sink, cnt);
+
+ /* Also feed it to the checksum machinery. */
+ if (pg_checksum_update(&checksum_ctx,
+ (uint8 *) sink->bbs_buffer, cnt) < 0)
+ elog(ERROR, "could not update checksum of base backup");
+ }
+
+ /* If the file was truncated while we were sending it, pad it with zeros */
+ while (bytes_done < filestat.st_size)
+ {
+ size_t remaining = filestat.st_size - bytes_done;
+ size_t nbytes = Min(sink->bbs_buffer_length, remaining);
+
+ MemSet(sink->bbs_buffer, 0, nbytes);
+ if (pg_checksum_update(&checksum_ctx,
+ (uint8 *) sink->bbs_buffer,
+ nbytes) < 0)
+ elog(ERROR, "could not update checksum of base backup");
+ bbsink_archive_contents(sink, nbytes);
+ bytes_done += nbytes;
+ }
+
+ /*
+ * Pad to a block boundary, per tar format requirements. (This small piece
+ * of data is probably not worth throttling, and is not checksummed
+ * because it's not actually part of the file.)
+ */
+ _tarWritePadding(sink, bytes_done);
+
+ CloseTransientFile(fd);
+
+ AddFileToBackupManifest(manifest, spcoid, tarfilename, filestat.st_size,
+ (pg_time_t) filestat.st_mtime, &checksum_ctx);
+
+ return true;
+}
+
+/*
+ * Write the TAR header & send a relation file.
*
* If 'missing_ok' is true, will not throw an error if the file is not found.
*
+ * The file is read block by block, verifying checksums if necessary.
+ *
* If dboid is anything other than InvalidOid then any checksum failures
* detected will get reported to the cumulative stats system.
*
@@ -1590,18 +1710,15 @@ sendDir(bbsink *sink, const char *path, int basepathlen, bool sizeonly,
* If the whole file is to be sent, then incremental_blocks should be NULL,
* and num_incremental_blocks can have any value, as it will be ignored.
*
- * For non-relation files, the stat data of the opened file is used, not
- * 'statbuf'.
- *
* Returns true if the file was successfully sent, false if 'missing_ok',
* and the file did not exist.
*/
static bool
-sendFile(bbsink *sink, const char *readfilename, const char *tarfilename,
- const struct stat *statbuf, bool missing_ok, Oid dboid, Oid spcoid,
- RelFileNumber relfilenumber, unsigned segno,
- backup_manifest_info *manifest, unsigned num_incremental_blocks,
- BlockNumber *incremental_blocks, unsigned truncation_block_length)
+sendRelFile(bbsink *sink, const char *readfilename, const char *tarfilename,
+ const struct stat *statbuf, bool missing_ok, Oid dboid, Oid spcoid,
+ RelFileNumber relfilenumber, unsigned segno,
+ backup_manifest_info *manifest, unsigned num_incremental_blocks,
+ BlockNumber *incremental_blocks, unsigned truncation_block_length)
{
int fd;
BlockNumber blkno = 0;
@@ -1611,7 +1728,9 @@ sendFile(bbsink *sink, const char *readfilename, const char *tarfilename,
bool verify_checksum = false;
pg_checksum_context checksum_ctx;
int ibindex = 0;
- struct stat filestat;
+ struct stat filestat = *statbuf;
+
+ Assert(RelFileNumberIsValid(relfilenumber));
if (pg_checksum_init(&checksum_ctx, manifest->checksum_type) < 0)
elog(ERROR, "could not initialize checksum of file \"%s\"",
@@ -1627,17 +1746,6 @@ sendFile(bbsink *sink, const char *readfilename, const char *tarfilename,
errmsg("could not open file \"%s\": %m", readfilename)));
}
- /*
- * Non-relation files may be replaced by a rename(), making irrelevant the
- * caller's stat data. Reuse fstat() on the opened file.
- */
- filestat = *statbuf;
-
- if (!RelFileNumberIsValid(relfilenumber) && fstat(fd, &filestat) != 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not stat file \"%s\": %m", readfilename)));
-
_tarWriteHeader(sink, tarfilename, NULL, &filestat, false);
/*
@@ -1647,15 +1755,15 @@ sendFile(bbsink *sink, const char *readfilename, const char *tarfilename,
Assert((sink->bbs_buffer_length % BLCKSZ) == 0);
/*
- * Verify checksums unless the client requested otherwise, but only for
- * relation files, and only while checksums have been continuously enabled
- * since the checkpoint this backup started from. Checksums can still be
- * disabled while the backup runs, thus we check at each point where we
- * could be validating a checksum.
+ * Verify checksums unless the client requested otherwise, but only while
+ * checksums have been continuously enabled since the checkpoint this
+ * backup started from. Checksums can still be disabled while the backup
+ * runs, thus we check at each point where we could be validating a
+ * checksum.
*/
- if (!noverify_checksums && RelFileNumberIsValid(relfilenumber) &&
- backup_checksums_verifiable(sink->bbs_state->startptr))
- verify_checksum = true;
+ verify_checksum = true;
+ if (noverify_checksums || !backup_checksums_verifiable(sink->bbs_state->startptr))
+ verify_checksum = false;
/*
* If we're sending an incremental file, write the file header.
@@ -1740,12 +1848,11 @@ sendFile(bbsink *sink, const char *readfilename, const char *tarfilename,
* Read as many bytes as will fit in the buffer, or however many
* are left to read, whichever is less.
*/
- cnt = read_file_data_into_buffer(sink, readfilename, fd,
- bytes_done, remaining,
- blkno + segno * RELSEG_SIZE,
- RelFileNumberIsValid(relfilenumber),
- verify_checksum,
- &checksum_failures);
+ cnt = read_relfile_data_into_buffer(sink, readfilename, fd,
+ bytes_done, remaining,
+ blkno + segno * RELSEG_SIZE,
+ verify_checksum,
+ &checksum_failures);
}
else
{
@@ -1762,16 +1869,15 @@ sendFile(bbsink *sink, const char *readfilename, const char *tarfilename,
* supposed to include.
*/
relative_blkno = incremental_blocks[ibindex++];
- cnt = read_file_data_into_buffer(sink, readfilename, fd,
- relative_blkno * BLCKSZ,
- BLCKSZ,
- relative_blkno + segno * RELSEG_SIZE,
- true,
- verify_checksum,
- &checksum_failures);
+ cnt = read_relfile_data_into_buffer(sink, readfilename, fd,
+ relative_blkno * BLCKSZ,
+ BLCKSZ,
+ relative_blkno + segno * RELSEG_SIZE,
+ verify_checksum,
+ &checksum_failures);
/*
- * read_file_data_into_buffer() retries short reads, so if we got
+ * read_relfile_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
@@ -1899,18 +2005,15 @@ 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
* *checksum_failures and issue warnings as appropriate.
*/
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)
+read_relfile_data_into_buffer(bbsink *sink, const char *readfilename, int fd,
+ off_t offset, size_t length, BlockNumber blkno,
+ bool verify_checksum, int *checksum_failures)
{
off_t cnt;
size_t requested = Min(sink->bbs_buffer_length, length);
@@ -1928,10 +2031,8 @@ read_file_data_into_buffer(bbsink *sink, const char *readfilename, int fd,
* 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)
+ if (cnt < requested && (cnt % BLCKSZ) != 0)
cnt -= cnt % BLCKSZ;
/* Can't verify checksums if read length is not a multiple of BLCKSZ. */
--
2.43.0
From 4bb45ac3664f0339875ad091a822b811523bbd41 Mon Sep 17 00:00:00 2001
From: Jakub Wartak <[email protected]>
Date: Thu, 5 Nov 2026 12:06:57 +0100
Subject: [PATCH v0005_05112026 7/7] add warning just in case
---
src/backend/backup/basebackup.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c
index cb7b968df78..3466750822d 100644
--- a/src/backend/backup/basebackup.c
+++ b/src/backend/backup/basebackup.c
@@ -1838,6 +1838,16 @@ sendRelFile(bbsink *sink, const char *readfilename, const char *tarfilename,
total_blocks = filestat.st_size / BLCKSZ;
blocks_done = 0;
ibindex = 0;
+
+ /*
+ * We read only whole blocks and that is harmless, but segment size not being
+ * multiple of BLCKSZ is unusual sign.
+ */
+ if (incremental_blocks == NULL && filestat.st_size % BLCKSZ != 0)
+ ereport(WARNING,
+ (errmsg("file \"%s\" has size %lld which is not a multiple of block size %d",
+ readfilename, (long long) filestat.st_size, BLCKSZ)));
+
while (1)
{
size_t cnt;
--
2.43.0
From 6f2ce467c57944d1693d6738dd07f28c4630b53f Mon Sep 17 00:00:00 2001
From: Jakub Wartak <[email protected]>
Date: Thu, 5 Nov 2026 10:53:35 +0100
Subject: [PATCH v0005_05112026 6/7] fix read_relfile_data_into_buffer()
comment
---
src/backend/backup/basebackup.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c
index ea2fedecadf..cb7b968df78 100644
--- a/src/backend/backup/basebackup.c
+++ b/src/backend/backup/basebackup.c
@@ -1991,7 +1991,7 @@ read_relfile_data_into_buffer(bbsink *sink, const char *readfilename,
readfilename);
/*
- * read_file_data_into_buffer() retries short reads, so if we got fewer
+ * basebackup_read_file() retries short reads, so if we got fewer
* bytes than requested we must have hit EOF, which means that the
* relation is being truncated.
*
--
2.43.0
