Re: block-level incremental backup

Hi Jeevan,

I have reviewed the backup part at code level and still looking into the

restore(combine) and functional part of it. But, here are my comments so far:

The patches need rebase.

May be we should rename to something like:

"INCREMENTAL BACKUP START WAL LOCATION" or simply "INCREMENTAL BACKUP START LOCATION"

to make it more intuitive?

File header structure is defined in both the files basebackup.c and

pg_combinebackup.c. I think it is better to move this to replication/basebackup.h.

I think we can avoid having flag isrelfile in sendFile(). 

Something like this:

Also, having isrelfile as part of following condition:

is confusing, because even the relation files in full backup are going to be

backed up by this loop only, but still, the condition reads '(!isrelfile &&...)'.

IMHO, while labels are not advisable in general, it may be better to use a label

here rather than a while(1) loop, so that we can move to the label in case we

want to retry once. I think here it opens doors for future bugs if someone

happens to add code here, ending up adding some condition and then the

break becomes conditional. That will leave us in an infinite loop.

Similar to structure partial_file_header, I think above macro can also be moved

to basebackup.h instead of defining it twice.

I think this is a huge memory request (1GB) and may fail on busy/loaded server at

times. We should check for failures of malloc, maybe throw some error on

getting ENOMEM as errno.

Here, should not we expect statbuf->st_size < (RELSEG_SIZE * BLCKSZ), and it

should be safe to read just statbuf_st_size always I guess? But, I am ok with

having this extra guard here.

In sendFile(), I am sorry if I am missing something, but I am not able to

understand why 'cnt' and 'i' should have different values when they are being

passed to verify_page_checksum(). I think passing only one of them should be

sufficient.

Maybe we should just have a variable no_of_blocks to store a number of blocks,

rather than calculating this say RELSEG_SIZE(i.e. 131072) times in the worst

case.

Sorry if I am missing something, but, should not it be just:

len = cnt;

As I said earlier in my previous email, we now do not need +decode_lsn_internal()

as it is already taken care by the introduction of function pg_lsn_in_internal().

Regards,

Jeevan Ladhe