Re: Refactor backup related code (was: Is it correct to say, "invalid data in file \"%s\"", BACKUP_LABEL_FILE in do_pg_backup_stop?)

On 2022/09/17 16:18, Bharath Rupireddy wrote:
> Good idea. It makes a lot more sense to me, because xlog.c is already
> a file of 9000 LOC. I've created xlogbackup.c/.h files and added the
> new code there. Once this patch gets in, I can offer my hand to move
> do_pg_backup_start() and do_pg_backup_stop() from xlog.c and if okay,
> pg_backup_start() and pg_backup_stop() from xlogfuncs.c to
> xlogbackup.c/.h. Then, we might have to create new get/set APIs for
> XLogCtl fields that do_pg_backup_start() and do_pg_backup_stop()
> access.

The definition of SessionBackupState enum type also should be in xlogbackup.h?

> We need to carry tablespace_map contents from do_pg_backup_start()
> till pg_backup_stop(), backup_label and history_file too are easy to
> carry across. Hence it will be good to have all of them i.e.
> tablespace_map, backup_label and history_file in the BackupState
> structure. IMO, this way is good.

backup_label and history_file are not carried between pg_backup_start()
and _stop(), so don't need to be saved in BackupState. Their contents
can be easily created from other saved fields in BackupState,
if necessary. So at least for me it's better to get rid of them from
BackupState and don't allocate TopMemoryContext memory for them.

>> Something unrelated to your patch that I am noticing while scanning
>> the area is that we have been always lazy in freeing the label file
>> data allocated in TopMemoryContext when using the SQL functions if the
>> backup is aborted. We are not talking about this much amount of
>> memory each time a backup is performed, but that could be a cause for
>> memory bloat in a server if the same session is used and backups keep
>> failing, as the data is freed only on a successful pg_backup_stop().
>> Base backups through the replication protocol don't care about that as
>> the code keeps around the same pointer for the whole duration of
>> perform_base_backup(). Trying to tie the cleanup of the label file
>> with the abort phase would be the cause of more complications with
>> do_pg_backup_stop(), and xlog.c has no need to know about that now.
>> Just a remark for later.
>
> Yeah, I think that can be solved by passing in backup_state to
> do_pg_abort_backup(). If okay, I can work on this too as 0002 patch in
> this thread or we can discuss this separately to get more attention
> after this refactoring patch gets in.

Or, to avoid such memory bloat, how about allocating the memory for
backup_state only when it's NULL?

> I'm attaching v5 patch with the above review comments addressed,
> please review it further.

Thanks for updating the patch!

+ char startxlogfile[MAXFNAMELEN_BACKUP]; /* backup start WAL file */
<snip>
+ char stopxlogfile[MAXFNAMELEN_BACKUP]; /* backup stop WAL file */

These file names seem not necessary in BackupState because they can be
calculated from other fields like startpoint and starttli, etc when
making backup_label and history file contents. If we remove them from
BackupState, we can also remove the definition of MAXFNAMELEN_BACKUP
macro from xlogbackup.h.

+ /* construct backup_label contents */
+ build_backup_content(state, false);

In basebackup case, build_backup_content() is called unnecessary twice
because do_pg_stop_backup() and its caller, perform_base_backup() call
that. This makes me think that it's better to get rid of the call to
build_backup_content() from do_pg_backup_stop(). Instead its callers,
perform_base_backup() and pg_backup_stop() should call that.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION