Horton

Thread

Commits

  1. Split vacuumdb to create vacuuming.c/h

  2. Preserve CurrentMemoryContext across notify and sinval interrupts.

  3. Preserve CurrentMemoryContext across Start/CommitTransactionCommand.

  1. Xact end leaves CurrentMemoryContext = TopMemoryContext

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-06-17T20:37:05Z

    AtCommit_Memory and friends have done $SUBJECT for at least a couple
of decades, but in the wake of analyzing bug #18512 [1], I'm feeling
like that's a really bad idea.  There is too much code running
around the system that assumes that it's fine to leak stuff in
CurrentMemoryContext.  If we execute any such thing between
AtCommit_Memory and the next AtStart_Memory, presto: we have a
session-lifespan memory leak.  I'm almost feeling that we should
have a policy that CurrentMemoryContext should never point at
TopMemoryContext.

As to what to do about it: I'm imagining that instead of resetting
CurrentMemoryContext to TopMemoryContext, we set it to some special
context that we expect we can reset every so often, like at the start
of the next transaction.  The existing TransactionAbortContext is
a very similar thing, and maybe could be repurposed/shared with this
usage.

Thoughts?

			regards, tom lane

[1] https://www.postgresql.org/message-id/18512-6e89f654d7da884d%40postgresql.org

  2. Re: Xact end leaves CurrentMemoryContext = TopMemoryContext

    Andres Freund <andres@anarazel.de> — 2024-06-17T21:43:05Z

    Hi,

On 2024-06-17 16:37:05 -0400, Tom Lane wrote:
> As to what to do about it: I'm imagining that instead of resetting
> CurrentMemoryContext to TopMemoryContext, we set it to some special
> context that we expect we can reset every so often, like at the start
> of the next transaction.  The existing TransactionAbortContext is
> a very similar thing, and maybe could be repurposed/shared with this
> usage.

One issue is that that could lead to hard to find use-after-free issues in
currently working code. Right now allocations made "between transactions"
live forever, if we just use a different context it won't anymore.

Particularly if the reset is only occasional, we'd make it hard to find
buggy allocations.

I wonder if we ought to set CurrentMemoryContext to NULL in that timeframe,
forcing code to explicitly choose what lifetime is needed, rather than just
defaulting such code into changed semantics.

Greetings,

Andres Freund

  3. Re: Xact end leaves CurrentMemoryContext = TopMemoryContext

    David Rowley <dgrowleyml@gmail.com> — 2024-06-18T04:34:43Z

    On Tue, 18 Jun 2024 at 08:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> As to what to do about it: I'm imagining that instead of resetting
> CurrentMemoryContext to TopMemoryContext, we set it to some special
> context that we expect we can reset every so often, like at the start
> of the next transaction.  The existing TransactionAbortContext is
> a very similar thing, and maybe could be repurposed/shared with this
> usage.
>
> Thoughts?

Instead, could we just not delete TopTransactionContext in
AtCommit_Memory() and instead do MemoryContextReset() on it? Likewise
in AtCleanup_Memory().

If we got to a stage where we didn't expect anything to allocate into
that context outside of a transaction, we could check if the context
is still reset in AtStart_Memory() and do something like raise a
WARNING on debug builds (or Assert()) to alert us that some code that
broke our expectations.

It might also be a very tiny amount more efficient to not delete the
context so we don't have to fetch a new context from the context
freelist in AtStart_Memory().  Certainly, it wouldn't add any
overhead.  Adding a new special context would and so would the logic
to reset it every so often.

David

  4. Re: Xact end leaves CurrentMemoryContext = TopMemoryContext

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-06-18T04:53:48Z

    David Rowley <dgrowleyml@gmail.com> writes:
> Instead, could we just not delete TopTransactionContext in
> AtCommit_Memory() and instead do MemoryContextReset() on it? Likewise
> in AtCleanup_Memory().

Hmm, that's a nice idea.  Maybe reset again in AtStart_Memory, although
that seems optional.  My first reaction was "what about memory context
callbacks attached to TopTransactionContext?" ... but those are defined
to be fired on either reset or delete, so semantically this seems like
it creates no issues.  And you're right that not constantly deleting
and recreating that context should save some microscopic amount.

> If we got to a stage where we didn't expect anything to allocate into
> that context outside of a transaction, we could check if the context
> is still reset in AtStart_Memory() and do something like raise a
> WARNING on debug builds (or Assert()) to alert us that some code that
> broke our expectations.

My point is exactly that I don't think that we can expect that,
or at least that the cost of guaranteeing it will vastly outweigh
any possible benefit.  (So I wasn't excited about Andres' suggestion.
But this one seems promising.)

I'll poke at this tomorrow, unless you're hot to try it right now.

			regards, tom lane

  5. Re: Xact end leaves CurrentMemoryContext = TopMemoryContext

    David Rowley <dgrowleyml@gmail.com> — 2024-06-18T05:30:21Z

    On Tue, 18 Jun 2024 at 16:53, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'll poke at this tomorrow, unless you're hot to try it right now.

Please go ahead. I was just in suggestion mode here.

David

  6. Re: Xact end leaves CurrentMemoryContext = TopMemoryContext

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-06-18T19:28:03Z

    David Rowley <dgrowleyml@gmail.com> writes:
> On Tue, 18 Jun 2024 at 16:53, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'll poke at this tomorrow, unless you're hot to try it right now.

> Please go ahead. I was just in suggestion mode here.

So I tried that, and while it kind of worked, certain parts of the
system (notably logical replication) had acute indigestion.  Turns
out there is a fair amount of code that does

	StartTransactionCommand();
	... some random thing or other ...
	CommitTransactionCommand();

