Re: Avoid memory leaks during base backups

At Thu, 20 Oct 2022 19:47:07 +0200, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote in 
> I agree that's a good idea, and the patch looks good to me, but I don't
> think asserting that they are null afterwards is useful.

+1 for this direction. And the patch is fine to me.


>    oldcontext = MemoryContextSwitchTo(backupcontext);
>    Assert(backup_state == NULL);
>    Assert(tablespace_map == NULL);
>    backup_state = (BackupState *) palloc0(sizeof(BackupState));
>    tablespace_map = makeStringInfo();
>    MemoryContextSwitchTo(oldcontext);

We can use MemoryContextAllocZero() for this purpose, but of couse not
mandatory.


+     * across. We keep the memory allocated in this memory context less,
+     * because any error before reaching pg_backup_stop() can leak the memory
+     * until pg_backup_start() is called again. While this is not smart, it
+     * helps to keep things simple.

I think the "less" is somewhat obscure.  I feel we should be more
explicitly. And we don't need to put emphasis on "leak".  I recklessly
propose this as the draft.

"The context is intended to be used by this function to store only
session-lifetime values.  It is, if left alone, reset at the next call
to blow away orphan memory blocks from the previous failed call.
While this is not smart, it helps to keep things simple."

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center