Horton

Thread

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().

  1. [PATCH v1] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Dimitrios Apostolou <jimis@gmx.net> — 2025-03-29T00:46:30Z

    Hello list,

I'm submitting a patch for improving an almost 1h long pause at the start
of parallel pg_restore of a big archive. Related discussion has taken
place at pgsql-performance mailing list at:

https://www.postgresql.org/message-id/flat/6bd16bdb-aa5e-0512-739d-b84100596035%40gmx.net

I think I explain it rather well in the commit message, so I paste it
inline:


Improve the performance of parallel pg_restore (-j) from a custom format
pg_dump archive that does not include data offsets - typically happening
when pg_dump has generated it by writing to stdout instead of a file.

In this case pg_restore workers manifest constant looping of reading
small sizes (4KB) and seeking forward small lenths (around 10KB for a
compressed archive):

read(4, "..."..., 4096) = 4096
lseek(4, 55544369152, SEEK_SET)         = 55544369152
read(4, "..."..., 4096) = 4096
lseek(4, 55544381440, SEEK_SET)         = 55544381440
read(4, "..."..., 4096) = 4096
lseek(4, 55544397824, SEEK_SET)         = 55544397824
read(4, "..."..., 4096) = 4096
lseek(4, 55544414208, SEEK_SET)         = 55544414208
read(4, "..."..., 4096) = 4096
lseek(4, 55544426496, SEEK_SET)         = 55544426496

This happens as each worker scans the whole file until it finds the
entry it wants, skipping forward each block. In combination to the small
block size of the custom format dump, this causes many seeks and low
performance.

Fix by avoiding forward seeks for jumps of less than 1MB forward.
Do instead sequential reads.

Performance gain can be significant, depending on the size of the dump
and the I/O subsystem. On my local NVMe drive, read speeds for that
phase of pg_restore increased from 150MB/s to 3GB/s.


This is my first patch submission, all help is much appreciated.
Regards,
Dimitris


P.S.  What is the recommended way to test a change, besides a generic make
check? And how do I run selectively only the pg_dump/restore tests, in
order to speed up my development routine?

  2. Re: [PATCH v1] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Dimitrios Apostolou <jimis@gmx.net> — 2025-04-01T19:33:32Z

    On Sat, 29 Mar 2025, Dimitrios Apostolou wrote:
>
> P.S.  What is the recommended way to test a change, besides a generic make
> check? And how do I run selectively only the pg_dump/restore tests, in order
> to speed up my development routine?

I have tested it with:

   make  -C src/bin/pg_dump  check

It didn't break any test, but I also don't see any difference, the
performance boost is noticeable only when restoring a huge archive that is
missing offsets.

Any volunteer to review this one-line patch?

Thanks,
Dimitris

  3. Re: [PATCH v1] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Nathan Bossart <nathandbossart@gmail.com> — 2025-04-01T20:04:00Z

    On Tue, Apr 01, 2025 at 09:33:32PM +0200, Dimitrios Apostolou wrote:
> It didn't break any test, but I also don't see any difference, the
> performance boost is noticeable only when restoring a huge archive that is
> missing offsets.

This seems generally reasonable to me, but how did you decide on 1MB as the
threshold?  Have you tested other values?  Could the best threshold vary
based on the workload and hardware?

-- 
nathan

  4. Re: [PATCH v1] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Dimitrios Apostolou <jimis@gmx.net> — 2025-04-01T22:25:25Z

    Thanks. This is the first value I tried and it works well. In the archive I have all blocks seem to be between 8 and 20KB so the jump forward before the change never even got close to 1MB. Could it be bigger in an uncompressed archive? Or in a future pg_dump that raises the block size? I don't really know, so it is difficult to test such scenario but it made sense to guard against these cases too.

I chose 1MB by basically doing a very crude calculation in my mind: when would it be worth seeking forward instead of reading? On very slow drives 60MB/s sequential and 60 IOPS for random reads is a possible speed. In that worst case it would be better to seek() forward for lengths of over 1MB. 

On 1 April 2025 22:04:00 CEST, Nathan Bossart <nathandbossart@gmail.com> wrote:
>On Tue, Apr 01, 2025 at 09:33:32PM +0200, Dimitrios Apostolou wrote:
>> It didn't break any test, but I also don't see any difference, the
>> performance boost is noticeable only when restoring a huge archive that is
>> missing offsets.
>
>This seems generally reasonable to me, but how did you decide on 1MB as the
>threshold?  Have you tested other values?  Could the best threshold vary
>based on the workload and hardware?
>

  5. Re: [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Dimitrios Apostolou <jimis@gmx.net> — 2025-04-04T16:55:44Z

    I just managed to run pgindent, here is v2 with the comment style fixed.

  6. [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Dimitrios Apostolou <jimis@gmx.net> — 2025-04-14T16:31:48Z

    On Fri, 4 Apr 2025, Dimitrios Apostolou wrote:

> I just managed to run pgindent, here is v2 with the comment style fixed.

Any feedback on this one-liner? Or is the lack of feedback a clue that I 
have been missing something important in my patch submission? :-)

Should I CC people that are frequent committers to the file?


Thanks,
Dimitris

  7. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-14T17:27:29Z

    Dimitrios Apostolou <jimis@gmx.net> writes:
> Any feedback on this one-liner? Or is the lack of feedback a clue that I 
> have been missing something important in my patch submission? :-)

The calendar ;-).  At this point we're in feature freeze for v18,
so things that aren't bugs aren't likely to get much attention
until v19 development opens up (in July, unless things are really
going badly with v18).

You should add your patch to the July commitfest [1] to make sure
we don't lose track of it.

			regards, tom lane

[1] https://commitfest.postgresql.org/53/

  8. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Dimitrios Apostolou <jimis@gmx.net> — 2025-06-09T20:08:29Z

    
On Mon, 14 Apr 2025, Tom Lane wrote:

>
> You should add your patch to the July commitfest [1] to make sure
> we don't lose track of it.

I rebased the patch (attached) and created an entry in the commitfest:

https://commitfest.postgresql.org/patch/5809/


Thanks!
Dimitris

  9. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Dimitrios Apostolou <jimis@gmx.net> — 2025-06-09T20:09:57Z

    Attached now...

On Mon, 9 Jun 2025, Dimitrios Apostolou wrote:

>
>
> On Mon, 14 Apr 2025, Tom Lane wrote:
>
>>
>>  You should add your patch to the July commitfest [1] to make sure
>>  we don't lose track of it.
>
> I rebased the patch (attached) and created an entry in the commitfest:
>
> https://commitfest.postgresql.org/patch/5809/
>
>
> Thanks!
> Dimitris
>
>
>

  10. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Nathan Bossart <nathandbossart@gmail.com> — 2025-06-10T21:47:59Z

    On Mon, Jun 09, 2025 at 10:09:57PM +0200, Dimitrios Apostolou wrote:
> Fix by avoiding forward seeks for jumps of less than 1MB forward.
> Do instead sequential reads.
> 
> Performance gain can be significant, depending on the size of the dump
> and the I/O subsystem. On my local NVMe drive, read speeds for that
> phase of pg_restore increased from 150MB/s to 3GB/s.

I was curious about what exactly was leading to the performance gains you
are seeing.  This page has an explanation:

	https://www.mjr19.org.uk/IT/fseek.html

I also wrote a couple of test programs to show the difference between
fseeko-ing and fread-ing through a file with various sizes.  On a Linux
machine, I see this:

     log2(n) | fseeko  | fread
    ---------+---------+-------
           1 | 109.288 | 5.528
           2 |  54.881 | 2.848
           3 |   27.65 | 1.504
           4 |  13.953 | 0.834
           5 |     7.1 |  0.49
           6 |   3.665 | 0.322
           7 |   1.944 | 0.244
           8 |   1.085 | 0.201
           9 |   0.658 | 0.185
          10 |   0.443 | 0.175
          11 |   0.253 | 0.171
          12 |   0.102 | 0.162
          13 |   0.075 |  0.13
          14 |   0.061 | 0.114
          15 |   0.054 |   0.1

So, fseeko() starts winning around 4096 bytes.  On macOS, the differences
aren't quite as dramatic, but 4096 bytes is the break-even point there,
too.  I imagine there's a buffer around that size somewhere...

This doesn't fully explain the results you are seeing, but it does seem to
validate the idea.  I'm curious if you see further improvement with even
lower thresholds (e.g., 8KB, 16KB, 32KB). 

-- 
nathan

  11. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Dimitrios Apostolou <jimis@gmx.net> — 2025-06-10T22:32:58Z

    On Tue, 10 Jun 2025, Nathan Bossart wrote:

