Re: Decoding speculative insert with toast leaks memory

On Thu, Jun 17, 2021 at 3:42 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> 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.

Yeah, I see that. If we can be sure that the callback can't get
called between hash_search() allocating the entry and the above code
block making the entry look valid, which appears to be the case, then
I guess we don't need to worry.

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

Oh I missed that the problem report is for the PG13 branch.

How about the attached patch then?

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