Re: Avoid memory leaks during base backups

From: Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Michael Paquier <michael(at)paquier(dot)xyz>, Cary Huang <cary(dot)huang(at)highgo(dot)ca>, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Avoid memory leaks during base backups
Date: 2022-10-19 15:53:51
Message-ID: CALj2ACXK85qiL92=iexHyHiWx96dTBuQRutJxkqZeaGP7uWypw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Wed, Oct 19, 2022 at 8:10 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> Well this still touches postgres.c. And I still think it's an awful
> lot of machinery for a pretty trivial problem. As a practical matter,
> nobody is going to open a connection and sit there and try to start a
> backup over and over again on the same connection. And even if someone
> wrote a client that did that -- why? -- they'd have to be awfully
> persistent to leak any amount of memory that would actually matter. So
> it is not insane to just think of ignoring this problem entirely.

I understand that the amount of memory allocated by pg_backup_start()
is small compared to the overall RAM, however, I don't think we can
ignore the problem and let postgres cause memory leaks.

> But if we want to fix it, I think we should do it in some more
> localized way.

Agreed.

> One option is to just have do_pg_start_backup() blow
> away any old memory context before it allocates any new memory, and
> forget about releasing anything in PostgresMain(). That means memory
> could remain allocated after a failure until you next retry the
> operation, but I don't think that really matters. It's not a lot of
> memory; we just don't want it to accumulate across many repetitions.

This seems reasonable to me.

> Another option, perhaps, is to delete some memory context from within
> the TRY/CATCH block if non-NULL, although that wouldn't work nicely if
> it might blow away the data we need to generate the error message.

Right.

> A third option is to do something useful inside WalSndErrorCleanup() or
> WalSndResourceCleanup() as I suggested previously.

These functions will not be called for SQL-callable backup functions
pg_backup_start() and pg_backup_stop(). And the memory leak problem
we're trying to solve is for SQL-callable functions, but not for
basebaskups as they already have a memory context named "Replication
command context" that gets deleted in PostgresMain().

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2022-10-19 16:23:28 Re: Mingw task for Cirrus CI
Previous Message Ants Aasma 2022-10-19 15:50:09 Standby recovers records from wrong timeline