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
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 |