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/22 16:43, Michael Paquier wrote:
>> Added that part before pg_backup_stop() now where it makes sense with
>> the refactoring.
>
> I have put my hands on 0001, and finished with the attached, that
> includes many fixes and tweaks. Some of the variable renames felt out
> of place, while some comments were overly verbose for their purpose,
> though for the last part we did not lose any information in the last
> version proposed.

Thanks for updating the patch! This looks better to me.

+ MemSet(backup_state, 0, sizeof(BackupState));
+ MemSet(backup_state->name, '\0', sizeof(backup_state->name));

The latter MemSet() is not necessary because the former already
resets that with zero, is it?

+ pfree(tablespace_map);
+ tablespace_map = NULL;
+ }
+
+ tablespace_map = makeStringInfo();

tablespace_map doesn't need to be reset to NULL here.

/* Free structures allocated in TopMemoryContext */
- pfree(label_file->data);
- pfree(label_file);
<snip>
+ pfree(backup_label->data);
+ pfree(backup_label);
+ backup_label = NULL;

This source comment is a bit misleading, isn't it? Because the memory
for backup_label is allocated under the memory context other than
TopMemoryContext.

+#include "access/xlog.h"
+#include "access/xlog_internal.h"
+#include "access/xlogbackup.h"
+#include "utils/memutils.h"

Seems "utils/memutils.h" doesn't need to be included.

+ XLByteToSeg(state->startpoint, stopsegno, wal_segment_size);
+ XLogFileName(stopxlogfile, state->starttli, stopsegno, wal_segment_size);
+ appendStringInfo(result, "STOP WAL LOCATION: %X/%X (file %s)\n",
+ LSN_FORMAT_ARGS(state->startpoint), stopxlogfile);

state->stoppoint and state->stoptli should be used instead of
state->startpoint and state->starttli?

+ pfree(tablespace_map);
+ tablespace_map = NULL;
+ pfree(backup_state);
+ backup_state = NULL;

It's harmless to set tablespace_map and backup_state to NULL after pfree(),
but it's also unnecessary at least here.

Regards,

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