RE: Fix for segfault in logical replication on master

From: "osumi(dot)takamichi(at)fujitsu(dot)com" <osumi(dot)takamichi(at)fujitsu(dot)com>
To: 'Japin Li' <japinli(at)hotmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
Cc: Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com>, "akapila(at)postgresql(dot)org" <akapila(at)postgresql(dot)org>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: RE: Fix for segfault in logical replication on master
Date: 2021-06-22 03:48:20
Message-ID: OSBPR01MB488853411EFD0B559F2EA47AED099@OSBPR01MB4888.jpnprd01.prod.outlook.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Tuesday, June 22, 2021 11:08 AM Japin Li <japinli(at)hotmail(dot)com> wrote:
> 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,
> >> >> 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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2021-06-22 03:53:46 Re: Different compression methods for FPI
Previous Message Justin Pryzby 2021-06-22 03:13:58 Re: Different compression methods for FPI