On Mon, 21 Jun 2021 at 19:06, Amit Kapila <amit.kapil...@gmail.com> wrote:
> On Mon, Jun 21, 2021 at 4:09 PM Japin Li <japi...@hotmail.com> wrote:
>>
>> 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?
>>
>
> 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.
>From 0582d92ba4df517f3e67717751cddb8916c8ab9f Mon Sep 17 00:00:00 2001
From: Japin Li <japi...@hotmail.com>
Date: Tue, 22 Jun 2021 09:59:38 +0800
Subject: [PATCH v1] Minor code beautification for RelationGetIdentityKeyBitmap
---
src/backend/utils/cache/relcache.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
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);
--
2.30.2
