Re: Refactor backup related code (was: Is it correct to say, "invalid data in file \"%s\"", BACKUP_LABEL_FILE in do_pg_backup_stop?) - Mailing list pgsql-hackers
From | Fujii Masao |
---|---|
Subject | Re: Refactor backup related code (was: Is it correct to say, "invalid data in file \"%s\"", BACKUP_LABEL_FILE in do_pg_backup_stop?) |
Date | |
Msg-id | 14b5dcb8-c9de-99f8-8073-c12b80da4419@oss.nttdata.com Whole thread Raw |
In response to | Re: Refactor backup related code (was: Is it correct to say, "invalid data in file \"%s\"", BACKUP_LABEL_FILE in do_pg_backup_stop?) (Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>) |
Responses |
Re: Refactor backup related code (was: Is it correct to say, "invalid data in file \"%s\"", BACKUP_LABEL_FILE in do_pg_backup_stop?)
|
List | pgsql-hackers |
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
pgsql-hackers by date: