Thread: relcache reference leak on refresh materialized view concurrently

the following patch fixes missing index_close in case
the mv has non-unique indexes.

YAMAMOTO Takashi

diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index d64f165..83efab1 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -611,6 +611,8 @@ refresh_by_match_merge(Oid matviewOid, Oid tempOid)
         Relation    indexRel;
         HeapTuple    indexTuple;
         Form_pg_index indexStruct;
+        int            numatts;
+        int            i;

         indexRel = index_open(indexoid, RowExclusiveLock);
         indexTuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexoid));
@@ -619,61 +621,63 @@ refresh_by_match_merge(Oid matviewOid, Oid tempOid)
         indexStruct = (Form_pg_index) GETSTRUCT(indexTuple);

         /* We're only interested if it is unique and valid. */
-        if (indexStruct->indisunique && IndexIsValid(indexStruct))
+        if (!indexStruct->indisunique || !IndexIsValid(indexStruct))
         {
-            int            numatts = indexStruct->indnatts;
-            int            i;
-
-            /* Skip any index on an expression. */
-            if (RelationGetIndexExpressions(indexRel) != NIL)
-            {
-                index_close(indexRel, NoLock);
-                ReleaseSysCache(indexTuple);
-                continue;
-            }
+            index_close(indexRel, NoLock);
+            ReleaseSysCache(indexTuple);
+            continue;
+        }

-            /* Skip partial indexes. */
-            if (RelationGetIndexPredicate(indexRel) != NIL)
-            {
-                index_close(indexRel, NoLock);
-                ReleaseSysCache(indexTuple);
-                continue;
-            }
+        /* Skip any index on an expression. */
+        if (RelationGetIndexExpressions(indexRel) != NIL)
+        {
+            index_close(indexRel, NoLock);
+            ReleaseSysCache(indexTuple);
+            continue;
+        }

-            /* Hold the locks, since we're about to run DML which needs them. */
+        /* Skip partial indexes. */
+        if (RelationGetIndexPredicate(indexRel) != NIL)
+        {
             index_close(indexRel, NoLock);
+            ReleaseSysCache(indexTuple);
+            continue;
+        }

-            /* Add quals for all columns from this index. */
-            for (i = 0; i < numatts; i++)
-            {
-                int            attnum = indexStruct->indkey.values[i];
-                Oid            type;
-                Oid            op;
-                const char *colname;
-
-                /*
-                 * Only include the column once regardless of how many times
-                 * it shows up in how many indexes.
-                 */
-                if (usedForQual[attnum - 1])
-                    continue;
-                usedForQual[attnum - 1] = true;
-
-                /*
-                 * Actually add the qual, ANDed with any others.
-                 */
-                if (foundUniqueIndex)
-                    appendStringInfoString(&querybuf, " AND ");
-
-                colname = quote_identifier(NameStr((tupdesc->attrs[attnum - 1])->attname));
-                appendStringInfo(&querybuf, "newdata.%s ", colname);
-                type = attnumTypeId(matviewRel, attnum);
-                op = lookup_type_cache(type, TYPECACHE_EQ_OPR)->eq_opr;
-                mv_GenerateOper(&querybuf, op);
-                appendStringInfo(&querybuf, " mv.%s", colname);
-
-                foundUniqueIndex = true;
-            }
+        /* Hold the locks, since we're about to run DML which needs them. */
+        index_close(indexRel, NoLock);
+
+        /* Add quals for all columns from this index. */
+        numatts = indexStruct->indnatts;
+        for (i = 0; i < numatts; i++)
+        {
+            int            attnum = indexStruct->indkey.values[i];
+            Oid            type;
+            Oid            op;
+            const char *colname;
+
+            /*
+             * Only include the column once regardless of how many times
+             * it shows up in how many indexes.
+             */
+            if (usedForQual[attnum - 1])
+                continue;
+            usedForQual[attnum - 1] = true;
+
+            /*
+             * Actually add the qual, ANDed with any others.
+             */
+            if (foundUniqueIndex)
+                appendStringInfoString(&querybuf, " AND ");
+
+            colname = quote_identifier(NameStr((tupdesc->attrs[attnum - 1])->attname));
+            appendStringInfo(&querybuf, "newdata.%s ", colname);
+            type = attnumTypeId(matviewRel, attnum);
+            op = lookup_type_cache(type, TYPECACHE_EQ_OPR)->eq_opr;
+            mv_GenerateOper(&querybuf, op);
+            appendStringInfo(&querybuf, " mv.%s", colname);
+
+            foundUniqueIndex = true;
         }
         ReleaseSysCache(indexTuple);
     }

yamamoto@valinux.co.jp (YAMAMOTO Takashi) writes:
> the following patch fixes missing index_close in case
> the mv has non-unique indexes.

Hm ... I see the leak, I think, but isn't this an extraordinarily
complex patch?  Looks like adding

    else
    {
        index_close(indexRel, NoLock);
    }

at the bottom of the "if" would also fix the problem, and would be
far easier to verify.

            regards, tom lane

> yamamoto@valinux.co.jp (YAMAMOTO Takashi) writes:
>> the following patch fixes missing index_close in case
>> the mv has non-unique indexes.
>
> Hm ... I see the leak, I think, but isn't this an extraordinarily
> complex patch?  Looks like adding

i tend to think it's simpler, but it's a matter of taste.

>
>     else
>     {
>         index_close(indexRel, NoLock);
>     }
>
> at the bottom of the "if" would also fix the problem, and would be
> far easier to verify.

yes, it's probably the physically smallest patch.

YAMAMOTO Takashi

>
>             regards, tom lane