Re: Xact end leaves CurrentMemoryContext = TopMemoryContext

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