Re: trying again to get incremental backup

On Tue, Sep 12, 2023 at 5:56 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> + BlockNumber relative_block_numbers[RELSEG_SIZE];
>
> This is close to 400kB of memory, so I think it is better we palloc it
> instead of keeping it in the stack.

Fixed.

> Unrelated code refactoring hunk

Fixed.

> This structure is not used anywhere.

Removed.

> /If the file is to be set incrementally/If the file is to be sent incrementally

Fixed.

> I do not really like this change, because after removing this you have
> put 2 independent checks for sending the full file[1] and sending it
> incrementally[1]. Actually for sending incrementally
> 'statbuf->st_size' is computed from the 'num_incremental_blocks'
> itself so why don't we keep this breaking condition in the while loop
> itself? So that we can avoid these two separate conditions.

I don't think that would be correct. The number of bytes that need to
be read from the original file is not equal to the number of bytes
that will be written to the incremental file. Admittedly, they're
currently different by less than a block, but that could change if we
change the format of the incremental file (e.g. suppose we compressed
the blocks in the incremental file with gzip, or smushed out the holes
in the pages). I wrote the loop as I did precisely so that the two
cases could have different loop exit conditions.

> Better we add some comments for these structures.

Done.

Here's a new patch set, also addressing Jakub's observation that
MINIMUM_VERSION_FOR_WAL_SUMMARIES needed updating.

--
Robert Haas
EDB: http://www.enterprisedb.com