| From: | Jakub Wartak <jakub(dot)wartak(at)enterprisedb(dot)com> |
|---|---|
| To: | Heikki Linnakangas <hlinnaka(at)iki(dot)fi> |
| Cc: | Gustavo William <gustavowilliam0805(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: enhancing pg_basebackup speeds up to ~23Gbps (small fixes + io_uring/Direct I/O) |
| Date: | 2026-10-06 13:10:38 |
| Message-ID: | CAKZiRmyVMbQHWLC9aLCRi4_a+Wq-HGpm-6vUthJatypcNrAYPQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Mon, Oct 5, 2026 at 6:24 PM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> I started to look at these, starting from this patch:
Hi Heikki, thanks for taking a look !
> > 0004 increases SINK_BUFFER_LENGTH from its current 32 kB. Profiling the
> > server side shows a huge number of small pread() calls coming from
> > basebackup_read_file(), which is surprising given that elsewhere in the tree
> > (backend/storage/buffer/README) we already mention that a 256 kB ring for seq.
> > scans scans is used because it fits comfortably in L2 cache. On my laptop,
> > with no SSL, no checksum generation or verification, a hot filesystem cache,
> > and a client blackhole to eliminate client I/O, a 10 GB backup over loopback
> > runs at:
> > 3.1 GB/s with the 32 kB buffer,
> > 5.1 GB/s at 128 kB,
> > 5.5 GB/s at 256 kB,
> > 6.0 GB/s at 1 MB
> > (those are average of five runs).
>
> Cool
>
> > The win comes from letting pread()
> > swallow much larger chunks of each segment in one go: per-core L2 caches are
> > 1-2 MB even on laptops these days, syscalls have become more expensive since
> > the Spectre/Meltdown mitigations, and the kernel's default readahead is
> > already in the 128-512 kB range, so there's little reason to trickle the data
> > through 32 kB at a time.
>
> While we're at it, we should probably allocate the buffer with
> palloc_aligned(PG_IO_ALIGN_SIZE); a pread() into an aligned buffer
> should be a little faster.
I've attached patchset with 0004 modified that has this adjustement, but I
haven't quantified the gain for this alone though.
BTW: I've added discussion link and other tags, fixed some minor types in to all
commitmsgs too.
> I wonder about this in read_file_data_into_buffer():
>
> > cnt = basebackup_read_file(fd, sink->bbs_buffer,
> > Min(sink->bbs_buffer_length, length),
> > offset, readfilename, true);
> >
> > /* Can't verify checksums if read length is not a multiple of BLCKSZ. */
> > if (!verify_checksum || (cnt % BLCKSZ) != 0)
> > return cnt;
>
> It doesn't seem right to assume that the count must be a multiple of
> BLCKSZ, especially if we use a larger buffer. Should we do something
> about that?
Your question has opened a whole can of worms on my side :(, but isn't this
code saying two things at once?
- it's only for calculating checksums only for BLCKSZ-sized blocks of
relations (I could imagine some user-placed file, or ENOSPC failure)
- we expect pread/pwrite() atomicity between two concurrent backends (one
backend doing pwrite()/fallocate() extends or truncation while the 2nd
is doing basebackup?)
bbsink_begin_backup(buffer_length=SINK_BUFFER_LENGTH) will set sink->
bbs_buffer_length to that SINK.. value and even there verify too:
Assert((sink->bbs_buffer_length % BLCKSZ) == 0);
sendFile() has the same (+ there's more occurences of this)
/*
* Checksums are verified in multiples of BLCKSZ, so the buffer length
* should be a multiple of the block size as well.
*/
Assert((sink->bbs_buffer_length % BLCKSZ) == 0);
further more sendFile() has this too above the "if (cnt < BLCKSZ)"
/*
* 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.
*
* 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.
*/
and below that, there is ereport() that says just checksums couldn't be
verified. I mean in 0004 we are just growing and so it assumes we shouldn't
get any problems with this code. It looks like it is written while expecting
proper pread()/pwrite() atomicity when extending for up to 32kb, or am I
missing something?
On 6.17.x and 6.14.x x86_64 ext4 I've got no issues with such atomicity for
buffered preads while __appending__ stuff (using pwrite() to extend) and pread()
got correct stuff or premature EOF - that was for both 8kB and 256kB. Of
course I'm able to get torn-pages (only on 6.17.x - sic!) while doing buffered
OVERWRITES (so yea, 8kb split into 4kB + old 4KB -- checksum wouldn't match]).
Anyway as this topic touches the lack of pread()/pwrite() atomicity, but:
a) for torn-pages we are already protected there (see nearby comment for
reread_cnt , so we re-read)
b) for truncation too we are protected too
c) and it seems there is no issue for extending/appending
I've done some extensive pgbench read-write tests with pg_basebackup back then
and that such stuff didn't happen, perhaps buildfarm would tell more(?) My
only worry is that there could be some silent kernel change (in the past)
that would allow too see incompletle data by pread() for pwrite(), but if
that is not happening on 8kB/32kB (which is already over page-size), why
would that be happening for higher values?
BTW: previous such discussions [1][2][3][4] all seem to point that lack of
similar atomicity guaranteees or seem to be kernel bugs or some bizzare platform
(see e.g. "pwrite/pread is non-atomic" in [2]) or at least deficit in fs
(while XFS being always safer [5]).
Still, even if if we just read miss some stale data due to pread() woes (and
skip checksums there) the data should be recoverable from WAL.
-J.
[1] - https://www.postgresql.org/message-id/20220116071210.GA735692%40rfd.leadboat.com
[2] - https://www.postgresql.org/message-id/20220116210241.GC756210%40rfd.leadboat.com
[3] - https://www.postgresql.org/message-id/CA%2BhUKGJ98%3DMOjDCnMAC5gSpkzrrey%3DO%2BaEQJ1OY03C%3DcVtiwkA%40mail.gmail.com
[4] - https://www.postgresql.org/message-id/CA%2BhUKG%2B19bZKidSiWmMsDmgUVe%3D_rr0m57LfR%2BnAbWprVDd_cw%40mail.gmail.com
[5] - https://www.postgresql.org/message-id/CA%2BhUKGJsT8G_YyjUzMZaJTWyua6PbwC3TAUMv_kDS0F0vzr2Pw%40mail.gmail.com
- after "As for ext4, we've detected and debugged clues.."
| Attachment | Content-Type | Size |
|---|---|---|
| v06102026-0002-pg_basebackup-add-new-client-blackhole-ben.patch | text/x-patch | 16.6 KB |
| v06102026-0001-pg_basebackup-rename-the-blackhole-backup-.patch | text/x-patch | 5.3 KB |
| v06102026-0005-basebackup-issue-posix_fadvise-for-more-ef.patch | text/x-patch | 7.0 KB |
| v06102026-0003-pg_basebackup-report-average-data-transfer.patch | text/x-patch | 3.1 KB |
| v06102026-0004-basebackup-bump-SINK_BUFFER_LENGTH-to-256k.patch | text/x-patch | 2.0 KB |
| v06102026-0007-pg_basebackup-preallocate-extracted-files-.patch | text/x-patch | 2.9 KB |
| v06102026-0008-libpq-pg_basebackup-add-PQgetCopyDataInter.patch | text/x-patch | 7.3 KB |
| v06102026-0009-pg_basebackup-add-support-for-Direct-I-O-a.patch | text/x-patch | 22.7 KB |
| v06102026-0010-pg_basebackup-preallocate-DIO-writes-also-.patch | text/x-patch | 6.5 KB |
| v06102026-0006-pg_basebackup-eliminate-usage-of-libc-to-c.patch | text/x-patch | 6.7 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Fujii Masao | 2026-10-06 13:30:21 | Re: Incremental backups report progress as if they were full backups |
| Previous Message | Andrey Borodin | 2026-10-06 13:09:38 | Re: Compression of bigger WAL records |