Re: Decoding speculative insert with toast leaks memory

On Thu, Jun 17, 2021 at 12:56 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Wed, Jun 16, 2021 at 8:18 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >
> > Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
> > > Pushed!
> >
> > skink reports that this has valgrind issues:
> >
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2021-06-15%2020%3A49%3A26
> >
>
> The problem happens at line:
> rel_sync_cache_relation_cb()
> {
> ..
> if (entry->map)
> ..
>
> I think the reason is that before we initialize 'entry->map' in
> get_rel_sync_entry(), the invalidation is processed as part of which
> when we try to clean up the entry, it tries to access uninitialized
> value. Note, this won't happen in HEAD as we initialize 'entry->map'
> before we get to process any invalidation. We have fixed a similar
> issue in HEAD sometime back as part of the commit 69bd60672a, so we
> need to make a similar change in PG-13 as well.
>
> This problem is introduced by commit d250568121 (Fix memory leak due
> to RelationSyncEntry.map.) not by the patch in this thread, so keeping
> Amit L and Osumi-San in the loop.

Thanks.

Maybe not sufficient as a fix, but I wonder if
rel_sync_cache_relation_cb() should really also check that
replicate_valid is true in the following condition:

/*
* Reset schema sent status as the relation definition may have changed.
* Also free any objects that depended on the earlier definition.
*/
if (entry != NULL)
{

If the problem is with HEAD, I don't quite understand why the last
statement of the following code block doesn't suffice:

/* Not found means schema wasn't sent */
if (!found)
{
/* immediately make a new entry valid enough to satisfy callbacks */
entry->schema_sent = false;
entry->streamed_txns = NIL;
entry->replicate_valid = false;
entry->pubactions.pubinsert = entry->pubactions.pubupdate =
entry->pubactions.pubdelete = entry->pubactions.pubtruncate = false;
entry->publish_as_relid = InvalidOid;
entry->map = NULL; /* will be set by maybe_send_schema() if needed */
}

Do we need the same statement at the end of the following block?

/* Validate the entry */
if (!entry->replicate_valid)
{

--
Amit Langote
EDB: http://www.enterprisedb.com