Re: enhancing pg_basebackup speeds up to ~23Gbps (small fixes + io_uring/Direct I/O)

On Wed, Nov 4, 2026 at 11:45 PM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>
> On 29/10/2026 16:16, Jakub Wartak wrote:
> > On Thu, Oct 15, 2026 at 2:09 PM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> 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