> I also wrote a couple of test programs to show the difference between
> fseeko-ing and fread-ing through a file with various sizes.  On a Linux
> machine, I see this:
>
>     log2(n) | fseeko  | fread
>    ---------+---------+-------
>           1 | 109.288 | 5.528
>           2 |  54.881 | 2.848
>           3 |   27.65 | 1.504
>           4 |  13.953 | 0.834
>           5 |     7.1 |  0.49
>           6 |   3.665 | 0.322
>           7 |   1.944 | 0.244
>           8 |   1.085 | 0.201
>           9 |   0.658 | 0.185
>          10 |   0.443 | 0.175
>          11 |   0.253 | 0.171
>          12 |   0.102 | 0.162
>          13 |   0.075 |  0.13
>          14 |   0.061 | 0.114
>          15 |   0.054 |   0.1
>
> So, fseeko() starts winning around 4096 bytes.  On macOS, the differences
> aren't quite as dramatic, but 4096 bytes is the break-even point there,
> too.  I imagine there's a buffer around that size somewhere...

Thank you for benchmarking! Before answering in more depth, I'm curious, 
what read-seek pattern do you see on the system call level (as 
shown by strace)? In pg_restore it was a constant loop of 
read(4K)-lseek(8-16K).

Dimitris

  12. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Nathan Bossart <nathandbossart@gmail.com> — 2025-06-11T21:14:53Z

    On Wed, Jun 11, 2025 at 12:32:58AM +0200, Dimitrios Apostolou wrote:
> Thank you for benchmarking! Before answering in more depth, I'm curious,
> what read-seek pattern do you see on the system call level (as shown by
> strace)? In pg_restore it was a constant loop of read(4K)-lseek(8-16K).

For fseeko(), sizes less than 4096 produce a repeating pattern of read()
calls followed by approximately (4096 / size) lseek() calls.  For greater
sizes, it's just a stream of lseek().  For fread(), sizes less than 4096
produce a stream of read(fd, "...", 4096), and for greater sizes, the only
difference is that the last argument is the size.

-- 
nathan

  13. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Dimitrios Apostolou <jimis@gmx.net> — 2025-06-11T23:25:00Z

    On Wed, 11 Jun 2025, Nathan Bossart wrote:

> On Wed, Jun 11, 2025 at 12:32:58AM +0200, Dimitrios Apostolou wrote:
>> what read-seek pattern do you see on the system call level (as shown by
>> strace)? In pg_restore it was a constant loop of read(4K)-lseek(8-16K).
>
> For fseeko(), sizes less than 4096 produce a repeating pattern of read()
> calls followed by approximately (4096 / size) lseek() calls.  For greater
> sizes, it's just a stream of lseek().

This is the opposite of what the link you shared before describes, so 
maybe glibc has changed its behaviour to improve performance.

Anyway, the fact that fseek(>4096) produces a stream of lseek()s, means 
that most likely no I/O is happening. You need to issue a getc() after 
each fseeko(), like pg_restore is doing.


Dimitris

  14. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Dimitrios Apostolou <jimis@gmx.net> — 2025-06-12T23:00:26Z

    On Tue, 10 Jun 2025, Nathan Bossart wrote:

> So, fseeko() starts winning around 4096 bytes.  On macOS, the differences
> aren't quite as dramatic, but 4096 bytes is the break-even point there,
> too.  I imagine there's a buffer around that size somewhere...
>
> This doesn't fully explain the results you are seeing, but it does seem to
> validate the idea.  I'm curious if you see further improvement with even
> lower thresholds (e.g., 8KB, 16KB, 32KB).

By the way, I might have set the threshold to 1MB in my program, but 
lowering it won't show a difference in my test case, since the lseek()s I 
was noticing before the patch were mostly 8-16KB forward. Not sure what is 
the defining factor for that. Maybe the compression algorithm, or how wide 
the table is?


Dimitris

  15. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Nathan Bossart <nathandbossart@gmail.com> — 2025-06-13T22:04:47Z

    On Fri, Jun 13, 2025 at 01:00:26AM +0200, Dimitrios Apostolou wrote:
> By the way, I might have set the threshold to 1MB in my program, but
> lowering it won't show a difference in my test case, since the lseek()s I
> was noticing before the patch were mostly 8-16KB forward. Not sure what is
> the defining factor for that. Maybe the compression algorithm, or how wide
> the table is?

I may have missed it, but could you share what the strace looks like with
the patch applied?

-- 
nathan

  16. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Thomas Munro <thomas.munro@gmail.com> — 2025-06-13T23:15:25Z

    On Wed, Jun 11, 2025 at 9:48 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
> So, fseeko() starts winning around 4096 bytes.  On macOS, the differences
> aren't quite as dramatic, but 4096 bytes is the break-even point there,
> too.  I imagine there's a buffer around that size somewhere...

BTW you can call setvbuf(f, my_buffer, _IOFBF, my_buffer_size) to
control FILE buffering.  I suspect that glibc ignores the size if you
pass NULL for my_buffer, so you'd need to allocate it yourself and it
should probably be aligned on PG_IO_ALIGN_SIZE for best results
(minimising the number of VM pages that must be held/pinned).  Then
you might be able to get better and less OS-dependent results.  I
haven't studied this seek business so I have no opinion on that and
what a good size would be, but interesting sizes might be
rounded to both PG_IO_ALIGN_SIZE and filesystem block size according
to fstat(fileno(stream)).  IDK, just a thought...

  17. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Dimitrios Apostolou <jimis@gmx.net> — 2025-06-14T16:01:13Z

    On Fri, 13 Jun 2025, Nathan Bossart wrote:

> On Fri, Jun 13, 2025 at 01:00:26AM +0200, Dimitrios Apostolou wrote:
>> By the way, I might have set the threshold to 1MB in my program, but
>> lowering it won't show a difference in my test case, since the lseek()s I
>> was noticing before the patch were mostly 8-16KB forward. Not sure what is
>> the defining factor for that. Maybe the compression algorithm, or how wide
>> the table is?
>
> I may have missed it, but could you share what the strace looks like with
> the patch applied?

read(4, "..."..., 8192) = 8192
read(4, "..."..., 4096) = 4096
read(4, "..."..., 12288) = 12288
read(4, "..."..., 4096) = 4096
read(4, "..."..., 8192) = 8192
read(4, "..."..., 4096) = 4096
read(4, "..."..., 8192) = 8192
read(4, "..."..., 4096) = 4096
read(4, "..."..., 8192) = 8192
read(4, "..."..., 4096) = 4096
read(4, "..."..., 8192) = 8192
read(4, "..."..., 4096) = 4096
read(4, "..."..., 8192) = 8192
read(4, "..."..., 4096) = 4096
read(4, "..."..., 8192) = 8192
read(4, "..."..., 4096) = 4096
read(4, "..."..., 12288) = 12288
read(4, "..."..., 4096) = 4096
read(4, "..."..., 8192) = 8192
read(4, "..."..., 4096) = 4096
read(4, "..."..., 12288) = 12288
read(4, "..."..., 4096) = 4096
read(4, "..."..., 8192) = 8192
read(4, "..."..., 4096) = 4096
read(4, "..."..., 8192) = 8192
read(4, "..."..., 4096) = 4096
read(4, "..."..., 12288) = 12288
read(4, "..."..., 4096) = 4096
read(4, "..."..., 8192) = 8192
read(4, "..."..., 4096) = 4096

  18. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Dimitrios Apostolou <jimis@gmx.net> — 2025-06-14T16:17:41Z

    On Sat, 14 Jun 2025, Dimitrios Apostolou wrote:

> On Fri, 13 Jun 2025, Nathan Bossart wrote:
>
>>  On Fri, Jun 13, 2025 at 01:00:26AM +0200, Dimitrios Apostolou wrote:
>>>  By the way, I might have set the threshold to 1MB in my program, but
>>>  lowering it won't show a difference in my test case, since the lseek()s I
>>>  was noticing before the patch were mostly 8-16KB forward. Not sure what
>>>  is
>>>  the defining factor for that. Maybe the compression algorithm, or how
>>>  wide
>>>  the table is?
>>
>>  I may have missed it, but could you share what the strace looks like with
>>  the patch applied?
>
> read(4, "..."..., 8192) = 8192
> read(4, "..."..., 4096) = 4096
> read(4, "..."..., 12288) = 12288
> read(4, "..."..., 4096) = 4096
> read(4, "..."..., 8192) = 8192
> read(4, "..."..., 4096) = 4096
> read(4, "..."..., 8192) = 8192
> read(4, "..."..., 4096) = 4096
> read(4, "..."..., 8192) = 8192
> read(4, "..."..., 4096) = 4096
> read(4, "..."..., 8192) = 8192
> read(4, "..."..., 4096) = 4096
> read(4, "..."..., 8192) = 8192
> read(4, "..."..., 4096) = 4096
> read(4, "..."..., 8192) = 8192
> read(4, "..."..., 4096) = 4096
> read(4, "..."..., 12288) = 12288
> read(4, "..."..., 4096) = 4096
> read(4, "..."..., 8192) = 8192
> read(4, "..."..., 4096) = 4096
> read(4, "..."..., 12288) = 12288
> read(4, "..."..., 4096) = 4096
> read(4, "..."..., 8192) = 8192
> read(4, "..."..., 4096) = 4096
> read(4, "..."..., 8192) = 8192
> read(4, "..."..., 4096) = 4096
> read(4, "..."..., 12288) = 12288
> read(4, "..."..., 4096) = 4096
> read(4, "..."..., 8192) = 8192
> read(4, "..."..., 4096) = 4096


