From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
cc: Pavel Stehule <pavel.stehule@gmail.com>,
        Amit Langote <amitlangote09@gmail.com>,
        PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: plan cache overhead on plpgsql expression
In-reply-to: <20200326184947.vniqm2kqa4hmv56x@alap3.anarazel.de>
References: 
 <CA+HiwqHuEmzTnYZaJp8D14VCc127dnAKdAWZoV-b6U7oO_oOJg@mail.gmail.com>
 <CA+HiwqEFt+cJUi=DLxmynYwFwWrC3mQMUSMcYefxU-5jgaeWew@mail.gmail.com>
 <CAFj8pRBVeseNF+jDaZOrC0-W540Mx-DAEvELMeveJs10L9i=Rg@mail.gmail.com>
 <CAFj8pRBH2T=7VoTrY7hf5MNUWE45zZpdWWvv+hzEem=Wn3GcNg@mail.gmail.com>
 <1478.1584815045@sss.pgh.pa.us>
 <20200325211825.gf2ta424kwkfghbr@alap3.anarazel.de>
 <15634.1585173110@sss.pgh.pa.us>
 <20200325224903.pwsx4pccyktq7tx6@alap3.anarazel.de>
 <11118.1585231340@sss.pgh.pa.us> <10353.1585247879@sss.pgh.pa.us>
 <20200326184947.vniqm2kqa4hmv56x@alap3.anarazel.de>
Comments: In-reply-to Andres Freund <andres@anarazel.de>
	message dated "Thu, 26 Mar 2020 11:49:47 -0700"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <21694.1585256389.1@sss.pgh.pa.us>
Date: Thu, 26 Mar 2020 16:59:49 -0400
Message-ID: <21695.1585256389@sss.pgh.pa.us>
Precedence: bulk

Andres Freund <andres@anarazel.de> writes:
> On 2020-03-26 14:37:59 -0400, Tom Lane wrote:
>> Testing that reminded me of the other regression test failure I'd seen
>> when I first tried to do it: select_parallel.sql shows a WARNING about
>> a plancache leak in a parallel worker process.  When I looked into the
>> reason for that, it turned out that some cowboy has split
>> XACT_EVENT_COMMIT into XACT_EVENT_COMMIT and
>> XACT_EVENT_PARALLEL_COMMIT (where the latter is used in parallel
>> workers) without bothering to fix the collateral damage to plpgsql.
>> So plpgsql_xact_cb isn't doing any cleanup in parallel workers, and
>> hasn't been for a couple of releases.
>> The bad effects of that are probably limited given that the worker
>> process will exit after committing, but I still think that that part
>> of this patch is a bug fix that needs to be back-patched.

> Ugh. Lucky that we don't register many things inside those resowners.

Yeah.  I spent some time trying to produce a failure this way, and
concluded that it's pretty hard because most of the relevant callbacks
will be run during ExprContext shutdown, which is done during plpgsql
function exit.  In a non-transaction-abort situation, the simple EState
shouldn't have any live ExprContexts left at commit.  I did find a case
where a memory context callback attached to the EState's query context
doesn't get run when expected ... but it still gets run later, when the
whole memory context tree is destroyed.  So I can't demonstrate any
user-visible misbehavior in the core code.  But it still seems like a
prudent idea to back-patch a fix, in case I missed something or there is
some extension that's pushing the boundaries further.  It's definitely
not very cool that we're leaving behind a dangling static pointer to an
EState that won't exist once TopTransactionMemoryContext is gone.

I'll back-patch relevant parts of those comments about DO block
management, too.

			regards, tom lane