and does not stop to think at all about whether that has any effect on
its memory context.  Andres' idea would break every single such place,
and this idea isn't much better because while it does provide a
current memory context after CommitTransactionCommand, that context is
effectively short-lived: the next Start/CommitTransactionCommand will
trash it.  That broke a lot more places than I'd hoped, mostly in
obscure ways.

After awhile I had an epiphany: what we should do is make
CommitTransactionCommand restore the memory context that was active
before StartTransactionCommand.  That's what we want in every place
that was cognizant of this issue, and it seems to be the case in every
place that wasn't doing anything explicit about it, either.

The 0001 patch attached does that, and seems to work nicely.
I made it implement the idea of recycling TopTransactionContext,
too.  (Interestingly, src/backend/utils/mmgr/README *already*
claims we manage TopTransactionContext this way.  Did we do that
and then change it back in the mists of time?)  The core parts
of the patch are all in xact.c --- the other diffs are just random
cleanup that I found while surveying use of TopMemoryContext and
CommitTransactionCommand.

Also, 0002 is what I propose for the back branches.  It just adds
memory context save/restore in notify interrupt processing to solve
the original bug report, as well as in sinval interrupt processing
which I discovered has the same disease.  We don't need this in
HEAD if we apply 0001.

At this point I'd be inclined to wait for the branch to be made,
and then apply 0001 in HEAD/v18 only and 0002 in v17 and before.
While 0001 seems fairly straightforward, it's still a nontrivial
change and I'm hesitant to shove it in at this late stage of the
v17 cycle.

			regards, tom lane

  7. Re: Xact end leaves CurrentMemoryContext = TopMemoryContext

    Andres Freund <andres@anarazel.de> — 2024-06-18T20:23:36Z

    Hi,

On 2024-06-18 15:28:03 -0400, Tom Lane wrote:
> After awhile I had an epiphany: what we should do is make
> CommitTransactionCommand restore the memory context that was active
> before StartTransactionCommand.  That's what we want in every place
> that was cognizant of this issue, and it seems to be the case in every
> place that wasn't doing anything explicit about it, either.

I like it.

I wonder if there's an argument the "persistent" TopTransactionContext should
live under a different name outside of transactions, to avoid references to it
working in a context where it's not valid?   It's probably not worth it, but
not sure.


> The 0001 patch attached does that, and seems to work nicely.
> I made it implement the idea of recycling TopTransactionContext,
> too

Nice.

I think there might be some benefit to doing that for some more things,
later/separately. E.g. the allocation of TopTransactionResourceOwner shows up
in profiles for workloads with small transactions. Which got a bit worse with
17 (largely bought back in other places by the advantages of the new resowner
system).



> At this point I'd be inclined to wait for the branch to be made,
> and then apply 0001 in HEAD/v18 only and 0002 in v17 and before.
> While 0001 seems fairly straightforward, it's still a nontrivial
> change and I'm hesitant to shove it in at this late stage of the
> v17 cycle.

Seems reasonable.


Greetings,

Andres Freund

  8. Re: Xact end leaves CurrentMemoryContext = TopMemoryContext

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-06-18T20:48:18Z

    Andres Freund <andres@anarazel.de> writes:
> I wonder if there's an argument the "persistent" TopTransactionContext should
> live under a different name outside of transactions, to avoid references to it
> working in a context where it's not valid?   It's probably not worth it, but
> not sure.

Hm.  We could stash the long-lived pointer in a static variable,
and only install it in TopTransactionContext when you're supposed
to use it.  I tend to agree not worth it, though.

			regards, tom lane

  9. Re: Xact end leaves CurrentMemoryContext = TopMemoryContext

    Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> — 2025-03-10T08:41:56Z

    Hi,

This change seems to have introduced an issue where a deleted context
can be restored. This happens when a replication command exports a
snapshot since the transaction used is aborted at the start of the
next command. This leads to a memory context allocated with itself as
a parent and child, triggering an infinite loop when attempting to
delete it. I've written more details in a separate thread[1].

[1]: https://www.postgresql.org/message-id/flat/CAO6_XqoJA7-_G6t7Uqe5nWF3nj+QBGn4F6Ptp=rUGDr0zo+KvA@mail.gmail.com

  10. Re: Xact end leaves CurrentMemoryContext = TopMemoryContext

    pavel seleznev <pavel.seleznev@gmail.com> — 2025-09-26T17:38:43Z

    Hello,

Should we use the same approach on line 1106 (
https://github.com/postgres/postgres/blob/c4067383cb2c155c4cfea2351036709e2ebb3535/src/backend/libpq/hba.c#L1106
)
as on line 266 (
https://github.com/postgres/postgres/blob/c4067383cb2c155c4cfea2351036709e2ebb3535/src/backend/tcop/backend_startup.c#L266
)
since they reference the same structure?

Regards,
Pavel Seleznev

  11. Re: Xact end leaves CurrentMemoryContext = TopMemoryContext

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-09-26T17:53:32Z

    pavel seleznev <pavel.seleznev@gmail.com> writes:
> Hello,
> Should we use the same approach on line 1106 (
> https://github.com/postgres/postgres/blob/c4067383cb2c155c4cfea2351036709e2ebb3535/src/backend/libpq/hba.c#L1106
> )
> as on line 266 (
> https://github.com/postgres/postgres/blob/c4067383cb2c155c4cfea2351036709e2ebb3535/src/backend/tcop/backend_startup.c#L266
> )
> since they reference the same structure?

I see no reason to change either one.  BackendInitialize is
explicitly choosing not to run in TopMemoryContext, which is fine,
but that doesn't mean that other code has to do the same.

			regards, tom lane