This was from pg_restoring a zstd-compressed custom format dump.

Out of curiosity I've tried the same with an uncompressed dump 
(--compress=none). Surprisingly it seems the blocksize is even smaller.

With my patched pg_restore I only get 4K reads and nothing else on 
the strace output.

read(4, "..."..., 4096) = 4096
read(4, "..."..., 4096) = 4096
read(4, "..."..., 4096) = 4096
read(4, "..."..., 4096) = 4096
read(4, "..."..., 4096) = 4096
read(4, "..."..., 4096) = 4096

The unpatched pg_restore gives me the weirdest output ever:

read(4, "..."..., 4096) = 4096
lseek(4, 98527916032, SEEK_SET)         = 98527916032
lseek(4, 98527916032, SEEK_SET)         = 98527916032
lseek(4, 98527916032, SEEK_SET)         = 98527916032
lseek(4, 98527916032, SEEK_SET)         = 98527916032
lseek(4, 98527916032, SEEK_SET)         = 98527916032
lseek(4, 98527916032, SEEK_SET)         = 98527916032
[ ... repeats about 80 times ...]
read(4, "..."..., 4096) = 4096
lseek(4, 98527920128, SEEK_SET)         = 98527920128
lseek(4, 98527920128, SEEK_SET)         = 98527920128
lseek(4, 98527920128, SEEK_SET)         = 98527920128
lseek(4, 98527920128, SEEK_SET)         = 98527920128
[ ... repeats ... ]



Seeing this, I think we should really consider raising the pg_dump block 
size like Tom suggested on a previous thread.


Dimitris

  19. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Dimitrios Apostolou <jimis@gmx.net> — 2025-07-23T21:54:44Z

    Hi Nathan, I've noticed you've set yourself as a reviewer of this patch 
in the commitfest. I appreciate it, but you might want to combine it 
with another simple patch [1] that speeds up the same part of 
pg_restore: the initial full scan on TOC-less archives.

[1] https://commitfest.postgresql.org/patch/5817/


On Saturday 2025-06-14 00:04, Nathan Bossart wrote:
>
> On Fri, Jun 13, 2025 at 01:00:26AM +0200, Dimitrios Apostolou wrote:
>> By the way, I might have set the threshold to 1MB in my program, but
>> lowering it won't show a difference in my test case, since the lseek()s I
>> was noticing before the patch were mostly 8-16KB forward. Not sure what is
>> the defining factor for that. Maybe the compression algorithm, or how wide
>> the table is?
>
> I may have missed it, but could you share what the strace looks like with
> the patch applied?

I hope you've seen my response here, with special focus on the small 
block size that both compressed and uncompressed custom format archives 
have.

I have been needing to pg_restore 10TB TOC-less dumps recently, and it's 
a pain to do the full scan, even with both of my patches applied.

Maybe the block size could be a command line option of pg_dump, so that 
one could set it to sizes like 100MB, which sounds like a normal block 
from the perspective of a 10TB gigantic dump.

Regards,
Dimitris

  20. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Dimitrios Apostolou <jimis@gmx.net> — 2025-07-28T13:34:20Z

    On Saturday 2025-06-14 18:17, Dimitrios Apostolou wrote:

> Out of curiosity I've tried the same with an uncompressed dump 
> (--compress=none). Surprisingly it seems the blocksize is even smaller.
>
> With my patched pg_restore I only get 4K reads and nothing else on the strace 
> output.
>
> read(4, "..."..., 4096) = 4096
> read(4, "..."..., 4096) = 4096
> read(4, "..."..., 4096) = 4096
> read(4, "..."..., 4096) = 4096
> read(4, "..."..., 4096) = 4096
> read(4, "..."..., 4096) = 4096


To clarify this output again, I have a huge uncompressed custom format 
dump without TOC (because pg_dump was writing to stdout), and at this 
point pg_restore is going through the whole archive to find the items it 
needs. Allow me to explain what goes on at this point since I have 
better insight now.

The code in question, in pg_backup_custom.c:


/*
  * Skip data from current file position.
  * Data blocks are formatted as an integer length, followed by data.
  * A zero length indicates the end of the block.
*/
static void
_skipData(ArchiveHandle *AH)
{
 	lclContext *ctx = (lclContext *) AH->formatData;
 	size_t		blkLen;
 	char	   *buf = NULL;
 	int			buflen = 0;

 	blkLen = ReadInt(AH);
 	while (blkLen != 0)
 	{
 		/* Sequential access is usually faster, so avoid seeking if the
 		 * jump forward is shorter than 1MB. */
 		if (ctx->hasSeek && blkLen > 1024 * 1024)
 		{
 			if (fseeko(AH->FH, blkLen, SEEK_CUR) != 0)
 				pg_fatal("error during file seek: %m");
 		}
 		else
 		{
 			if (blkLen > buflen)
 			{
 				free(buf);
 				buf = (char *) pg_malloc(blkLen);
 				buflen = blkLen;
 			}
 			if (fread(buf, 1, blkLen, AH->FH) != blkLen)
 			{
 				if (feof(AH->FH))
 					pg_fatal("could not read from input file: end of file");
 				else
 					pg_fatal("could not read from input file: %m");
 			}
 		}

 		blkLen = ReadInt(AH);
 	}

 	free(buf);
}


blkLen is almost always a number around 35 to 38.
So fread() is called all the time doing reads of about ~35 bytes.
Then ReadInt() is actually doing getc() a few times.
And it loops over.

Libc is doing buffering of 4k, and that's how we end up seeing so many 
4k reads. This also explains the ~80 lseek() between each 4k read() on 
the unpatched version, mentioned in previous email.

I've tried setvbuf() like Thomas Munro suggested and I saw a significant 
improvement by allocating and using a 1MB buffer for libc stream 
buffering.

Question that remains: where is pg_dump setting this ~35B length block? 
Is that easy to change without breaking old versions?


Thanks in advance,
Dimitris

  21. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-10T20:26:57Z

    Dimitrios Apostolou <jimis@gmx.net> writes:
> Question that remains: where is pg_dump setting this ~35B length block? 

I poked into that question, and found that the answer is some
exceedingly brain-dead buffering logic in compress_zstd.c.
It will dump its buffer during every loop iteration within
_ZstdWriteCommon, no matter how much buffer space it has left;
and each call to cs->writeF() produces a new "data block"
in the output file.  The amount of data fed to _ZstdWriteCommon
per call is whatever the backend sends per "copy data" message,
which is generally one table row.  So if the table rows aren't
too wide, or if they're highly compressible, you get these
tiny data blocks.

compress_lz4.c is equally broken, again buffering no bytes across
calls; although liblz4 seems to do some buffering internally.
I got blocks of around 300 bytes on the test case I was using.
That's still ridiculous.

compress_gzip.c is actually sanely implemented, and consistently
produces blocks of 4096 bytes, which traces to DEFAULT_IO_BUFFER_SIZE
in compress_io.h.

If you choose --compress=none, you get data blocks that correspond
exactly to table rows.  We could imagine doing some internal
buffering to amalgamate short rows into larger blocks, but I'm
not entirely convinced it's worth messing with that case.

The attached patch fixes the buffering logic in compress_zstd.c
and compress_lz4.c.  For zstd, most blocks are now 131591 bytes,
which seems to be determined by ZSTD_CStreamOutSize() not by
our code.  For lz4, I see a range of block sizes but they're
almost all around 64K.  That's apparently emergent from the
behavior of LZ4F_compressBound(): when told we want to supply
it up to 4K at a time, it says it needs a buffer around 64K.

