relcache reference leak on refresh materialized view concurrently

Started by Nonameabout 12 years ago3 messagesbugs
Jump to latest
#1Noname
yamamoto@valinux.co.jp

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);
 	}

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Noname (#1)
Re: relcache reference leak on refresh materialized view concurrently

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

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

#3Noname
yamamoto@valinux.co.jp
In reply to: Tom Lane (#2)
Re: relcache reference leak on refresh materialized view concurrently

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

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs