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
