Re: Table refer leak in logical replication

On Mon, Apr 19, 2021 at 6:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Mon, Apr 19, 2021 at 12:32 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Sat, Apr 17, 2021 at 10:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > On Fri, Apr 16, 2021 at 11:55 AM Michael Paquier <michael@paquier.xyz> wrote:
> > > > Attached is v5 that I am finishing with.  Much more could be done but
> > > > I don't want to do something too invasive at this stage of the game.
> > > > There are a couple of extra relations in terms of relations opened for
> > > > a partitioned table within create_estate_for_relation() when
> > > > redirecting to the tuple routing, but my guess is that this would be
> > > > better in the long-term.
> > > >
> > >
> > > Hmm, I am not sure if it is a good idea to open indexes needlessly
> > > especially when it is not done in the previous code.
> > >
> > > @@ -1766,8 +1771,11 @@ apply_handle_tuple_routing(ResultRelInfo *relinfo,
> > >   slot_getallattrs(remoteslot);
> > >   }
> > >   MemoryContextSwitchTo(oldctx);
> > > +
> > > + ExecOpenIndices(partrelinfo_new, false);
> > >   apply_handle_insert_internal(partrelinfo_new, estate,
> > >   remoteslot_part);
> > > + ExecCloseIndices(partrelinfo_new);
> > >   }
> > >
> > > It seems you forgot to call open indexes before apply_handle_delete_internal.
> > >
> > > I am not sure if it is a good idea to do the refactoring related to
> > > indexes or other things to fix a minor bug in commit 1375422c. It
> > > might be better to add a simple fix like what Hou-San has originally
> > > proposed [1] because even using ExecInitResultRelation might not be
> > > the best thing as it is again trying to open a range table which we
> > > have already opened in logicalrep_rel_open.
> >
> > FWIW, I agree with fixing this bug of 1375422c in as least scary
> > manner as possible.  Hou-san proposed that we add the ResultRelInfo
> > that apply_handle_{insert|update|delete} initialize themselves to
> > es_opened_result_relations.  I would prefer that only
> > ExecInitResultRelation() add anything to es_opened_result_relations()
> > to avoid future maintenance problems.  Instead, a fix as simple as the
> > Hou-san's proposed fix would be to add a ExecCloseResultRelations()
> > call at the end of each of apply_handle_{insert|update|delete}.
> >
>
> Yeah, that will work too but might look a bit strange. BTW, how that
> is taken care of for ExecuteTruncateGuts? I mean we do add rels there
> like Hou-San's patch without calling ExecCloseResultRelations, the
> rels are probably closed when we close the relation in worker.c but
> what about memory for the list?

It seems I had forgotten the code I had written myself.  The following
is from ExecuteTruncateGuts():

    /*
     * To fire triggers, we'll need an EState as well as a ResultRelInfo for
     * each relation.  We don't need to call ExecOpenIndices, though.
     *
     * We put the ResultRelInfos in the es_opened_result_relations list, even
     * though we don't have a range table and don't populate the
     * es_result_relations array.  That's a bit bogus, but it's enough to make
     * ExecGetTriggerResultRel() find them.
     */
    estate = CreateExecutorState();
    resultRelInfos = (ResultRelInfo *)
        palloc(list_length(rels) * sizeof(ResultRelInfo));
    resultRelInfo = resultRelInfos;
    foreach(cell, rels)
    {
        Relation    rel = (Relation) lfirst(cell);

        InitResultRelInfo(resultRelInfo,
                          rel,
                          0,    /* dummy rangetable index */
                          NULL,
                          0);
        estate->es_opened_result_relations =
            lappend(estate->es_opened_result_relations, resultRelInfo);
        resultRelInfo++;
    }

So, that is exactly what Hou-san's patch did.  Although, the comment
does admit that doing this is a bit bogus and maybe written (by Heikki
IIRC) as a caution against repeating the pattern.


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