On Wed, Oct 19, 2022 at 3:04 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
> > Echoing with what I mentioned upthread in [1], I don't quite
> > understand why this patch needs to touch basebackup.c, walsender.c
> > and postgres.c. In the case of a replication command processed by a
> > WAL sender, memory allocations happen in the memory context created
> > for replication commands, which is itself, as far as I understand, the
> > message memory context when we get a 'Q' message for a simple query.
> > Why do we need more code for a cleanup that should be already
> > happening? Am I missing something obvious?
>
> My bad, I missed that. You are right. We have "Replication command
> context" as a child of "MessageContext" memory context for base backup
> that gets cleaned upon error in PostgresMain() [1].
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.
But if we want to fix it, I think we should do it in some more
localized way. 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.
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. A
third option is to do something useful inside WalSndErrorCleanup() or
WalSndResourceCleanup() as I suggested previously.
I'm not exactly sure what the right solution is here, but I think you
need to put more thought into how to make the code look simple,
elegant, and non-invasive.
--
Robert Haas
EDB: http://www.enterprisedb.com
