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 Tue, Sep 27, 2022 at 1:54 PM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)com> wrote:
>
> This commit introduced BackupState struct. The comment of
> do_pg_backup_start says that:
>
> > * It fills in backup_state with the information required for the backup,
>
> And the parameters are:
>
> > do_pg_backup_start(const char *backupidstr, bool fast, List **tablespaces,
> > BackupState *state, StringInfo tblspcmapfile)
>
> So backup_state is different from both the type BackupState and the
> parameter state. I find it annoying. Don't we either rename the
> parameter or fix the comment?
>
> The parameter "state" sounds a bit too generic. So I prefer to rename
> the parameter to backup_state, as the attached.
>
> What do you think about this?

-1 from me. We have the function context and the structure name there
to represent that the parameter name 'state' is actually 'backup
state'. I don't think we gain anything here. Whenever the BackupState
is used away from any function, the variable name backup_state is
used, static variable in xlogfuncs.c

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