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

On Thu, Oct 15, 2026 at 2:09 PM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>
> On 07/10/2026 12:55, Jakub Wartak wrote:
> > On Tue, Oct 6, 2026 at 5:42 PM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> 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.