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 Mon, Sep 26, 2022 at 8:13 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> What I had at hand seemed fine on a second look, so applied after
> tweaking a couple of comments.  One thing that I have been wondering
> after-the-fact is whether it would be cleaner to return the contents
> of the backup history file or backup_label as a char rather than a
> StringInfo?  This simplifies a bit what the callers of
> build_backup_content() need to do.

+1 because callers don't use returned StringInfo structure outside of
build_backup_content(). The patch looks good to me. I think it will be
good to add a note about the caller freeing up the retired string's
memory [1], just in case.

[1]
 * Returns the result generated as a palloc'd string. It is the caller's
 * responsibility to free the returned string's memory.
 */
char *
build_backup_content(BackupState *state, bool ishistoryfile)
{

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