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

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.

- Heikki