From 52e6e33ab495cb2b0e694ee480ba7c6394315053 Mon Sep 17 00:00:00 2001
From: Kevin Grittner <kgrittn@postgresql.org>
Date: Tue, 9 Apr 2013 13:02:49 -0500
Subject: [PATCH] Create a distinction between a populated matview and a
 scannable one.

The intent was that being populated would, long term, be just one
of the conditions which could affect whether a matview was
scannable; being populated should be necessary but not always
sufficient to scan the relation.  Since only CREATE and REFRESH
currently determine the scannability, names and comments
accidentally conflated these concepts, leading to confusion.

Also add missing locking for the SQL function which allows a
test for scannability, and fix a modularity violatiion.

Per complaints from Tom Lane, although its not clear that these
will satisfy his concerns.  Hopefully this will at least better
frame the discussion.
---
 src/backend/commands/cluster.c       | 13 ++++---
 src/backend/commands/createas.c      |  2 +-
 src/backend/commands/matview.c       | 15 ++++----
 src/backend/executor/execMain.c      | 55 +++++++++++++++-------------
 src/backend/rewrite/rewriteHandler.c |  3 +-
 src/backend/utils/adt/dbsize.c       | 13 +++++--
 src/backend/utils/cache/relcache.c   | 20 +++++-----
 src/bin/pg_dump/pg_dump.c            |  4 +-
 src/include/commands/matview.h       |  2 +-
 src/include/utils/rel.h              | 12 +++++-
 10 files changed, 81 insertions(+), 58 deletions(-)

diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index ef9c5f1adc7..ed62246cc52 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -381,13 +381,14 @@ cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose,
 		check_index_is_clusterable(OldHeap, indexOid, recheck, AccessExclusiveLock);
 
 	/*
-	 * Quietly ignore the request if the a materialized view is not scannable.
-	 * No harm is done because there is nothing no data to deal with, and we
-	 * don't want to throw an error if this is part of a multi-relation
-	 * request -- for example, CLUSTER was run on the entire database.
+	 * Quietly ignore the request if this is a materialized view which has not
+	 * been populated from its query. No harm is done because there is no data
+	 * to deal with, and we don't want to throw an error if this is part of a
+	 * multi-relation request -- for example, CLUSTER was run on the entire
+	 * database.
 	 */
 	if (OldHeap->rd_rel->relkind == RELKIND_MATVIEW &&
-		!OldHeap->rd_isscannable)
+		!OldHeap->rd_ispopulated)
 	{
 		relation_close(OldHeap, AccessExclusiveLock);
 		return;
@@ -923,7 +924,7 @@ copy_heap_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
 
 	if (OldHeap->rd_rel->relkind == RELKIND_MATVIEW)
 		/* Make sure the heap looks good even if no rows are written. */
-		SetRelationIsScannable(NewHeap);
+		SetMatViewToPopulated(NewHeap);
 
 	/*
 	 * Scan through the OldHeap, either in OldIndex order or sequentially;
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 06bbae5cc59..079fafa06fb 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -417,7 +417,7 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
 
 	if (into->relkind == RELKIND_MATVIEW && !into->skipData)
 		/* Make sure the heap looks good even if no rows are written. */
-		SetRelationIsScannable(intoRelationDesc);
+		SetMatViewToPopulated(intoRelationDesc);
 
 	/*
 	 * Check INSERT permission on the constructed table.
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index 1d2b3478289..ac7719e40da 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -52,22 +52,21 @@ static void refresh_matview_datafill(DestReceiver *dest, Query *query,
 									 const char *queryString);
 
 /*
- * SetRelationIsScannable
- *		Make the relation appear scannable.
+ * SetMatViewToPopulated
+ *		Indicate that the materialized view has been populated by its query.
  *
- * NOTE: This is only implemented for materialized views. The heap starts out
- * in a state that doesn't look scannable, and can only transition from there
- * to scannable, unless a new heap is created.
+ * NOTE: The heap starts out in a state that doesn't look scannable, and can
+ * only transition from there to scannable at the time a new heap is created.
  *
  * NOTE: caller must be holding an appropriate lock on the relation.
  */
 void
-SetRelationIsScannable(Relation relation)
+SetMatViewToPopulated(Relation relation)
 {
 	Page        page;
 
 	Assert(relation->rd_rel->relkind == RELKIND_MATVIEW);
-	Assert(relation->rd_isscannable == false);
+	Assert(relation->rd_ispopulated == false);
 
 	page = (Page) palloc(BLCKSZ);
 	PageInit(page, BLCKSZ, 0);
@@ -323,7 +322,7 @@ transientrel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
 		myState->hi_options |= HEAP_INSERT_SKIP_WAL;
 	myState->bistate = GetBulkInsertState();
 
-	SetRelationIsScannable(transientrel);
+	SetMatViewToPopulated(transientrel);
 
 	/* Not using WAL requires smgr_targblock be initially invalid */
 	Assert(RelationGetTargetBlock(transientrel) == InvalidBlockNumber);
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 1f2a23bcdd6..2a72e3c9e6c 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -499,7 +499,8 @@ ExecutorRewind(QueryDesc *queryDesc)
  *		Check that relations which are to be accessed are in a scannable
  *		state.
  *
- * If not, throw error. For a materialized view, suggest refresh.
+ * Currently the only relations which are not are materialized views which
+ * have not been populated by their queries.
  */
 static void
 ExecCheckRelationsScannable(List *rangeTable)
@@ -513,32 +514,29 @@ ExecCheckRelationsScannable(List *rangeTable)
 		if (rte->rtekind != RTE_RELATION)
 			continue;
 
-		if (!RelationIdIsScannable(rte->relid))
-		{
-			if (rte->relkind == RELKIND_MATVIEW)
-			{
-				/* It is OK to replace the contents of an invalid matview. */
-				if (rte->isResultRel)
-					continue;
+		if (rte->relkind != RELKIND_MATVIEW)
+			continue;
 
-				ereport(ERROR,
-						(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-						 errmsg("materialized view \"%s\" has not been populated",
-								get_rel_name(rte->relid)),
-						 errhint("Use the REFRESH MATERIALIZED VIEW command.")));
-			}
-			else
-				/* This should never happen, so elog will do. */
-				elog(ERROR, "relation \"%s\" is not flagged as scannable",
-					 get_rel_name(rte->relid));
-		}
+		/* It is OK to target an unpopulated materialized for results. */
+		if (rte->isResultRel)
+			continue;
+
+		if (!RelationIdIsScannable(rte->relid))
+			ereport(ERROR,
+					(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+					 errmsg("materialized view \"%s\" has not been populated",
+							get_rel_name(rte->relid)),
+					 errhint("Use the REFRESH MATERIALIZED VIEW command.")));
 	}
 }
 
 /*
- * Tells whether a relation is scannable.
+ * Tells whether a relation is scannable based on its OID.
+ *
+ * Currently only non-populated materialized views are not.  This is likely to
+ * change to include other conditions.
  *
- * Currently only non-populated materialzed views are not.
+ * This should only be called while a lock is held on the relation.
  */
 static bool
 RelationIdIsScannable(Oid relid)
@@ -546,9 +544,9 @@ RelationIdIsScannable(Oid relid)
 	Relation	relation;
 	bool		result;
 
-	relation = RelationIdGetRelation(relid);
-	result = relation->rd_isscannable;
-	RelationClose(relation);
+	relation = heap_open(relid, NoLock);
+	result = RelationIsScannable(relation);
+	heap_close(relation, NoLock);
 
 	return result;
 }
