Re: Decoding speculative insert with toast leaks memory

On Thu, Jun 17, 2021 at 12:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Wed, Jun 16, 2021 at 8:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Amit Kapila <amit.kapila16@gmail.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