Re: Fix for segfault in logical replication on master

On Mon, 21 Jun 2021 at 19:06, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Mon, Jun 21, 2021 at 4:09 PM Japin Li <japinli(at)hotmail(dot)com> wrote:
>>
>> On Mon, 21 Jun 2021 at 17:54, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> > On Mon, Jun 21, 2021 at 2:06 PM Japin Li <japinli(at)hotmail(dot)com> wrote:
>> >>
>> >> On Mon, 21 Jun 2021 at 16:22, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> >> > On Mon, Jun 21, 2021 at 1:30 PM Japin Li <japinli(at)hotmail(dot)com> wrote:
>> >> >>
>> >> >> On Sat, 19 Jun 2021 at 17:18, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> >> >> > On Fri, Jun 18, 2021 at 9:18 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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, 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.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.