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 Thu, Sep 22, 2022 at 3:17 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
> +               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?

Yes, earlier, the name was a palloc'd string but now it is a fixed
array. Removed.

> +               pfree(tablespace_map);
> +               tablespace_map = NULL;
> +       }
> +
> +       tablespace_map = makeStringInfo();
>
> tablespace_map doesn't need to be reset to NULL here.
>
> +               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.

Yes. But we can retain it for the sake of consistency with the other
places and avoid dangling pointers, if at all any new code gets added
in between it will be useful.

>         /* 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.

Yes, we can just say /* Deallocate backup-related variables. */. The
pg_backup_start() has the info about the variables being allocated in
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.

Yes, removed now.

> +               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?

Nice catch! Corrected.

PSA v12 patch with the above review comments addressed.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com