I'm tempted to increase DEFAULT_IO_BUFFER_SIZE so that gzip
also produces blocks in the vicinity of 64K, but we'd have
to decouple the behavior of compress_lz4.c somehow, or it
would want to produce blocks around a megabyte which might
be excessive.  (Or if it's not, we'd still want all these
compression methods to choose similar block sizes, I'd think.)

Anyway, these fixes should remove the need for pg_restore to look
at quite so many places in the archive file.  There may still be
a need for altering the seek-versus-read behavior as you suggest,
but I think we need to re-measure that tradeoff after fixing the
pg_dump side.

			regards, tom lane

  22. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-11T02:22:16Z

    I wrote:
> I'm tempted to increase DEFAULT_IO_BUFFER_SIZE so that gzip
> also produces blocks in the vicinity of 64K, but we'd have
> to decouple the behavior of compress_lz4.c somehow, or it
> would want to produce blocks around a megabyte which might
> be excessive.  (Or if it's not, we'd still want all these
> compression methods to choose similar block sizes, I'd think.)

After a bit of further experimentation, here is a v2 patchset.

0001 is like my previous patch except that it also fixes
Zstd_write and Zstd_close so that the "stream API" code doesn't
behave differently from the older API.  Also, now with draft
commit message.

0002 adjusts things so that lz4 and gzip compression produce
block sizes around 128K, which is what compress_zstd.c already
does after 0001.  While I wouldn't necessarily follow zstd's
lead if it were easy to do differently, it isn't.  We'd have
to ignore ZSTD_CStreamOutSize() in favor of making our own
buffer size choice.  That seems to carry some risks of tickling
bugs that upstream isn't testing for, and the value of 128K is
not so far off that I care to take any such risk.

This brings us to a place where all three compression modes
should yield roughly-comparable data block sizes, which is a good
starting point for further discussion of whether pg_restore needs
seek-versus-read adjustments.

			regards, tom lane

  23. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-12T01:25:20Z

    While playing around with the test cases for pg_dump compression,
I was startled to discover that the performance of compress_lz4's
"stream API" code is absolutely abysmal.  Here is a simple test
case to demonstrate, using the regression database as test data:

$ pg_dump -Fd --compress=lz4 -f rlz4.dir regression
$ time pg_restore -f /dev/null rlz4.dir

real    0m0.023s
user    0m0.017s
sys     0m0.006s

So far so good, but now let's compress the toc.dat file:

$ lz4 -f -m --rm rlz4.dir/toc.dat
$ time pg_restore -f /dev/null rlz4.dir

real    0m1.335s
user    0m1.326s
sys     0m0.008s

Considering that lz4 prides itself on fast decompression speed,
that is not a sane result.  Decompressing the file only requires
a couple ms on my machine:

$ time lz4cat rlz4.dir/toc.dat.lz4 >/dev/null

real    0m0.002s
user    0m0.000s
sys     0m0.002s

So on this example, pg_restore is something more than 600x slower
to read the TOC data than it ought to be.

On investigation, the blame mostly affixes to LZ4Stream_read_overflow's
habit of memmove'ing all the remaining buffered data after each read
operation.  Since reading a TOC file tends to involve a lot of small
(even one-byte) decompression calls, that amounts to an O(N^2) cost.

This could have been fixed with a minimal patch, but to my
eyes LZ4Stream_read_internal and LZ4Stream_read_overflow are
badly-written spaghetti code; in particular the eol_flag logic
is inefficient and duplicative.  I chose to throw the code
away and rewrite from scratch.  This version is about sixty
lines shorter as well as not having the performance issue.

Fortunately, AFAICT the only way to get to this problem is to
manually LZ4-compress the toc.dat and/or blobs.toc files within a
directory-style archive.  Few people do that, which likely explains
the lack of field complaints.

On top of that, a similar case with gzip doesn't work at all,
though it's supposed to:

$ pg_dump -Fd --compress=gzip -f rgzip.dir regression
$ gzip rgzip.dir/toc.dat 
$ pg_restore -f /dev/null rgzip.dir
pg_restore: error: could not read from input file: 

Tracking this down, it seems that Gzip_read doesn't cope with
a request to read zero bytes.  I wonder how long that's been
broken.

As far as I can see, 002_pg_dump.pl doesn't exercise the case of
manually-compressed toc.dat files.  I wonder why not.

0001 and 0002 attached are the same as before, then 0003 adds a fix
for the LZ4 performance problem, and 0004 fixes the Gzip_read problem.
While at it, I got rid of a few other minor inefficiencies such as
unnecessary buffer-zeroing.

			regards, tom lane

  24. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Chao Li <li.evan.chao@gmail.com> — 2025-10-13T03:33:06Z

    
> On Oct 12, 2025, at 09:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> While playing around with the test cases for pg_dump compression,
> I was startled to discover that the performance of compress_lz4's
> "stream API" code is absolutely abysmal.  Here is a simple test
> case to demonstrate, using the regression database as test data:
> 
> $ pg_dump -Fd --compress=lz4 -f rlz4.dir regression
> $ time pg_restore -f /dev/null rlz4.dir
> 
> real    0m0.023s
> user    0m0.017s
> sys     0m0.006s
> 
> So far so good, but now let's compress the toc.dat file:
> 
> $ lz4 -f -m --rm rlz4.dir/toc.dat
> $ time pg_restore -f /dev/null rlz4.dir
> 
> real    0m1.335s
> user    0m1.326s
> sys     0m0.008s
> 
> Considering that lz4 prides itself on fast decompression speed,
> that is not a sane result.  Decompressing the file only requires
> a couple ms on my machine:
> 
> $ time lz4cat rlz4.dir/toc.dat.lz4 >/dev/null
> 
> real    0m0.002s
> user    0m0.000s
> sys     0m0.002s
> 
> So on this example, pg_restore is something more than 600x slower
> to read the TOC data than it ought to be.
> 


I tested the patch on my MacBook M4 (Sequoia 15.6.1), compiled with -O2 optimization:

- For LZ4 performance improvement:

Without the fix (latest master):
```
chaol@ChaodeMacBook-Air tmp % pg_dump -Fd --compress=lz4 -f rlz4.dir regression

chaol@ChaodeMacBook-Air tmp % time pg_restore -f /dev/null rlz4.dir
pg_restore -f /dev/null rlz4.dir  0.03s user 0.03s system 11% cpu 0.463 total

chaol@ChaodeMacBook-Air tmp % lz4 -f -m --rm rlz4.dir/toc.dat

# It took 1.59s to restore from compressed data, much slower than from uncompressed data.
chaol@ChaodeMacBook-Air tmp % time pg_restore -f /dev/null rlz4.dir
pg_restore -f /dev/null rlz4.dir  1.59s user 0.02s system 97% cpu 1.653 total
```

With the fix:
```
chaol@ChaodeMacBook-Air tmp % pg_dump -Fd --compress=lz4 -f rlz4.dir regression

chaol@ChaodeMacBook-Air tmp % time pg_restore -f /dev/null rlz4.dir
pg_restore -f /dev/null rlz4.dir  0.02s user 0.03s system 16% cpu 0.305 total

chaol@ChaodeMacBook-Air tmp % lz4 -f -m --rm rlz4.dir/toc.dat

# Much faster !!!
chaol@ChaodeMacBook-Air tmp % time pg_restore -f /dev/null rlz4.dir
pg_restore -f /dev/null rlz4.dir  0.02s user 0.02s system 93% cpu 0.043 total
```

- For Gzip zero read bug:

Without the fix:
```
chaol@ChaodeMacBook-Air tmp % pg_dump -Fd --compress=gzip -f rgzip.dir regression
chaol@ChaodeMacBook-Air tmp % gzip rgzip.dir/toc.dat

# Failed
chaol@ChaodeMacBook-Air tmp % pg_restore -f /dev/null rgzip.dir
pg_restore: error: could not read from input file:
```

With the fix:
```
chaol@ChaodeMacBook-Air tmp % pg_dump -Fd --compress=gzip -f rgzip.dir regression
chaol@ChaodeMacBook-Air tmp % gzip rgzip.dir/toc.dat

# Ran successfully 
chaol@ChaodeMacBook-Air tmp % pg_restore -f /dev/null rgzip.dir
```

I also reviewed the code change, basically LGTM. Just a couple of trivial comments:

1 - 0001
In WriteDataToArchiveLZ4()

```
+		required = LZ4F_compressBound(chunk, &state->prefs);
+		if (required > state->buflen - state->bufdata)
+		{
+			cs->writeF(AH, state->buffer, state->bufdata);
+			state->bufdata = 0;
+		}
```

And in EndCompressorLZ4()
```
+	required = LZ4F_compressBound(0, &state->prefs);
+	if (required > state->buflen - state->bufdata)
+	{
+		cs->writeF(AH, state->buffer, state->bufdata);
+		state->bufdata = 0;
+	}
```

These two code pieces are similar, only difference is the first parameter passed to LZ4F_compressBound().

I think we can create an inline function for it. But these are just 5 lines, I don’t have a strong option on that.

Same thing for the other code pieces in LZ4Stream_write() and LZ4Stream_close().

2 - 0003
```
 /*
  * LZ4 equivalent to feof() or gzeof().  Return true iff there is no
- * decompressed output in the overflow buffer and the end of the backing file
```

This doesn’t belong to the current patch. But “iff” seems a typo of “if”. You may fix it as you are touching this piece of code.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  25. Re: [PATCH v1] parallel pg_restore: avoid disk seeks when jumping short distance forward

    David G. Johnston <david.g.johnston@gmail.com> — 2025-10-13T03:39:31Z

    On Sunday, October 12, 2025, Chao Li <li.evan.chao@gmail.com> wrote:

> 2 - 0003
> ```
>  /*
>   * LZ4 equivalent to feof() or gzeof().  Return true iff there is no
> - * decompressed output in the overflow buffer and the end of the backing
> file
> ```
>
> This doesn’t belong to the current patch. But “iff” seems a typo of “if”.
> You may fix it as you are touching this piece of code.
>

“iif” is shorthand for “if and only if”.  So it isn’t likely to be a typo;
it only needs to be changed if it is wrong.  I haven’t looked to see.

David J.

  26. Re: [PATCH v1] parallel pg_restore: avoid disk seeks when jumping short distance forward

    David G. Johnston <david.g.johnston@gmail.com> — 2025-10-13T03:40:34Z

    On Sunday, October 12, 2025, David G. Johnston <david.g.johnston@gmail.com>
wrote:

> On Sunday, October 12, 2025, Chao Li <li.evan.chao@gmail.com> wrote:
>
>> 2 - 0003
>> ```
>>  /*
>>   * LZ4 equivalent to feof() or gzeof().  Return true iff there is no
>> - * decompressed output in the overflow buffer and the end of the backing
>> file
>> ```
>>
>> This doesn’t belong to the current patch. But “iff” seems a typo of “if”.
>> You may fix it as you are touching this piece of code.
>>
>
> “iif” is shorthand for “if and only if”.  So it isn’t likely to be a typo;
> it only needs to be changed if it is wrong.  I haven’t looked to see.
>

Never mind…two f’s, not two i’s …

David J.

  27. Re: [PATCH v1] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Chao Li <li.evan.chao@gmail.com> — 2025-10-13T03:41:15Z

    
> On Oct 13, 2025, at 11:39, David G. Johnston <david.g.johnston@gmail.com> wrote:
> 
> 
> “iif” is shorthand for “if and only if”.  So it isn’t likely to be a typo; it only needs to be changed if it is wrong.  I haven’t looked to see.
> 

Thanks for the explanation, I wasn’t aware of that. I learned.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  28. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-13T21:07:52Z

    Chao Li <li.evan.chao@gmail.com> writes:
>> On Oct 12, 2025, at 09:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> While playing around with the test cases for pg_dump compression,
>> I was startled to discover that the performance of compress_lz4's
>> "stream API" code is absolutely abysmal.

> I tested the patch on my MacBook M4 (Sequoia 15.6.1), compiled with -O2 optimization:

Thanks for looking at it!

> I also reviewed the code change, basically LGTM. Just a couple of trivial comments:

> 1 - 0001
> In WriteDataToArchiveLZ4()

> ```
> +		required = LZ4F_compressBound(chunk, &state->prefs);
> +		if (required > state->buflen - state->bufdata)
> +		{
> +			cs->writeF(AH, state->buffer, state->bufdata);
> +			state->bufdata = 0;
> +		}
> ```

> And in EndCompressorLZ4()
> ```
> +	required = LZ4F_compressBound(0, &state->prefs);
> +	if (required > state->buflen - state->bufdata)
> +	{
> +		cs->writeF(AH, state->buffer, state->bufdata);
> +		state->bufdata = 0;
> +	}
> ```

> These two code pieces are similar, only difference is the first parameter passed to LZ4F_compressBound().

> I think we can create an inline function for it. But these are just 5 lines, I don’t have a strong option on that.

Yeah, I don't think that would improve code readability noticeably.
By the time you got done writing a documentation comment for the
new subroutine, the code would probably be longer not shorter.

I've pushed the parts of that patch set that I thought were
uncontroversial.  What's left is the business about increasing
DEFAULT_IO_BUFFER_SIZE and then adjusting the tests appropriately.

So, v4-0001 attached is the previous v3-0002 to increase
DEFAULT_IO_BUFFER_SIZE, plus additions in compress_none.c to make
--compress=none also produce predictably large data blocks.
I decided that if we're going to rely on that behavior as part
of the solution for this thread's original problem, we'd better
make it happen for all compression options.

0002 adds a test case in 002_pg_dump.pl to exercise --compress=none,
because without that we don't have any coverage of the new code
0001 added in compress_none.c.  That makes for a small increase
in the runtime of 002_pg_dump.pl, but I'm inclined to think it's
worth doing.

0003 modifies the existing test cases that manually compress
blobs.toc files so that they also compress toc.dat.  I feel
like it's mostly an oversight that that wasn't done to begin
with; if it had been done, we'd have caught the Gzip_read bug
right away.  Also, AFAICT, this doesn't cost anything measurable
in test runtime.

0004 increases the row width in the existing test case that says
it's trying to push more than DEFAULT_IO_BUFFER_SIZE through
the compressors.  While I agree with the premise, this solution
is hugely expensive: it adds about 12% to the already-long runtime
of 002_pg_dump.pl.  I'd like to find a better way, but ran out of
energy for today.  (I think the reason this costs so much is that
it's effectively iterated hundreds of times because of
002_pg_dump.pl's more or less cross-product approach to testing
everything.  Maybe we should pull it out of that structure?)

			regards, tom lane

  29. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Chao Li <li.evan.chao@gmail.com> — 2025-10-14T00:09:56Z

    
> On Oct 14, 2025, at 05:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> 
> I've pushed the parts of that patch set that I thought were
> uncontroversial.  What's left is the business about increasing
> DEFAULT_IO_BUFFER_SIZE and then adjusting the tests appropriately.
> 
> So, v4-0001 attached is the previous v3-0002 to increase
> DEFAULT_IO_BUFFER_SIZE, plus additions in compress_none.c to make
> --compress=none also produce predictably large data blocks.
> I decided that if we're going to rely on that behavior as part
> of the solution for this thread's original problem, we'd better
> make it happen for all compression options.
> 
> 0002 adds a test case in 002_pg_dump.pl to exercise --compress=none,
> because without that we don't have any coverage of the new code
> 0001 added in compress_none.c.  That makes for a small increase
> in the runtime of 002_pg_dump.pl, but I'm inclined to think it's
> worth doing.
> 
> 0003 modifies the existing test cases that manually compress
> blobs.toc files so that they also compress toc.dat.  I feel
> like it's mostly an oversight that that wasn't done to begin
> with; if it had been done, we'd have caught the Gzip_read bug
> right away.  Also, AFAICT, this doesn't cost anything measurable
> in test runtime.
> 
> 0004 increases the row width in the existing test case that says
> it's trying to push more than DEFAULT_IO_BUFFER_SIZE through
> the compressors.  While I agree with the premise, this solution
> is hugely expensive: it adds about 12% to the already-long runtime
> of 002_pg_dump.pl.  I'd like to find a better way, but ran out of
> energy for today.  (I think the reason this costs so much is that
> it's effectively iterated hundreds of times because of
> 002_pg_dump.pl's more or less cross-product approach to testing
> everything.  Maybe we should pull it out of that structure?)
> 

In v4 patch, the code changes are straightforward. 0001 changes compress_none.c to write data to a 128K buffer first, then only flush the buffer when it’s filled up. 0002, 0003 and 0004 add more test cases. I have no comment to the code diff.

I tested DEFAULT_IO_BUFFER_SIZE with 4K, 32K, 64K, 128K and 256K. Looks like increasing the buffer size doesn’t improve the performance significantly. Actually, with the buffer size 64K, 128K and 256K, the test results are very close. I tested both with lz4 and none compression. I am not suggesting tuning the buffer size. These data are only for your reference.

To do the test, I created a test db and filled in several GB of data.

```
256K ====
% time pg_dump -Fd --compress=lz4 -f dump_A.dir evantest
pg_dump -Fd --compress=lz4 -f dump_A.dir evantest  3.37s user 0.82s system 57% cpu 7.249 total
% time pg_restore -f /dev/null dump_A.dir
pg_restore -f /dev/null dump_A.dir  0.24s user 0.19s system 43% cpu 0.991 total

% time pg_dump -Fd --compress=none -f dump_A.dir evantest
pg_dump -Fd --compress=none -f dump_A.dir evantest  2.34s user 1.72s system 68% cpu 5.949 total
% time pg_restore -f /dev/null dump_A.dir
pg_restore -f /dev/null dump_A.dir  0.02s user 0.19s system 22% cpu 0.921 total

128K ===
% time pg_dump -Fd --compress=lz4 -f dump_A.dir evantest
pg_dump -Fd --compress=lz4 -f dump_A.dir evantest  3.38s user 0.85s system 64% cpu 6.525 total
% time pg_restore -f /dev/null dump_A.dir
pg_restore -f /dev/null dump_A.dir  0.28s user 0.21s system 47% cpu 1.042 total

% time pg_dump -Fd --compress=none -f dump_A.dir evantest
pg_dump -Fd --compress=none -f dump_A.dir evantest  2.34s user 1.67s system 68% cpu 5.835 total
% time pg_restore -f /dev/null dump_A.dir
pg_restore -f /dev/null dump_A.dir  0.03s user 0.22s system 22% cpu 1.118 total

64K ===
% time pg_dump -Fd --compress=lz4 -f dump_A.dir evantest
pg_dump -Fd --compress=lz4 -f dump_A.dir evantest  3.39s user 0.92s system 63% cpu 6.761 total
% time pg_restore -f /dev/null dump_A.dir
pg_restore -f /dev/null dump_A.dir  0.33s user 0.24s system 40% cpu 1.420 total

% time pg_dump -Fd --compress=none -f dump_A.dir evantest
pg_dump -Fd --compress=none -f dump_A.dir evantest  2.35s user 1.74s system 69% cpu 5.849 total
% time pg_restore -f /dev/null dump_A.dir
pg_restore -f /dev/null dump_A.dir  0.04s user 0.22s system 27% cpu 0.939 total

32K ===
% time pg_dump -Fd --compress=lz4 -f dump_A.dir evantest
pg_dump -Fd --compress=lz4 -f dump_A.dir evantest  3.43s user 0.94s system 58% cpu 7.416 total
% time pg_restore -f /dev/null dump_A.dir
pg_restore -f /dev/null dump_A.dir  0.34s user 0.22s system 56% cpu 0.983 total

% time pg_dump -Fd --compress=none -f dump_A.dir evantest
pg_dump -Fd --compress=none -f dump_A.dir evantest  2.34s user 1.75s system 67% cpu 6.070 total
% time pg_restore -f /dev/null dump_A.dir
pg_restore -f /dev/null dump_A.dir  0.05s user 0.23s system 29% cpu 0.926 total

4k====
% time pg_dump -Fd --compress=lz4 -f dump_A.dir evantest
pg_dump -Fd --compress=lz4 -f dump_A.dir evantest  3.45s user 0.94s system 60% cpu 7.298 total
% time pg_restore -f /dev/null dump_A.dir
pg_restore -f /dev/null dump_A.dir  0.37s user 0.29s system 64% cpu 1.016 total

% time pg_dump -Fd --compress=none -f dump_A.dir evantest
pg_dump -Fd --compress=none -f dump_A.dir evantest  2.33s user 1.78s system 69% cpu 5.947 total
% time pg_restore -f /dev/null dump_A.dir
pg_restore -f /dev/null dump_A.dir  0.12s user 0.29s system 40% cpu 1.009 total
```

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  30. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-14T00:36:07Z

    Chao Li <li.evan.chao@gmail.com> writes:
> I tested DEFAULT_IO_BUFFER_SIZE with 4K, 32K, 64K, 128K and 256K. Looks like increasing the buffer size doesn’t improve the performance significantly. Actually, with the buffer size 64K, 128K and 256K, the test results are very close. I tested both with lz4 and none compression. I am not suggesting tuning the buffer size. These data are only for your reference.

Yeah, I would not expect straight pg_dump/pg_restore performance
to vary very much once the buffer size gets above not-too-many KB.
The thing we are really interested in here is how fast pg_restore
can skip over unwanted table data in a large archive file, and that
I believe should be pretty sensitive to block size.

You could measure that without getting into the complexities of
parallel restore if you make a custom-format dump of a few large
tables that does not have offset data in it, and then seeing how
fast is selective restore of just the last table.

			regards, tom lane

  31. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Chao Li <li.evan.chao@gmail.com> — 2025-10-14T01:37:45Z

    
> On Oct 14, 2025, at 08:36, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Chao Li <li.evan.chao@gmail.com> writes:
>> I tested DEFAULT_IO_BUFFER_SIZE with 4K, 32K, 64K, 128K and 256K. Looks like increasing the buffer size doesn’t improve the performance significantly. Actually, with the buffer size 64K, 128K and 256K, the test results are very close. I tested both with lz4 and none compression. I am not suggesting tuning the buffer size. These data are only for your reference.
> 
> Yeah, I would not expect straight pg_dump/pg_restore performance
> to vary very much once the buffer size gets above not-too-many KB.
> The thing we are really interested in here is how fast pg_restore
> can skip over unwanted table data in a large archive file, and that
> I believe should be pretty sensitive to block size.
> 
> You could measure that without getting into the complexities of
> parallel restore if you make a custom-format dump of a few large
> tables that does not have offset data in it, and then seeing how
> fast is selective restore of just the last table.
> 
> 	

Not sure if I did something wrong, but I still don’t see much difference between buffer size 4K and 128K with your suggested test.

I created 3 tables, each with 10 millions of rows:

```
evantest=# CREATE TABLE tbl1 AS SELECT generate_series(1,10000000) AS id;
SELECT 10000000
evantest=# CREATE TABLE tbl2 AS SELECT generate_series(1,10000000) AS id;
SELECT 10000000
evantest=# CREATE TABLE tbl3 AS SELECT generate_series(1,10000000) AS id;
SELECT 10000000
```

And did a custom-format dump:
```
% time pg_dump -Fc -f db.dump evantest
pg_dump -Fc -f db.dump evantest  51.72s user 1.13s system 98% cpu 53.602 total
```

Then pg_restore the last tabl,  compiled with buffer size 4k and 128k: (I dropped tbl3 before running pg_restore)
```
# 4K ===
% time pg_restore -d evantest -t tbl3 db.dump
pg_restore -d evantest -t tbl3 db.dump  0.06s user 0.04s system 6% cpu 1.528 total

# 128K
% time pg_restore -d evantest -t tbl3 db.dump
pg_restore -d evantest -t tbl3 db.dump  0.05s user 0.04s system 3% cpu 2.146 total
```

The other thing I noticed is that, when I do custom-format dump, if a target file exists, pg_dump will just go ahead overwrite the existing file; however, when I do directory dump, if a target dir exists, pg_dump will fail with an error “directory xxx is not empty”. Why the behaviors are different?

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  32. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-14T02:44:45Z

    Chao Li <li.evan.chao@gmail.com> writes:
>> On Oct 14, 2025, at 08:36, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The thing we are really interested in here is how fast pg_restore
>> can skip over unwanted table data in a large archive file, and that
>> I believe should be pretty sensitive to block size.

> Not sure if I did something wrong, but I still don’t see much difference between buffer size 4K and 128K with your suggested test.
>
> % time pg_dump -Fc -f db.dump evantest

This won't show the effect, because pg_dump will be able to go back
and insert data offsets into the dump's TOC, so pg_restore can just
seek to where the data is.  See upthread discussion about what's
needed to provoke Dimitrios' problem.

I tried this very tiny (relatively speaking) test case:

regression=# create database d1;
CREATE DATABASE
regression=# \c d1
You are now connected to database "d1" as user "postgres".
d1=# create table alpha as select repeat(random()::text, 1000) from generate_series(1,1000000);
SELECT 1000000
d1=# create table omega as select 42 as x;
SELECT 1
d1=# \q

Then

$ pg_dump -Fc d1 | cat >d1.dump
$ time pg_restore -f /dev/null -t omega d1.dump

The point of the pipe-to-cat is to reproduce Dimitrios' problem case
with no data offsets in the TOC.  Then the restore is doing about the
simplest thing I can think of to make it skip over most of the archive
file.  Also, I'm intentionally using the default choice of gzip
because that already responds to DEFAULT_IO_BUFFER_SIZE properly.
(This test is with current HEAD, no patches except adjusting
DEFAULT_IO_BUFFER_SIZE.)

I got these timings:

DEFAULT_IO_BUFFER_SIZE = 1K
real    0m0.020s
user    0m0.002s
sys     0m0.017s

DEFAULT_IO_BUFFER_SIZE = 4K
real    0m0.014s
user    0m0.003s
sys     0m0.011s

DEFAULT_IO_BUFFER_SIZE = 128K
real    0m0.002s
user    0m0.000s
sys     0m0.002s

This test case has only about 50MB worth of compressed data,
so of course the times are very small; scaling it up to
gigabytes would yield more impressive results.  But the
effect is clearly visible.

			regards, tom lane

  33. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Chao Li <li.evan.chao@gmail.com> — 2025-10-14T05:36:07Z

    
> On Oct 14, 2025, at 10:44, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> This won't show the effect, because pg_dump will be able to go back
> and insert data offsets into the dump's TOC, so pg_restore can just
> seek to where the data is.  See upthread discussion about what's
> needed to provoke Dimitrios' problem.
> 
> I tried this very tiny (relatively speaking) test case:
> 
> regression=# create database d1;
> CREATE DATABASE
> regression=# \c d1
> You are now connected to database "d1" as user "postgres".
> d1=# create table alpha as select repeat(random()::text, 1000) from generate_series(1,1000000);
> SELECT 1000000
> d1=# create table omega as select 42 as x;
> SELECT 1
> d1=# \q
> 
> Then
> 
> $ pg_dump -Fc d1 | cat >d1.dump
> $ time pg_restore -f /dev/null -t omega d1.dump
> 
> The point of the pipe-to-cat is to reproduce Dimitrios' problem case
> with no data offsets in the TOC.  Then the restore is doing about the
> simplest thing I can think of to make it skip over most of the archive
> file.  Also, I'm intentionally using the default choice of gzip
> because that already responds to DEFAULT_IO_BUFFER_SIZE properly.
> (This test is with current HEAD, no patches except adjusting
> DEFAULT_IO_BUFFER_SIZE.)
> 
> I got these timings:
> 
> DEFAULT_IO_BUFFER_SIZE = 1K
> real    0m0.020s
> user    0m0.002s
> sys     0m0.017s
> 
> DEFAULT_IO_BUFFER_SIZE = 4K
> real    0m0.014s
> user    0m0.003s
> sys     0m0.011s
> 
> DEFAULT_IO_BUFFER_SIZE = 128K
> real    0m0.002s
> user    0m0.000s
> sys     0m0.002s
> 
> This test case has only about 50MB worth of compressed data,
> so of course the times are very small; scaling it up to
> gigabytes would yield more impressive results.  But the
> effect is clearly visible.
> 

With your example, I can now see the difference, however, I had to create 5 more times of rows in the first table:

```
evantest=# CREATE TABLE alpha AS SELECT repeat(random()::text, 1000) FROM generate_series(1, 5000000);
SELECT 5000000
evantest=#
evantest=# CREATE TABLE omega AS SELECT 42 AS x;
SELECT 1
```

My test is with the patch, I only adjusted DEFAULT_IO_BUFFER_SIZE.

DEFAULT_IO_BUFFER_SIZE=4K
```
% /usr/bin/time pg_dump -Fc evantest | cat > d1.dump
      294.83 real       220.28 user        45.90 sys

% /usr/bin/time pg_restore -f /dev/null -t omega d1.dump
        0.16 real         0.02 user         0.09 sys
```

DEFAULT_IO_BUFFER_SIZE=128K
```
% /usr/bin/time pg_dump -Fc evantest | cat > d1.dump
      296.89 real       220.85 user        46.64 sys

% /usr/bin/time pg_restore -f /dev/null -t omega d1.dump
        0.01 real         0.00 user         0.00 sys
```

With bigger blocker size, pg_restore skips less blocks, so it gets faster.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  34. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-15T19:21:50Z

    I wrote:
> 0004 increases the row width in the existing test case that says
> it's trying to push more than DEFAULT_IO_BUFFER_SIZE through
> the compressors.  While I agree with the premise, this solution
> is hugely expensive: it adds about 12% to the already-long runtime
> of 002_pg_dump.pl.  I'd like to find a better way, but ran out of
> energy for today.  (I think the reason this costs so much is that
> it's effectively iterated hundreds of times because of
> 002_pg_dump.pl's more or less cross-product approach to testing
> everything.  Maybe we should pull it out of that structure?)

The attached patchset accomplishes that by splitting 002_pg_dump.pl
into two scripts, one that is just concerned with the compression
test cases and one that does everything else.  This might not be
the prettiest solution, since it duplicates a lot of perl code.
I thought about refactoring 002_pg_dump.pl so that it could handle
two separate sets of runs-plus-tests, but decided it was overly
complicated already.

Anyway, 0001 attached is the same as in v4, 0002 performs the
test split without intending to change coverage, and then 0003
adds the new test cases I wanted.  For me, this ends up with
just about the same runtime as before, or maybe a smidge less.
I'd hoped for possibly more savings than that, but I'm content
with it being a wash.

I think this is more or less committable, and then we could get
back to the original question of whether it's worth tweaking
pg_restore's seek-vs-scan behavior.

			regards, tom lane

  35. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-16T17:01:10Z

    I wrote:
> I think this is more or less committable, and then we could get
> back to the original question of whether it's worth tweaking
> pg_restore's seek-vs-scan behavior.

And done.  Dimitrios, could you re-do your testing against current
HEAD, and see if there's still a benefit to tweaking pg_restore's
seek-vs-read decisions, and if so what's the best number?

			regards, tom lane

  36. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Dimitrios Apostolou <jimis@gmx.net> — 2025-10-16T18:59:05Z

    Thank you for your work on this, Tom.
I'll try to test it in the weekend. 

Dimitris

On 16 October 2025 19:01:10 CEST, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>I wrote:
>> I think this is more or less committable, and then we could get
>> back to the original question of whether it's worth tweaking
>> pg_restore's seek-vs-scan behavior.
>
>And done.  Dimitrios, could you re-do your testing against current
>HEAD, and see if there's still a benefit to tweaking pg_restore's
>seek-vs-read decisions, and if so what's the best number?
>
>			regards, tom lane

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

    Dimitrios Apostolou <jimis@gmx.net> — 2025-10-20T18:40:27Z

    On Thursday 2025-10-16 19:01, Tom Lane wrote:

>> I think this is more or less committable, and then we could get
>> back to the original question of whether it's worth tweaking
>> pg_restore's seek-vs-scan behavior.
>
> And done.  Dimitrios, could you re-do your testing against current
> HEAD, and see if there's still a benefit to tweaking pg_restore's
> seek-vs-read decisions, and if so what's the best number?

Sorry for the delay, I hadn't realized a needed to generate a new 
database dump using the current HEAD. So I did that, using 
--compress=none and storing it on compressed btrfs filesystem, since 
that's my primary use case.

I notice that things have improved immensely!
Using the test you suggested (see NOTE1):

     pg_restore -t last_table -f /dev/null  huge.pg_dump


1. The strace output is much more reasonable now; basically it's
    repeating the pattern

        read(4k)
        lseek(~128k forward)

   As a reminder, with old archives it was repeating the pattern:

        read(4k)
        lseek(4k forward)
        lseek(same offset as above) x ~80 times

2. The IO speed is better than before:

       On my 20TB HDD I get 30-50 MB/s read rate.

       With old archives I get 10-20 MB/s read rate.

3. Time to complete: ~25 min

4. CPU usage is low. With old archives the pg_restore process shows
    high *system* CPU (because of the amount of syscalls).


I can't really compare the actual runtime between old and new dump, 
because the two dumps are very different. But I have no doubt the new 
dump is several times faster to seek through.


NOTE1: My original testcase was

           pg_restore -t last_table -j $NCPU -d testdb

        This testcase does not show as big improvement,
        because every single of the parallel workers is
        concurrently seeking through the dump file.



*** All above was measured from master branch HEAD **
277dec6514728e2d0d87c1279dd5e0afbf897428
Don't rely on zlib's gzgetc() macro.

*** Below I have applied attached patch ***


Regarding the attached patch (rebased and edited commit message), it 
basically replaces seek(up to 1MB forward) with read(). The 1MB number 
comes a bit out of the top of my head. But tweaking it between 128KB and 
1MB wouldn't really change anything, given that the block size is now 
128KB: The read() will always be chosen against the seek(). Do you know 
of a real-world case with block sizes >128KB?

Anyway I tried it with the new archive from above.


1. strace output is a loop of the following:

         read(4k)
         read(~128k)

2. Read rate is between 150-250MB/s basically max that the HDD can give.

3. Time to complete: ~5 min

4. CPU usage: HIGH (63%), most likely because of the sheer amount
    of data it's parsing.


Regards,
Dimitris

  38. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Dimitrios Apostolou <jimis@gmx.net> — 2025-10-20T20:50:44Z

    
  WriteDataToArchiveNone(ArchiveHandle *AH, CompressorState *cs,
                                            const void *data, size_t dLen)
  {
-       cs->writeF(AH, data, dLen);
+       NoneCompressorState *nonecs = (NoneCompressorState *) cs->private_data;
+       size_t          remaining = dLen;
+
+       while (remaining > 0)
+       {
+               size_t          chunk;
+
+               /* Dump buffer if full */
+               if (nonecs->bufdata >= nonecs->buflen)

Shouldn't this be equality check instead:
     if (nonecs->bufdata == nonecs->buflen)

And possibly also assert(nonecs->bufdata <= nonecs->buflen) ?

+               {
+                       cs->writeF(AH, nonecs->buffer, nonecs->bufdata);
+                       nonecs->bufdata = 0;
+               }
+               /* And fill it */
+               chunk = nonecs->buflen - nonecs->bufdata;
+               if (chunk > remaining)
+                       chunk = remaining;
+               memcpy(nonecs->buffer + nonecs->bufdata, data, chunk);
+               nonecs->bufdata += chunk;
+               data = ((const char *) data) + chunk;
+               remaining -= chunk;
+       }
  }

  39. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Dimitrios Apostolou <jimis@gmx.net> — 2025-10-20T21:12:35Z

    On Wednesday 2025-10-15 21:21, Tom Lane wrote:

>> 0004 increases the row width in the existing test case that says
>> it's trying to push more than DEFAULT_IO_BUFFER_SIZE through
>> the compressors.  While I agree with the premise, this solution
>> is hugely expensive: it adds about 12% to the already-long runtime
>> of 002_pg_dump.pl.  I'd like to find a better way, but ran out of
>> energy for today.  (I think the reason this costs so much is that
>> it's effectively iterated hundreds of times because of
>> 002_pg_dump.pl's more or less cross-product approach to testing
>> everything.  Maybe we should pull it out of that structure?)
>
> The attached patchset accomplishes that by splitting 002_pg_dump.pl
> into two scripts, one that is just concerned with the compression
> test cases and one that does everything else.  This might not be
> the prettiest solution, since it duplicates a lot of perl code.
> I thought about refactoring 002_pg_dump.pl so that it could handle
> two separate sets of runs-plus-tests, but decided it was overly
> complicated already.
>
> Anyway, 0001 attached is the same as in v4, 0002 performs the
> test split without intending to change coverage, and then 0003
> adds the new test cases I wanted.  For me, this ends up with
> just about the same runtime as before, or maybe a smidge less.
> I'd hoped for possibly more savings than that, but I'm content
> with it being a wash.
>
> I think this is more or less committable, and then we could get
> back to the original question of whether it's worth tweaking
> pg_restore's seek-vs-scan behavior.


Hi Tom, since you are dealing with pg_restore testing, you might want to 
have a look in the 2nd patch from here:

https://www.postgresql.org/message-id/413c1cd8-1d6d-90ba-ac7b-b226a4dad5ed%40gmx.net

Direct link to the patch is:

https://www.postgresql.org/message-id/attachment/177661/v3-0002-Add-new-test-file-with-pg_restore-test-cases.patch


It's a much shorter test, focused on pg_restore.

1. It generates two custom-format dumps (with-TOC and TOC-less).

2. Restores each dump to an empty database using pg_restore with
    a couple of switches combinations
    (one combination (--clean --data-only will not work without a patch
     of mine so we might want to remove that and enrich with others).

3. Tests pg_restore over pre-existing database

4. Tests pg_restore reading file from stdin.


Regards,
Dimitris

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-20T21:21:16Z

    Dimitrios Apostolou <jimis@gmx.net> writes:
> Regarding the attached patch (rebased and edited commit message), it 
> basically replaces seek(up to 1MB forward) with read(). The 1MB number 
> comes a bit out of the top of my head. But tweaking it between 128KB and 
> 1MB wouldn't really change anything, given that the block size is now 
> 128KB: The read() will always be chosen against the seek(). Do you know 
> of a real-world case with block sizes >128KB?

Yeah, with the recent changes I'd expect table data to pretty much
always consist of blocks around 128K, unless the table is smaller
than that of course.

I experimented with this patch locally and came away not too
impressed; it seems the results may be highly platform-dependent.

In the interests of having a common benchmark case that's easy to
replicate, here's precisely what I did:

Use non-assert build of current HEAD (4bea91f21 at the moment).

$ createdb bench
$ time pgbench -i -s 10000 bench

real    14m40.474s
user    1m26.717s
sys     0m5.045s
$ psql bench
...
bench=# create table zedtable(f1 int);
CREATE TABLE
bench=# insert into zedtable values(42);
INSERT 0 1
bench=# \q
$ time pg_dump -Fc --compress=none bench | cat >bench10000.dump

real    7m48.969s
user    0m36.334s
sys     1m35.209s

(At this -s value, the database occupies about 147G and the dump
file about 95G.  It's important the dump file not fit in RAM.)

$ time pg_restore -f /dev/null -t zedtable bench10000.dump

real    1m12.646s
user    0m0.355s
sys     0m5.083s

This compares rather favorably to "cat":

$ time cat bench10000.dump >/dev/null

real    3m6.627s
user    0m0.167s
sys     0m30.999s

I then applied your patch and repeated the restore run:

$ time pg_restore -f /dev/null -t zedtable bench10000.dump

real    2m39.138s
user    0m0.386s
sys     0m28.493s

So for me, the proposed patch actually makes it 2X slower.

Watching it with "iostat 1", I'm seeing about 40MB/s disk read
rate with HEAD, and 500MB/s with the patch; "cat" also shows
read rate around 500MB/s.  So yeah, we can saturate the disk
interface by doing all reads and no seeks, but that doesn't
net out faster.

I did this on a few-years-old Dell Precision 5820 workstation.
The specs for it are a bit vague about the disk subsystem:
  Storage Drive Controllers
  Integrated Intel AHCI SATA chipset controller (8x 6.0Gb/s), SW RAID 0,1,5,10
  Storage Drive
  2.5 1.92TB SATA AG Enterprise Solid State Drive
and hdparm isn't enormously helpful either:

ATA device, with non-removable media
	Model Number:       SSDSC2KB019T8R                          
	Serial Number:      PHYF1291017A1P9DGN  
	Firmware Revision:  XCV1DL69
	Media Serial Num:   
	Media Manufacturer: 
	Transport:          Serial, ATA8-AST, SATA 1.0a, SATA II Extensions, SATA Rev 2.5, SATA Rev 2.6, SATA Rev 3.0
Standards:
	Used: unknown (minor revision code 0x006d) 
	Supported: 10 9 8 7 6 5 
	Likely used: 10

I'm running RHEL 8.10, file system is xfs.

So I find this a bit discouraging.  It's not clear why you're seeing a
win and I'm not, and it's even less clear whether there'd be enough
of a win across enough platforms to make it worth trying to engineer
a solution that helps many more people than it hurts.

			regards, tom lane

  41. Re: [PING] [PATCH v2] parallel pg_restore: avoid disk seeks when jumping short distance forward

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-20T21:29:22Z

    Dimitrios Apostolou <jimis@gmx.net> writes:
> +               /* Dump buffer if full */
> +               if (nonecs->bufdata >= nonecs->buflen)

> Shouldn't this be equality check instead:
>      if (nonecs->bufdata == nonecs->buflen)

Old defensive-programming habit.  The invariant we want to establish
is that there's some space available, ie
	nonecs->bufdata < nonecs->buflen
and if we just test for equality then we haven't proven that.
Agreed that bufdata shouldn't ever be greater than buflen, but if
it somehow is, an equality test here would contribute to making
things worse (writing ever further past the buffer) not better.

> And possibly also assert(nonecs->bufdata <= nonecs->buflen) ?

Maybe, but that code is simple enough that I didn't see a big
need for an assertion check.

			regards, tom lane

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

    Dimitrios Apostolou <jimis@gmx.net> — 2025-10-20T22:09:46Z

    Thanks for the extensive testing! Did you see the same syscall pattern in strace output, as I did?

If yes, then the only reason I can think of that excuses the regression with my patch is that the SATA interface was maxed out when reading sequentially, while the very short latency of SSDs guarantees thousands of seek() operations per second.

I was using an HDD, and in older measurements I was using a VM with mounted volume over iSCSI. The first imposes physical limits in the amount of seeks, and the second network round-trip limits.

So you are right, it's probably very platform dependent, and the most important fix was to enlarge the underlying block size, that you have done.


Dimitris

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-20T22:15:13Z

    I 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.

			regards, tom lane

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-20T22:23:27Z

    Dimitrios Apostolou <jimis@gmx.net> writes:
> Thanks for the extensive testing! Did you see the same syscall pattern in strace output, as I did?

Yes, I did look at that, and it's the same as you saw:
HEAD repeats

        read(4k)
        lseek(~128k forward)

which is to be expected if we have to read data block headers
that are ~128K apart; while patched repeats

         read(4k)
         read(~128k)

which is a bit odd in itself, why isn't it merging the reads better?

> I was using an HDD,

Ah.  Your original message mentioned NVMe so I was assuming you
were also looking at solid-state drives.  I can imagine that
seeking is more painful on HDDs ...

			regards, tom lane

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

    Dimitrios Apostolou <jimis@gmx.net> — 2025-10-21T13:57:31Z

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

> HEAD repeats
>
>        read(4k)
>        lseek(~128k forward)
>
> which is to be expected if we have to read data block headers
> that are ~128K apart; while patched repeats
>
>         read(4k)
>         read(~128k)
>
> which is a bit odd in itself, why isn't it merging the reads better?

The read(4k) happens because of the getc() calls that read the next 
block's length.

As noticed in a message above [1], glibc seems to do 4KB buffering by 
default, for some reason. setvbuf() can mitigate this.

[1] https://www.postgresql.org/message-id/1po8os49-r63o-2923-p37n-12698o1qn7p0%40tzk.arg

I'm attaching a patch that sets glibc buffering to 1MB just as a proof 
of concept. It's obviously WIP, it allocates and never frees. :-)
Feel free to pick it up and change it as you see fit.
With this patch, read() calls are unified in strace. lseeks() remain, 
even if they are not actually reading anything.

It seems to me that glibc could implement an optimisation for fseeko(): 
store the current position in the file, and do not issue the lseek() 
system call if the position does not change.


>> I was using an HDD,
>
> Ah.  Your original message mentioned NVMe so I was assuming you
> were also looking at solid-state drives.  I can imagine that
> seeking is more painful on HDDs ...

Sorry for the confusion, in all this time I've run tests on too many 
different hardware combinations. Not the best way to draw conclusions, 
but it's what I had available at each time.


Dimitris

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

    Dimitrios Apostolou <jimis@gmx.net> — 2025-10-21T14:16:26Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-21T17:36:53Z

    Dimitrios Apostolou <jimis@gmx.net> writes:
> On Tuesday 2025-10-21 00:15, Tom Lane wrote:
>> 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.

Yes, that's a fair case to worry about.

> 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.

I spent some time strace'ing pg_restore on older dump files with
relatively small block sizes.  You are right that glibc seems quite
stupid about this: it appears to issue a kernel lseek() call for every
fseeko(), even though it keeps using the same 4K worth of data it had
previously read in.  I thought maybe that was an artifact of the
relatively old glibc in RHEL8, but glibc 2.40 from Fedora 41 is no
better.  I also checked current macOS, and it's marginally smarter:
it issues just one seek call per read call.  But that seek is still
useless, since the read is still 4K adjacent to the previous 4K.

(I wonder if maybe there is some POSIX standards compliance issue
involved here?  We don't care about the possibility that the disk
file is changing under us, but other applications do, and maybe
the kernel-visible seek calls are required for some reason.)

Putting in a minimum-block-size-to-seek check gets rid of the seek
calls entirely, producing a straight stream of reads, on all three
platforms.  I agree that doing this with a threshold of 4K seems
like a no-brainer, since that seems to be the common default stdio
buffer size.  Using a larger threshold, or setting up a larger buffer,
seems to risk platform-dependent results, so it would require more
performance testing than I care to do now.  Let's do 4K and call it
a day.

			regards, tom lane