From: Tom Lane <tgl@sss.pgh.pa.us>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
cc: Andres Freund <andres@anarazel.de>,
        PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: memory leak in trigger handling (since PG12)
In-reply-to: <a0f903db-292d-2ec3-79ea-0ab0d63ff3bc@enterprisedb.com>
References: <222a3442-7f7d-246c-ed9b-a76209d19239@enterprisedb.com> <20230523171433.earidmyzock7fnk4@awork3.anarazel.de> <b9f40ba0-6b8b-0a4a-ad10-891e6f41d882@enterprisedb.com> <993814.1684877984@sss.pgh.pa.us> <a0f903db-292d-2ec3-79ea-0ab0d63ff3bc@enterprisedb.com>
Comments: In-reply-to Tomas Vondra <tomas.vondra@enterprisedb.com>
	message dated "Wed, 24 May 2023 10:49:14 +0200"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1140461.1684943844.1@sss.pgh.pa.us>
Date: Wed, 24 May 2023 11:57:24 -0400
Message-ID: <1140462.1684943844@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/1140462.1684943844%40sss.pgh.pa.us>
Precedence: bulk

Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
> On 5/23/23 23:39, Tom Lane wrote:
>> FWIW, I've had some success localizing palloc memory leaks with valgrind's
>> leak detection mode.  The trick is to ask it for a report before the
>> context gets destroyed.  Beats writing your own infrastructure ...

> I haven't tried valgrind, so can't compare.
> Would it be possible to filter which memory contexts to track? Say we
> know the leak is in ExecutorState, so we don't need to track allocations
> in other contexts. That was a huge speedup for me, maybe it'd help
> valgrind too.

I don't think valgrind has a way to do that, but this'd be something you
set up specially in any case.

> Also, how did you ask for the report before context gets destroyed?

There are several valgrind client requests that could be helpful:

/* Do a full memory leak check (like --leak-check=full) mid-execution. */
#define VALGRIND_DO_LEAK_CHECK                                   \
    VALGRIND_DO_CLIENT_REQUEST_STMT(VG_USERREQ__DO_LEAK_CHECK,   \
                                    0, 0, 0, 0, 0)

/* Same as VALGRIND_DO_LEAK_CHECK but only showing the entries for
   which there was an increase in leaked bytes or leaked nr of blocks
   since the previous leak search. */
#define VALGRIND_DO_ADDED_LEAK_CHECK                            \
    VALGRIND_DO_CLIENT_REQUEST_STMT(VG_USERREQ__DO_LEAK_CHECK,  \
                                    0, 1, 0, 0, 0)

/* Return number of leaked, dubious, reachable and suppressed bytes found by
   all previous leak checks.  They must be lvalues.  */
#define VALGRIND_COUNT_LEAK_BLOCKS(leaked, dubious, reachable, suppressed) \

Putting VALGRIND_DO_ADDED_LEAK_CHECK someplace in the executor loop would
help narrow things down pretty quickly, assuming you had a self-contained
example demonstrating the leak.  I don't recall exactly how I used these
but it was something along that line.

>> Yeah, it's not clear whether we could make the still-hypothetical check
>> sensitive enough to find leaks using small test cases without getting an
>> unworkable number of false positives.  Still, might be worth trying.

> I'm not against experimenting with that. Were you thinking about
> something that'd be cheap enough to just be enabled always/everywhere,
> or something we'd enable during testing?

We seem to have already paid the overhead of counting all palloc
allocations, so I don't think it'd be too expensive to occasionally check
the ExecutorState's mem_allocated and see if it's growing (especially
if we only do so in assert-enabled builds).  The trick is to define the
rules for what's worth reporting.

>> It might be an acceptable tradeoff to have stricter rules for what can
>> be allocated in ExecutorState in order to make this sort of problem
>> more easily detectable.

> Would these rules be just customary, or defined/enforced in the code
> somehow? I can't quite imagine how would that work, TBH.

If the check bleated "WARNING: possible executor memory leak" during
regression testing, people would soon become conditioned to doing
whatever they have to do to avoid it ;-)

			regards, tom lane