RE: Fix for segfault in logical replication on master

On Tuesday, June 22, 2021 11:08 AM Japin Li <japinli@hotmail.com> wrote:
> On Mon, 21 Jun 2021 at 19:06, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > On Mon, Jun 21, 2021 at 4:09 PM Japin Li <japinli@hotmail.com> wrote:
> >>
> >> On Mon, 21 Jun 2021 at 17:54, Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> >> > On Mon, Jun 21, 2021 at 2:06 PM Japin Li <japinli@hotmail.com> wrote:
> >> >>
> >> >> On Mon, 21 Jun 2021 at 16:22, Amit Kapila
> <amit.kapila16@gmail.com> wrote:
> >> >> > On Mon, Jun 21, 2021 at 1:30 PM Japin Li <japinli@hotmail.com>
> wrote:
> >> >> >>
> >> >> >> On Sat, 19 Jun 2021 at 17:18, Amit Kapila
> <amit.kapila16@gmail.com> wrote:
> >> >> >> > On Fri, Jun 18, 2021 at 9:18 AM Amit Kapila
> <amit.kapila16@gmail.com> wrote:
> >> >> >>
> >> >> >> Or we can free the memory owned by indexoidlist after check
> >> >> >> whether it is NIL, because we do not use it in the later.
> >> >> >>
> >> >> >
> >> >> > Valid point. But I am thinking do we really need to fetch and
> >> >> > check indexoidlist here?
> >> >>
> >> >> IMO, we shold not fetch and check the indexoidlist here, since we
> >> >> do not use it.  However, we should use RelationGetIndexList() to
> >> >> update the
> >> >> reladion->rd_replidindex, so we should fetch the indexoidlist,
> >> >> reladion->maybe we
> >> >> can use the following code:
> >> >>
> >> >>     indexoidlist = RelationGetIndexList(relation);
> >> >>     list_free(indexoidlist);
> >> >>
> >> >> Or does there any function that only update the
> >> >> relation->rd_replidindex or related fields, but do not fetch the
> indexoidlist?
> >> >>
> >> >
> >> > How about RelationGetReplicaIndex? It fetches the indexlist only
> >> > when required and frees it immediately. But otherwise, currently,
> >> > there shouldn't be any memory leak because we allocate this in
> >> > "logical replication output context" which is reset after
> >> > processing each change message, see pgoutput_change.
> >>
> >> Thanks for your explanation.  It might not  be a memory leak, however
> >> it's a little confuse when we free the memory of the indexoidlist in
> >> one place, but not free it in another place.
> >>
> >> I attached a patch to fix this.  Any thoughts?
> >>
> >
> > Your patch appears to be on the lines we discussed but I would prefer
> > to get it done after Beta2 as this is just a minor code improvement.
> > Can you please send the change as a patch file instead of copy-pasting
> > the diff at the end of the email?
>
> Thanks for your review! Attached v1 patch.
Your patch can be applied to the HEAD.
And, I also reviewed your patch, which seems OK.
Make check-world has passed with your patch in my env as well.

Best Regards,
    Takamichi Osumi