Re: Refactor backup related code (was: Is it correct to say, "invalid data in file \"%s\"", BACKUP_LABEL_FILE in do_pg_backup_stop?)

At Mon, 26 Sep 2022 11:57:58 +0530, Bharath Rupireddy  wrote in 
> On Mon, Sep 26, 2022 at 8:13 AM Michael Paquier  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.

Doesn't the following (from you :) work?

+ * Returns the result generated as a palloc'd string.

This suggests no need for pfree if the caller properly destroys the
context or pfree is needed otherwise. In this case, the active memory
contexts are "ExprContext" and "Replication command context" so I
think we actually do not need to pfree it but I don't mean we sholnd't
do that in this patch (since those contexts are somewhat remote from
what the function does and pfree doesn't matter at all here.).

> [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)
> {

+1.  A nitpick.

-	if (strcmp(backupfrom, "standby") == 0 && !backup_started_in_recovery)
+	if (state->started_in_recovery == true &&
+		backup_stopped_in_recovery == false)

Using == for Booleans may not be great?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center