On Mon, 22 Jun 2020 at 23:22, Alvaro Herrera <alvhe...@2ndquadrant.com> wrote:
> > I'm not sure that expecting the relcache entry's refcount drops to zero > at the right time is a good approach; that may cause leaks some other > place might have refcounts you're not expecting (say, an open cursor > that's not fully read). > My proposal was to maintain a refcount counting the number of times an index is opened in ResultRelInfo itself, not to rely on the relcache ref count. But I think that would be an overkill. Please read ahead > > (I'm not terribly worried about refcount leakage as a theoretical > concern, since the ResourceOwner mechanism will warn us about that if it > happens.) > > > 2. Throw an error in ExecOpenIndices if all the arrays are present. We > will > > need to check leaf_part_rri->ri_IndexRelationDescs == NULL in > > ExecInitPartitionInfo(). > > This sounds like a job for an assert rather than an error. > I agree. Here's a patch to fix to add Assert'ion in ExecOpenIndices(). I ran make check with this patch and the assertion didn't trip. I think this will be a good step forward. -- Best Wishes, Ashutosh
From 638e922bebd9ee4657e40daa63b61ccac4bd205c Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat <ashutosh.bapat@2ndquadrant.com> Date: Thu, 25 Jun 2020 18:50:52 +0530 Subject: [PATCH] Assert if ExecOpenIndices() is called twice on same result relation. If ExecOpenIndices() is called more than once for the result relation, it overwrites the arrays for indices, leaking memory. Assert would catch this in development. Ashutosh Bapat. --- src/backend/executor/execIndexing.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c index 1862af621b..a94b98b045 100644 --- a/src/backend/executor/execIndexing.c +++ b/src/backend/executor/execIndexing.c @@ -158,6 +158,13 @@ ExecOpenIndices(ResultRelInfo *resultRelInfo, bool speculative) RelationPtr relationDescs; IndexInfo **indexInfoArray; + /* + * This function can be called only once per ResultRelInfo, otherwise, we + * will leak memory for arrays and also leak index relcache references. + */ + Assert(!resultRelInfo->ri_IndexRelationDescs); + Assert(!resultRelInfo->ri_IndexRelationInfo); + resultRelInfo->ri_NumIndices = 0; /* fast path if no indexes */ -- 2.17.1