@@ -945,7 +943,14 @@ InitPlan(QueryDesc *queryDesc, int eflags)
 
 	/*
 	 * Unless we are creating a view or are creating a materialized view WITH
-	 * NO DATA, ensure that all referenced relations are scannable.
+	 * NO DATA, ensure that all referenced relations are scannable.  The
+	 * omitted cases will be checked as SELECT statements in a different
+	 * phase, so checking again here would be wasteful and it would generate
+	 * errors on a materialized view referenced as a target.
+	 *
+	 * NB: This is being done after all relations are locked, files have been
+	 * opened, etc., to avoid duplicating that effort or creating deadlock
+	 * possibilities.
 	 */
 	if ((eflags & EXEC_FLAG_WITH_NO_DATA) == 0)
 		ExecCheckRelationsScannable(rangeTable);
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 8abf5e438c1..638fd1a85b5 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1615,7 +1615,8 @@ fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
 		 * expansion doesn't give us a lot to work with, so we are trusting
 		 * earlier validations to throw error if needed.
 		 */
-		if (rel->rd_rel->relkind == RELKIND_MATVIEW && rel->rd_isscannable)
+		if (rel->rd_rel->relkind == RELKIND_MATVIEW &&
+			RelationIsScannable(rel))
 		{
 			heap_close(rel, NoLock);
 			continue;
diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c
index d589d26070d..d32d9014c7e 100644
--- a/src/backend/utils/adt/dbsize.c
+++ b/src/backend/utils/adt/dbsize.c
@@ -840,7 +840,8 @@ pg_relation_filepath(PG_FUNCTION_ARGS)
  * Indicate whether a relation is scannable.
  *
  * Currently, this is always true except for a materialized view which has not
- * been populated.
+ * been populated.  It is expected that other conditions for allowing a
+ * materialized view to be scanned will be added in later releases.
  */
 Datum
 pg_relation_is_scannable(PG_FUNCTION_ARGS)
@@ -850,9 +851,13 @@ pg_relation_is_scannable(PG_FUNCTION_ARGS)
 	bool		result;
 
 	relid = PG_GETARG_OID(0);
-	relation = RelationIdGetRelation(relid);
-	result = relation->rd_isscannable;
-	RelationClose(relation);
+	relation = try_relation_open(relid, AccessShareLock);
 
+	if (relation == NULL)
+		PG_RETURN_BOOL(false);
+
+	result = RelationIsScannable(relation);
+
+	relation_close(relation, AccessShareLock);
 	PG_RETURN_BOOL(result);
 }
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 5b1d1e5b10a..670fa8c1667 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -958,9 +958,9 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
 
 	if (relation->rd_rel->relkind == RELKIND_MATVIEW &&
 		heap_is_matview_init_state(relation))
-		relation->rd_isscannable = false;
+		relation->rd_ispopulated = false;
 	else
-		relation->rd_isscannable = true;
+		relation->rd_ispopulated = true;
 
 	/*
 	 * now we can free the memory allocated for pg_class_tuple
@@ -1531,7 +1531,7 @@ formrdesc(const char *relationName, Oid relationReltype,
 	 * initialize physical addressing information for the relation
 	 */
 	RelationInitPhysicalAddr(relation);
-	relation->rd_isscannable = true;
+	relation->rd_ispopulated = true;
 
 	/*
 	 * initialize the rel-has-index flag, using hardwired knowledge
@@ -1756,7 +1756,7 @@ RelationReloadIndexInfo(Relation relation)
 	heap_freetuple(pg_class_tuple);
 	/* We must recalculate physical address in case it changed */
 	RelationInitPhysicalAddr(relation);
-	relation->rd_isscannable = true;
+	relation->rd_ispopulated = true;
 
 	/*
 	 * For a non-system index, there are fields of the pg_index row that are
@@ -1907,9 +1907,9 @@ RelationClearRelation(Relation relation, bool rebuild)
 		RelationInitPhysicalAddr(relation);
 		if (relation->rd_rel->relkind == RELKIND_MATVIEW &&
 			heap_is_matview_init_state(relation))
-			relation->rd_isscannable = false;
+			relation->rd_ispopulated = false;
 		else
-			relation->rd_isscannable = true;
+			relation->rd_ispopulated = true;
 
 		if (relation->rd_rel->relkind == RELKIND_INDEX)
 		{
@@ -2700,9 +2700,9 @@ RelationBuildLocalRelation(const char *relname,
 
 	/* materialized view not initially scannable */
 	if (relkind == RELKIND_MATVIEW)
-		rel->rd_isscannable = false;
+		rel->rd_ispopulated = false;
 	else
-		rel->rd_isscannable = true;
+		rel->rd_ispopulated = true;
 
 	/*
 	 * Okay to insert into the relcache hash tables.
@@ -4450,9 +4450,9 @@ load_relcache_init_file(bool shared)
 		RelationInitPhysicalAddr(rel);
 		if (rel->rd_rel->relkind == RELKIND_MATVIEW &&
 			heap_is_matview_init_state(rel))
-			rel->rd_isscannable = false;
+			rel->rd_ispopulated = false;
 		else
-			rel->rd_isscannable = true;
+			rel->rd_ispopulated = true;
 	}
 
 	/*
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 97751afc2da..366eca1ffad 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -4264,7 +4264,8 @@ getTables(Archive *fout, int *numTables)
 						  "c.relhasindex, c.relhasrules, c.relhasoids, "
 						  "c.relfrozenxid, tc.oid AS toid, "
 						  "tc.relfrozenxid AS tfrozenxid, "
-		 "c.relpersistence, pg_relation_is_scannable(c.oid) as isscannable, "
+						  "c.relpersistence, "
+						  "CASE WHEN c.relkind = '%c' THEN pg_relation_is_scannable(c.oid) ELSE 't'::bool END as isscannable, "
 						  "c.relpages, "
 						  "CASE WHEN c.reloftype <> 0 THEN c.reloftype::pg_catalog.regtype ELSE NULL END AS reloftype, "
 						  "d.refobjid AS owning_tab, "
@@ -4282,6 +4283,7 @@ getTables(Archive *fout, int *numTables)
 				   "WHERE c.relkind in ('%c', '%c', '%c', '%c', '%c', '%c') "
 						  "ORDER BY c.oid",
 						  username_subquery,
+						  RELKIND_MATVIEW,
 						  RELKIND_SEQUENCE,
 						  RELKIND_RELATION, RELKIND_SEQUENCE,
 						  RELKIND_VIEW, RELKIND_COMPOSITE_TYPE,
diff --git a/src/include/commands/matview.h b/src/include/commands/matview.h
index b5d52503c0d..09bc384086f 100644
--- a/src/include/commands/matview.h
+++ b/src/include/commands/matview.h
@@ -20,7 +20,7 @@
 #include "utils/relcache.h"
 
 
-extern void SetRelationIsScannable(Relation relation);
+extern void SetMatViewToPopulated(Relation relation);
 
 extern void ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
 				  ParamListInfo params, char *completionTag);
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index a4daf772e57..1fd3f67b1e4 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -83,7 +83,7 @@ typedef struct RelationData
 	BackendId	rd_backend;		/* owning backend id, if temporary relation */
 	bool		rd_islocaltemp; /* rel is a temp rel of this session */
 	bool		rd_isnailed;	/* rel is nailed in cache */
-	bool		rd_isscannable; /* rel can be scanned */
+	bool		rd_ispopulated;	/* matview has query results */
 	bool		rd_isvalid;		/* relcache entry is valid */
 	char		rd_indexvalid;	/* state of rd_indexlist: 0 = not valid, 1 =
 								 * valid, 2 = temporarily forced */
@@ -407,6 +407,16 @@ typedef struct StdRdOptions
 	((relation)->rd_rel->relpersistence == RELPERSISTENCE_TEMP && \
 	 !(relation)->rd_islocaltemp)
 
+
+/*
+ * RelationIsScannable
+ * 		Currently can only be false for a materialized view which has not been
+ * 		populated by its query.  This is likely to get more complicated later,
+ * 		so use a macro which looks like a function.
+ */
+#define RelationIsScannable(relation) ((relation)->rd_ispopulated)
+
+
 /* routines in utils/cache/relcache.c */
 extern void RelationIncrementReferenceCount(Relation rel);
 extern void RelationDecrementReferenceCount(Relation rel);
-- 
GitLab