Re: Fix for segfault in logical replication on master

[Japin Li]

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