On 06.03.24 22:54, Thomas Munro wrote:
> Rebased. I had intended to try to get this into v17, but a couple of
> unresolved problems came up while rebasing over the new incremental
> backup stuff. You snooze, you lose. Hopefully we can sort these out
> in time for the next commitfest:
>
> * should pg_combinebasebackup read the control file to fetch the segment size?
> * hunt for other segment-size related problems that may be lurking in
> new incremental backup stuff
> * basebackup_incremental.c wants to use memory in proportion to
> segment size, which looks like a problem, and I wrote about that in a
> new thread[1]
Overall, I like this idea, and the patch seems to have many bases covered.
The patch will need a rebase. I was able to test it on
master@{2024-03-13}, but after that there are conflicts.
In .cirrus.tasks.yml, one of the test tasks uses
--with-segsize-blocks=6, but you are removing that option. You could
replace that with something like
PG_TEST_INITDB_EXTRA_OPTS='--rel-segsize=48kB'
But that won't work exactly because
initdb: error: argument of --rel-segsize must be a power of two
I suppose that's ok as a change, since it makes the arithmetic more
efficient. But maybe it should be called out explicitly in the commit
message.
If I run it with 64kB, the test pgbench/001_pgbench_with_server fails
consistently, so it seems there is still a gap somewhere.
A minor point, the initdb error message
initdb: error: argument of --rel-segsize must be a multiple of BLCKSZ
would be friendlier if actually showed the value of the block size
instead of just the symbol. Similarly for the nearby error message
about the off_t size.
In the control file, all the other fields use unsigned types. Should
relseg_size be uint64?
PG_CONTROL_VERSION needs to be changed.