Re: Fix for segfault in logical replication on master

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?

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

diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index d55ae016d0..94fbf1aa19 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -5244,9 +5244,9 @@ Bitmapset *
RelationGetIdentityKeyBitmap(Relation relation)
{
Bitmapset *idindexattrs = NULL; /* columns in the replica identity */
- List *indexoidlist;
Relation indexDesc;
int i;
+ Oid replidindex;
MemoryContext oldcxt;

/* Quick exit if we already computed the result */
@@ -5260,18 +5260,14 @@ RelationGetIdentityKeyBitmap(Relation relation)
/* Historic snapshot must be set. */
Assert(HistoricSnapshotActive());

- indexoidlist = RelationGetIndexList(relation);
-
- /* Fall out if no indexes (but relhasindex was set) */
- if (indexoidlist == NIL)
- return NULL;
+ replidindex = RelationGetReplicaIndex(relation);

/* Fall out if there is no replica identity index */
- if (!OidIsValid(relation->rd_replidindex))
+ if (!OidIsValid(replidindex))
return NULL;

/* Look up the description for the replica identity index */
- indexDesc = RelationIdGetRelation(relation->rd_replidindex);
+ indexDesc = RelationIdGetRelation(replidindex);

if (!RelationIsValid(indexDesc))
elog(ERROR, "could not open relation with OID %u",
@@ -5295,7 +5291,6 @@ RelationGetIdentityKeyBitmap(Relation relation)
}

RelationClose(indexDesc);
- list_free(indexoidlist);

/* Don't leak the old values of these bitmaps, if any */
bms_free(relation->rd_idattr);