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 Wed, Sep 21, 2022 at 05:10:39PM +0530, Bharath Rupireddy wrote:
>> deallocate_backup_variables() is the only part of xlogbackup.c that
>> includes references of the tablespace map_and backup_label
>> StringInfos. I would be tempted to fully decouple that from
>> xlogbackup.c/h for the time being.
>
> There's no problem with it IMO, after all, they are backup related
> variables. And that function reduces a bit of duplicate code.

Hmm. I'd like to disagree with this statement :)

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

As I suspected, the deallocate routine felt unnecessary, as
xlogbackup.c/h have no idea what these are. The remark is
particularly true for the StringInfo of the backup_label file: for
basebackup.c we need to build it when sending base.tar and in
xlogfuncs.c we need it only at the backup stop phase. The code was
actually a bit wrong, because free-ing StringInfos requires to free
its ->data and then the main object (stringinfo.h explains that). My
tweaks have shaved something like 10%~15% of the patch, while making
it IMO more readable.

A second issue I had was with the build function, and again it seemed
much cleaner to let the routine do the makeStringInfo() and return the
result. This is not the most popular routine ever, but this reduces
the workload of the caller of build_backup_content().

So, opinions?
--
Michael