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