On Mon, Sep 26, 2022 at 8:13 AM Michael Paquier <mich...@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
