Re: Avoid memory leaks during base backups

On Fri, Oct 21, 2022 at 11:58 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> To be exact, it seems to me that tablespace_map and backup_state
> should be reset before deleting backupcontext, but the reset of
> backupcontext should happen after the fact.
>
> +       backup_state = NULL;
>         tablespace_map = NULL;
> These two in pg_backup_start() don't matter, do they?  They are
> reallocated a couple of lines down.

After all, that is what is being discussed here; what if palloc down
below fails and they're not reset to NULL after MemoryContextReset()?

> +    * across. We keep the memory allocated in this memory context less,
> What does "We keep the memory allocated in this memory context less"
> mean here?

We try to keep it less because we don't want to allocate more memory
and leak it unless pg_start_backup() is called again. Please read the
description. I'll leave it to the committer's discretion whether to
have that part or remove it.

On Fri, Oct 21, 2022 at 12:11 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
>
> +        * 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.

I tried to put it simple, please see the attached v10. I'll leave it
to the committer's discretion for better wording.

On Fri, Oct 21, 2022 at 7:47 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Fri, Oct 21, 2022 at 2:18 AM Michael Paquier <michael@paquier.xyz> wrote:
> > AFAIK, one of the callbacks associated to a memory context could
> > fail, see comments before MemoryContextCallResetCallbacks() in
> > MemoryContextDelete().  I agree that it should not matter here, but I
> > think that it is better to reset the pointers before attempting the
> > deletion of the memory context in this case.
>
> I think this is nitpicking. There's no real danger here, and if there
> were, the error handling would have to take it into account somehow,
> which it doesn't.
>
> I'd probably do it before resetting the context as a matter of style,
> to make it clear that there's no window in which the pointers are set
> but referencing invalid memory. But I do not think it makes any
> practical difference at all.

Please see the attached v10.

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