Horton

Re: [PATCH v4] parallel pg_restore: avoid disk seeks when jumping short distance forward

Dimitrios Apostolou <jimis@gmx.net>

View thread View on postgr.es
From: Dimitrios Apostolou <jimis@gmx.net>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Chao Li <li.evan.chao@gmail.com>, Nathan Bossart <nathandbossart@gmail.com>, Thomas Munro <thomas.munro@gmail.com>, pgsql-hackers@lists.postgresql.org
Date: 2025-10-21T14:16:26Z
Lists: pgsql-hackers

Attachments

On Tuesday 2025-10-21 00:15, Tom Lane wrote:

>> So for me, the proposed patch actually makes it 2X slower.
>
> I went and tried this same test case on a 2024 Mac Mini M4 Pro.
> Cutting to the chase:
>
> HEAD:
>
> $ time pg_restore -f /dev/null -t zedtable bench10000.dump
>
> real    1m26.525s
> user    0m0.364s
> sys     0m6.806s
>
> Patched:
>
> $ time pg_restore -f /dev/null -t zedtable bench10000.dump
>
> real    0m15.419s
> user    0m0.279s
> sys     0m8.224s
>
> So on this hardware it *does* win (although maybe things would
> be different for a parallel restore).  The patched pg_restore
> takes just about the same amount of time as "cat", and iostat
> shows both of them reaching a bit more than 6GB/s read speed.
>
> My feeling at this point is that we'd probably drop the block
> size test as irrelevant, and instead simply ignore ctx->hasSeek
> within this loop if we think we're on a platform where that's
> the right thing.  But how do we figure that out?
>
> Not sure where we go from here, but clearly a bunch of research
> is going to be needed to decide whether this is committable.

pg_dump files from before your latest fix still exist, and they possibly 
contain block header every 30 bytes (or however wide is the table rows). 
A patch in pg_restore would vastly improve this use case.

May I suggest the attached patch, which replaces fseeko() with fread() 
if the distance is 32KB or less? Sounds rather improbable that this 
would make things worse, but maybe it's possible to generate a dump file 
with 32KB wide rows, and try restoring on various hardware?

If this too is controversial, then we can reduce the number to 4KB. This 
is the buffering that glibc does internally. By using the same in the 
given patch, we avoid all the lseek(same-offset) repetitions between the 
4K reads. This should be a strict gain, with no downsides.



Dimitris

Commits

  1. Avoid short seeks in pg_restore.

  2. Don't rely on zlib's gzgetc() macro.

  3. Add more TAP test coverage for pg_dump.

  4. Split 002_pg_dump.pl into two test files.

  5. Align the data block sizes of pg_dump's various compression modes.

  6. Fix serious performance problems in LZ4Stream_read_internal.

  7. Fix poor buffering logic in pg_dump's lz4 and zstd compression code.

  8. Fix issue with reading zero bytes in Gzip_read.

  9. Restore test coverage of LZ4Stream_gets().