1 files changed, 98 insertions, 118 deletions

diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index 4b4e4694930..d3f8e8f06c1 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -187,159 +187,139 @@ policy_role_list_to_array(List *roles, int *num_roles)
 /*
  * Load row security policy from the catalog, and store it in
  * the relation's relcache entry.
+ *
+ * Note that caller should have verified that pg_class.relrowsecurity
+ * is true for this relation.
  */
 void
 RelationBuildRowSecurity(Relation relation)
 {
 	MemoryContext rscxt;
 	MemoryContext oldcxt = CurrentMemoryContext;
-	RowSecurityDesc *volatile rsdesc = NULL;
+	RowSecurityDesc *rsdesc;
+	Relation	catalog;
+	ScanKeyData skey;
+	SysScanDesc sscan;
+	HeapTuple	tuple;
 
 	/*
 	 * Create a memory context to hold everything associated with this
 	 * relation's row security policy.  This makes it easy to clean up during
-	 * a relcache flush.
+	 * a relcache flush.  However, to cover the possibility of an error
+	 * partway through, we don't make the context long-lived till we're done.
 	 */
-	rscxt = AllocSetContextCreate(CacheMemoryContext,
+	rscxt = AllocSetContextCreate(CurrentMemoryContext,
 								  "row security descriptor",
 								  ALLOCSET_SMALL_SIZES);
+	MemoryContextCopyAndSetIdentifier(rscxt,
+									  RelationGetRelationName(relation));
+
+	rsdesc = MemoryContextAllocZero(rscxt, sizeof(RowSecurityDesc));
+	rsdesc->rscxt = rscxt;
 
 	/*
-	 * Since rscxt lives under CacheMemoryContext, it is long-lived.  Use a
-	 * PG_TRY block to ensure it'll get freed if we fail partway through.
+	 * Now scan pg_policy for RLS policies associated with this relation.
+	 * Because we use the index on (polrelid, polname), we should consistently
+	 * visit the rel's policies in name order, at least when system indexes
+	 * aren't disabled.  This simplifies equalRSDesc().
 	 */
-	PG_TRY();
-	{
-		Relation	catalog;
-		ScanKeyData skey;
-		SysScanDesc sscan;
-		HeapTuple	tuple;
-
-		MemoryContextCopyAndSetIdentifier(rscxt,
-										  RelationGetRelationName(relation));
+	catalog = table_open(PolicyRelationId, AccessShareLock);
 
-		rsdesc = MemoryContextAllocZero(rscxt, sizeof(RowSecurityDesc));
-		rsdesc->rscxt = rscxt;
+	ScanKeyInit(&skey,
+				Anum_pg_policy_polrelid,
+				BTEqualStrategyNumber, F_OIDEQ,
+				ObjectIdGetDatum(RelationGetRelid(relation)));
 
-		catalog = table_open(PolicyRelationId, AccessShareLock);
+	sscan = systable_beginscan(catalog, PolicyPolrelidPolnameIndexId, true,
+							   NULL, 1, &skey);
 
-		ScanKeyInit(&skey,
-					Anum_pg_policy_polrelid,
-					BTEqualStrategyNumber, F_OIDEQ,
-					ObjectIdGetDatum(RelationGetRelid(relation)));
+	while (HeapTupleIsValid(tuple = systable_getnext(sscan)))
+	{
+		Form_pg_policy policy_form = (Form_pg_policy) GETSTRUCT(tuple);
+		RowSecurityPolicy *policy;
+		Datum		datum;
+		bool		isnull;
+		char	   *str_value;
 
-		sscan = systable_beginscan(catalog, PolicyPolrelidPolnameIndexId, true,
-								   NULL, 1, &skey);
+		policy = MemoryContextAllocZero(rscxt, sizeof(RowSecurityPolicy));
 
 		/*
-		 * Loop through the row level security policies for this relation, if
-		 * any.
+		 * Note: we must be sure that pass-by-reference data gets copied into
+		 * rscxt.  We avoid making that context current over wider spans than
+		 * we have to, though.
 		 */
-		while (HeapTupleIsValid(tuple = systable_getnext(sscan)))
-		{
-			Datum		value_datum;
-			char		cmd_value;
-			bool		permissive_value;
-			Datum		roles_datum;
-			char	   *qual_value;
-			Expr	   *qual_expr;
-			char	   *with_check_value;
-			Expr	   *with_check_qual;
-			char	   *policy_name_value;
-			bool		isnull;
-			RowSecurityPolicy *policy;
-
-			/*
-			 * Note: all the pass-by-reference data we collect here is either
-			 * still stored in the tuple, or constructed in the caller's
-			 * short-lived memory context.  We must copy it into rscxt
-			 * explicitly below.
-			 */
-
-			/* Get policy command */
-			value_datum = heap_getattr(tuple, Anum_pg_policy_polcmd,
-									   RelationGetDescr(catalog), &isnull);
-			Assert(!isnull);
-			cmd_value = DatumGetChar(value_datum);
-
-			/* Get policy permissive or restrictive */
-			value_datum = heap_getattr(tuple, Anum_pg_policy_polpermissive,
-									   RelationGetDescr(catalog), &isnull);
-			Assert(!isnull);
-			permissive_value = DatumGetBool(value_datum);
-
-			/* Get policy name */
-			value_datum = heap_getattr(tuple, Anum_pg_policy_polname,
-									   RelationGetDescr(catalog), &isnull);
-			Assert(!isnull);
-			policy_name_value = NameStr(*(DatumGetName(value_datum)));
-
-			/* Get policy roles */
-			roles_datum = heap_getattr(tuple, Anum_pg_policy_polroles,
-									   RelationGetDescr(catalog), &isnull);
-			/* shouldn't be null, but initdb doesn't mark it so, so check */
-			if (isnull)
-				elog(ERROR, "unexpected null value in pg_policy.polroles");
-
-			/* Get policy qual */
-			value_datum = heap_getattr(tuple, Anum_pg_policy_polqual,
-									   RelationGetDescr(catalog), &isnull);
-			if (!isnull)
-			{
-				qual_value = TextDatumGetCString(value_datum);
-				qual_expr = (Expr *) stringToNode(qual_value);
-			}
-			else
-				qual_expr = NULL;
 
-			/* Get WITH CHECK qual */
-			value_datum = heap_getattr(tuple, Anum_pg_policy_polwithcheck,
-									   RelationGetDescr(catalog), &isnull);
-			if (!isnull)
-			{
-				with_check_value = TextDatumGetCString(value_datum);
-				with_check_qual = (Expr *) stringToNode(with_check_value);
-			}
-			else
-				with_check_qual = NULL;
+		/* Get policy command */
+		policy->polcmd = policy_form->polcmd;
 
-			/* Now copy everything into the cache context */
-			MemoryContextSwitchTo(rscxt);
+		/* Get policy, permissive or restrictive */
+		policy->permissive = policy_form->polpermissive;
 
-			policy = palloc0(sizeof(RowSecurityPolicy));
-			policy->policy_name = pstrdup(policy_name_value);
-			policy->polcmd = cmd_value;
-			policy->permissive = permissive_value;
-			policy->roles = DatumGetArrayTypePCopy(roles_datum);
-			policy->qual = copyObject(qual_expr);
-			policy->with_check_qual = copyObject(with_check_qual);
-			policy->hassublinks = checkExprHasSubLink((Node *) qual_expr) ||
-				checkExprHasSubLink((Node *) with_check_qual);
+		/* Get policy name */
+		policy->policy_name =
+			MemoryContextStrdup(rscxt, NameStr(policy_form->polname));
 
-			rsdesc->policies = lcons(policy, rsdesc->policies);
+		/* Get policy roles */
+		datum = heap_getattr(tuple, Anum_pg_policy_polroles,
+							 RelationGetDescr(catalog), &isnull);
+		/* shouldn't be null, but let's check for luck */
+		if (isnull)
+			elog(ERROR, "unexpected null value in pg_policy.polroles");
+		MemoryContextSwitchTo(rscxt);
+		policy->roles = DatumGetArrayTypePCopy(datum);
+		MemoryContextSwitchTo(oldcxt);
 
+		/* Get policy qual */
+		datum = heap_getattr(tuple, Anum_pg_policy_polqual,
+							 RelationGetDescr(catalog), &isnull);
+		if (!isnull)
+		{
+			str_value = TextDatumGetCString(datum);
+			MemoryContextSwitchTo(rscxt);
+			policy->qual = (Expr *) stringToNode(str_value);
 			MemoryContextSwitchTo(oldcxt);
+			pfree(str_value);
+		}
+		else
+			policy->qual = NULL;
 
-			/* clean up some (not all) of the junk ... */
-			if (qual_expr != NULL)
-				pfree(qual_expr);
-			if (with_check_qual != NULL)
-				pfree(with_check_qual);
+		/* Get WITH CHECK qual */
+		datum = heap_getattr(tuple, Anum_pg_policy_polwithcheck,
+							 RelationGetDescr(catalog), &isnull);
+		if (!isnull)
+		{
+			str_value = TextDatumGetCString(datum);
+			MemoryContextSwitchTo(rscxt);
+			policy->with_check_qual = (Expr *) stringToNode(str_value);
+			MemoryContextSwitchTo(oldcxt);
+			pfree(str_value);
 		}
+		else
+			policy->with_check_qual = NULL;
 
-		systable_endscan(sscan);
-		table_close(catalog, AccessShareLock);
-	}
-	PG_CATCH();
-	{
-		/* Delete rscxt, first making sure it isn't active */
+		/* We want to cache whether there are SubLinks in these expressions */
+		policy->hassublinks = checkExprHasSubLink((Node *) policy->qual) ||
+			checkExprHasSubLink((Node *) policy->with_check_qual);
+
+		/*
+		 * Add this object to list.  For historical reasons, the list is built
+		 * in reverse order.
+		 */
+		MemoryContextSwitchTo(rscxt);
+		rsdesc->policies = lcons(policy, rsdesc->policies);
 		MemoryContextSwitchTo(oldcxt);
-		MemoryContextDelete(rscxt);
-		PG_RE_THROW();
 	}
-	PG_END_TRY();
 
-	/* Success --- attach the policy descriptor to the relcache entry */
+	systable_endscan(sscan);
+	table_close(catalog, AccessShareLock);
+
+	/*
+	 * Success.  Reparent the descriptor's memory context under
+	 * CacheMemoryContext so that it will live indefinitely, then attach the
+	 * policy descriptor to the relcache entry.
+	 */
+	MemoryContextSetParent(rscxt, CacheMemoryContext);
+
 	relation->rd_rsdesc = rsdesc;
 }