Re: Decoding speculative insert with toast leaks memory

On Thu, Jun 17, 2021 at 10:39 AM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>
> 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:
>

I don't think that is required because we initialize the entry in "if
(!found)" case in the HEAD.

> /*
> * 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,
>

The problem occurs only in PG-13. So, we need to make PG-13 code
similar to HEAD as far as initialization of entry is concerned.

--
With Regards,
Amit Kapila.