Xact end leaves CurrentMemoryContext = TopMemoryContext

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

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

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

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

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

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

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

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]/messages/by-id/CAO6_XqoJA7-_G6t7Uqe5nWF3nj+QBGn4F6Ptp=rUGDr0zo+KvA@mail.gmail.com.

[1]: /messages/by-id/CAO6_XqoJA7-_G6t7Uqe5nWF3nj+QBGn4F6Ptp=rUGDr0zo+KvA@mail.gmail.com

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

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