Clean up some mess in row-security patches.

Fix unsafe coding around PG_TRY in RelationBuildRowSecurity: can't change
a variable inside PG_TRY and then use it in PG_CATCH without marking it
"volatile".  In this case though it seems saner to avoid that by doing
a single assignment before entering the TRY block.

I started out just intending to fix that, but the more I looked at the
row-security code the more distressed I got.  This patch also fixes
incorrect construction of the RowSecurityPolicy cache entries (there was
not sufficient care taken to copy pass-by-ref data into the cache memory
context) and a whole bunch of sloppiness around the definition and use of
pg_policy.polcmd.  You can't use nulls in that column because initdb will
mark it NOT NULL --- and I see no particular reason why a null entry would
be a good idea anyway, so changing initdb's behavior is not the right
answer.  The internal value of '\0' wouldn't be suitable in a "char" column
either, so after a bit of thought I settled on using '*' to represent ALL.
Chasing those changes down also revealed that somebody wasn't paying
attention to what the underlying values of ACL_UPDATE_CHR etc really were,
and there was a great deal of lackadaiscalness in the catalogs.sgml
documentation for pg_policy and pg_policies too.

This doesn't pretend to be a complete code review for the row-security
stuff, it just fixes the things that were in my face while dealing with
the bugs in RelationBuildRowSecurity.

4 files changed, 98 insertions, 91 deletions

diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 4bc874f5c25..6df6bf27d19 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -79,13 +79,12 @@ CREATE VIEW pg_policies AS
                     WHERE oid = ANY (pol.polroles) ORDER BY 1
                 )
         END AS roles,
-        CASE WHEN pol.polcmd IS NULL THEN 'ALL' ELSE
-            CASE pol.polcmd
-                WHEN 'r' THEN 'SELECT'
-                WHEN 'a' THEN 'INSERT'
-                WHEN 'u' THEN 'UPDATE'
-                WHEN 'd' THEN 'DELETE'
-            END
+        CASE pol.polcmd
+            WHEN 'r' THEN 'SELECT'
+            WHEN 'a' THEN 'INSERT'
+            WHEN 'w' THEN 'UPDATE'
+            WHEN 'd' THEN 'DELETE'
+            WHEN '*' THEN 'ALL'
         END AS cmd,
         pg_catalog.pg_get_expr(pol.polqual, pol.polrelid) AS qual,
         pg_catalog.pg_get_expr(pol.polwithcheck, pol.polrelid) AS with_check
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index 9b79d886332..d98da0dd506 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -28,10 +28,10 @@
 #include "miscadmin.h"
 #include "nodes/makefuncs.h"
 #include "nodes/pg_list.h"
-#include "optimizer/clauses.h"
 #include "parser/parse_clause.h"
 #include "parser/parse_node.h"
 #include "parser/parse_relation.h"
+#include "rewrite/rewriteManip.h"
 #include "rewrite/rowsecurity.h"
 #include "storage/lock.h"
 #include "utils/acl.h"
@@ -109,10 +109,10 @@ parse_policy_command(const char *cmd_name)
 	char cmd;
 
 	if (!cmd_name)
-		elog(ERROR, "unrecognized command");
+		elog(ERROR, "unrecognized policy command");
 
 	if (strcmp(cmd_name, "all") == 0)
-		cmd = 0;
+		cmd = '*';
 	else if (strcmp(cmd_name, "select") == 0)
 		cmd = ACL_SELECT_CHR;
 	else if (strcmp(cmd_name, "insert") == 0)
@@ -122,7 +122,7 @@ parse_policy_command(const char *cmd_name)
 	else if (strcmp(cmd_name, "delete") == 0)
 		cmd = ACL_DELETE_CHR;
 	else
-		elog(ERROR, "unrecognized command");
+		elog(ERROR, "unrecognized policy command");
 
 	return cmd;
 }
@@ -190,44 +190,54 @@ policy_role_list_to_array(List *roles)
 }
 
 /*
- * Load row security policy from the catalog, and keep it in
- * the relation cache.
+ * Load row security policy from the catalog, and store it in
+ * the relation's relcache entry.
+ *
+ * We will always set up some kind of policy here.  If no explicit policies
+ * are found then an implicit default-deny policy is created.
  */
 void
 RelationBuildRowSecurity(Relation relation)
 {
-	Relation			catalog;
-	ScanKeyData			skey;
-	SysScanDesc			sscan;
-	HeapTuple			tuple;
-	MemoryContext		oldcxt;
-	MemoryContext		rscxt = NULL;
-	RowSecurityDesc	   *rsdesc = NULL;
-
-	catalog = heap_open(PolicyRelationId, AccessShareLock);
+	MemoryContext		rscxt;
+	MemoryContext		oldcxt = CurrentMemoryContext;
+	RowSecurityDesc	   * volatile rsdesc = NULL;
 
-	ScanKeyInit(&skey,
-				Anum_pg_policy_polrelid,
-				BTEqualStrategyNumber, F_OIDEQ,
-				ObjectIdGetDatum(RelationGetRelid(relation)));
+	/*
+	 * 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.
+	 */
+	rscxt = AllocSetContextCreate(CacheMemoryContext,
+								  "row security descriptor",
+								  ALLOCSET_SMALL_MINSIZE,
+								  ALLOCSET_SMALL_INITSIZE,
+								  ALLOCSET_SMALL_MAXSIZE);
 
-	sscan = systable_beginscan(catalog, PolicyPolrelidPolnameIndexId, true,
-							   NULL, 1, &skey);
+	/*
+	 * 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.
+	 */
 	PG_TRY();
 	{
-		/*
-		 * Set up our memory context- we will always set up some kind of
-		 * policy here.  If no explicit policies are found then an implicit
-		 * default-deny policy is created.
-		 */
-		rscxt = AllocSetContextCreate(CacheMemoryContext,
-									  "row security descriptor",
-									  ALLOCSET_SMALL_MINSIZE,
-									  ALLOCSET_SMALL_INITSIZE,
-									  ALLOCSET_SMALL_MAXSIZE);
+		Relation			catalog;
+		ScanKeyData			skey;
+		SysScanDesc			sscan;
+		HeapTuple			tuple;
+
 		rsdesc = MemoryContextAllocZero(rscxt, sizeof(RowSecurityDesc));
 		rsdesc->rscxt = rscxt;
 
+		catalog = heap_open(PolicyRelationId, AccessShareLock);
+
+		ScanKeyInit(&skey,
+					Anum_pg_policy_polrelid,
+					BTEqualStrategyNumber, F_OIDEQ,
+					ObjectIdGetDatum(RelationGetRelid(relation)));
+
+		sscan = systable_beginscan(catalog, PolicyPolrelidPolnameIndexId, true,
+								   NULL, 1, &skey);
+
 		/*
 		 * Loop through the row level security policies for this relation, if
 		 * any.
@@ -236,7 +246,7 @@ RelationBuildRowSecurity(Relation relation)
 		{
 			Datum				value_datum;
 			char				cmd_value;
-			ArrayType		   *roles;
+			Datum				roles_datum;
 			char			   *qual_value;
 			Expr			   *qual_expr;
 			char			   *with_check_value;
@@ -244,29 +254,33 @@ RelationBuildRowSecurity(Relation relation)
 			char			   *policy_name_value;
 			Oid					policy_id;
 			bool				isnull;
-			RowSecurityPolicy  *policy = NULL;
+			RowSecurityPolicy  *policy;
 
-			oldcxt = MemoryContextSwitchTo(rscxt);
+			/*
+			 * 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);
-			if (isnull)
-				cmd_value = 0;
-			else
-				cmd_value = DatumGetChar(value_datum);
+			Assert(!isnull);
+			cmd_value = DatumGetChar(value_datum);
 
 			/* Get policy name */
 			value_datum = heap_getattr(tuple, Anum_pg_policy_polname,
 										RelationGetDescr(catalog), &isnull);
 			Assert(!isnull);
-			policy_name_value = DatumGetCString(value_datum);
+			policy_name_value = NameStr(*(DatumGetName(value_datum)));
 
 			/* Get policy roles */
-			value_datum = heap_getattr(tuple, Anum_pg_policy_polroles,
+			roles_datum = heap_getattr(tuple, Anum_pg_policy_polroles,
 										RelationGetDescr(catalog), &isnull);
-			Assert(!isnull);
-			roles = DatumGetArrayTypeP(value_datum);
+			/* 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,
@@ -282,7 +296,6 @@ RelationBuildRowSecurity(Relation relation)
 			/* Get WITH CHECK qual */
 			value_datum = heap_getattr(tuple, Anum_pg_policy_polwithcheck,
 										RelationGetDescr(catalog), &isnull);
-
 			if (!isnull)
 			{
 				with_check_value = TextDatumGetCString(value_datum);
@@ -293,27 +306,33 @@ RelationBuildRowSecurity(Relation relation)
 
 			policy_id = HeapTupleGetOid(tuple);
 
+			/* Now copy everything into the cache context */
+			MemoryContextSwitchTo(rscxt);
+
 			policy = palloc0(sizeof(RowSecurityPolicy));
-			policy->policy_name = policy_name_value;
+			policy->policy_name = pstrdup(policy_name_value);
 			policy->policy_id = policy_id;
-			policy->cmd = cmd_value;
-			policy->roles = roles;
+			policy->polcmd = cmd_value;
+			policy->roles = DatumGetArrayTypePCopy(roles_datum);
 			policy->qual = copyObject(qual_expr);
 			policy->with_check_qual = copyObject(with_check_qual);
-			policy->hassublinks = contain_subplans((Node *) qual_expr) ||
-								  contain_subplans((Node *) with_check_qual);
+			policy->hassublinks = checkExprHasSubLink((Node *) qual_expr) ||
+								  checkExprHasSubLink((Node *) with_check_qual);
 
 			rsdesc->policies = lcons(policy, rsdesc->policies);
 
 			MemoryContextSwitchTo(oldcxt);
 
+			/* clean up some (not all) of the junk ... */
 			if (qual_expr != NULL)
 				pfree(qual_expr);
-
 			if (with_check_qual != NULL)
 				pfree(with_check_qual);
 		}
 
+		systable_endscan(sscan);
+		heap_close(catalog, AccessShareLock);
+
 		/*
 		 * Check if no policies were added
 		 *
@@ -324,17 +343,17 @@ RelationBuildRowSecurity(Relation relation)
 		 */
 		if (rsdesc->policies == NIL)
 		{
-			RowSecurityPolicy  *policy = NULL;
+			RowSecurityPolicy  *policy;
 			Datum				role;
 
-			oldcxt = MemoryContextSwitchTo(rscxt);
+			MemoryContextSwitchTo(rscxt);
 
 			role = ObjectIdGetDatum(ACL_ID_PUBLIC);
 
 			policy = palloc0(sizeof(RowSecurityPolicy));
 			policy->policy_name = pstrdup("default-deny policy");
 			policy->policy_id = InvalidOid;
-			policy->cmd = '\0';
+			policy->polcmd = '*';
 			policy->roles = construct_array(&role, 1, OIDOID, sizeof(Oid), true,
 											'i');
 			policy->qual = (Expr *) makeConst(BOOLOID, -1, InvalidOid,
@@ -350,15 +369,14 @@ RelationBuildRowSecurity(Relation relation)
 	}
 	PG_CATCH();
 	{
-		if (rscxt != NULL)
-			MemoryContextDelete(rscxt);
+		/* Delete rscxt, first making sure it isn't active */
+		MemoryContextSwitchTo(oldcxt);
+		MemoryContextDelete(rscxt);
 		PG_RE_THROW();
 	}
 	PG_END_TRY();
 
-	systable_endscan(sscan);
-	heap_close(catalog, AccessShareLock);
-
+	/* Success --- attach the policy descriptor to the relcache entry */
 	relation->rd_rsdesc = rsdesc;
 }
 
@@ -555,27 +573,20 @@ CreatePolicy(CreatePolicyStmt *stmt)
 				 stmt->policy_name, RelationGetRelationName(target_table))));
 
 	values[Anum_pg_policy_polrelid - 1] = ObjectIdGetDatum(table_id);
-	values[Anum_pg_policy_polname - 1]
-		= DirectFunctionCall1(namein, CStringGetDatum(stmt->policy_name));
-
-	if (polcmd)
-		values[Anum_pg_policy_polcmd - 1] = CharGetDatum(polcmd);
-	else
-		isnull[Anum_pg_policy_polcmd - 1] = true;
-
+	values[Anum_pg_policy_polname - 1] = DirectFunctionCall1(namein,
+															 CStringGetDatum(stmt->policy_name));
+	values[Anum_pg_policy_polcmd - 1] = CharGetDatum(polcmd);
 	values[Anum_pg_policy_polroles - 1] = PointerGetDatum(role_ids);
 
 	/* Add qual if present. */
 	if (qual)
-		values[Anum_pg_policy_polqual - 1]
-			= CStringGetTextDatum(nodeToString(qual));
+		values[Anum_pg_policy_polqual - 1] = CStringGetTextDatum(nodeToString(qual));
 	else
 		isnull[Anum_pg_policy_polqual - 1] = true;
 
 	/* Add WITH CHECK qual if present */
 	if (with_check_qual)
-		values[Anum_pg_policy_polwithcheck - 1]
-			= CStringGetTextDatum(nodeToString(with_check_qual));
+		values[Anum_pg_policy_polwithcheck - 1] = CStringGetTextDatum(nodeToString(with_check_qual));
 	else
 		isnull[Anum_pg_policy_polwithcheck - 1] = true;
 
@@ -738,10 +749,8 @@ AlterPolicy(AlterPolicyStmt *stmt)
 	cmd_datum = heap_getattr(policy_tuple, Anum_pg_policy_polcmd,
 							 RelationGetDescr(pg_policy_rel),
 							 &polcmd_isnull);
-	if (polcmd_isnull)
-		polcmd = 0;
-	else
-		polcmd = DatumGetChar(cmd_datum);
+	Assert(!polcmd_isnull);
+	polcmd = DatumGetChar(cmd_datum);
 
 	/*
 	 * If the command is SELECT or DELETE then WITH CHECK should be NULL.
diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c
index 35790a948f5..8f8e291fb88 100644
--- a/src/backend/rewrite/rowsecurity.c
+++ b/src/backend/rewrite/rowsecurity.c
@@ -273,8 +273,7 @@ prepend_row_security_policies(Query* root, RangeTblEntry* rte, int rt_index)
 	 * query, then set hasSubLinks on the Query to force subLinks to be
 	 * properly expanded.
 	 */
-	if (hassublinks)
-		root->hasSubLinks = hassublinks;
+	root->hasSubLinks |= hassublinks;
 
 	/* If we got this far, we must have added quals */
 	return true;
@@ -305,36 +304,36 @@ pull_row_security_policies(CmdType cmd, Relation relation, Oid user_id)
 		policy = (RowSecurityPolicy *) lfirst(item);
 
 		/* Always add ALL policies, if they exist. */
-		if (policy->cmd == '\0' &&
+		if (policy->polcmd == '*' &&
 				check_role_for_policy(policy->roles, user_id))
 			policies = lcons(policy, policies);
 
-		/* Build the list of policies to return. */
+		/* Add relevant command-specific policies to the list. */
 		switch(cmd)
 		{
 			case CMD_SELECT:
-				if (policy->cmd == ACL_SELECT_CHR
+				if (policy->polcmd == ACL_SELECT_CHR
 					&& check_role_for_policy(policy->roles, user_id))
 					policies = lcons(policy, policies);
 				break;
 			case CMD_INSERT:
 				/* If INSERT then only need to add the WITH CHECK qual */
-				if (policy->cmd == ACL_INSERT_CHR
+				if (policy->polcmd == ACL_INSERT_CHR
 					&& check_role_for_policy(policy->roles, user_id))
 					policies = lcons(policy, policies);
 				break;
 			case CMD_UPDATE:
-				if (policy->cmd == ACL_UPDATE_CHR
+				if (policy->polcmd == ACL_UPDATE_CHR
 					&& check_role_for_policy(policy->roles, user_id))
 					policies = lcons(policy, policies);
 				break;
 			case CMD_DELETE:
-				if (policy->cmd == ACL_DELETE_CHR
+				if (policy->polcmd == ACL_DELETE_CHR
 					&& check_role_for_policy(policy->roles, user_id))
 					policies = lcons(policy, policies);
 				break;
 			default:
-				elog(ERROR, "unrecognized command type.");
+				elog(ERROR, "unrecognized policy command type %d", (int) cmd);
 				break;
 		}
 	}
@@ -354,7 +353,7 @@ pull_row_security_policies(CmdType cmd, Relation relation, Oid user_id)
 		policy = palloc0(sizeof(RowSecurityPolicy));
 		policy->policy_name = pstrdup("default-deny policy");
 		policy->policy_id = InvalidOid;
-		policy->cmd = '\0';
+		policy->polcmd = '*';
 		policy->roles = construct_array(&role, 1, OIDOID, sizeof(Oid), true,
 										'i');
 		policy->qual = (Expr *) makeConst(BOOLOID, -1, InvalidOid,
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 24c92e791ff..1db4ba84100 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -868,7 +868,7 @@ equalPolicy(RowSecurityPolicy *policy1, RowSecurityPolicy *policy2)
 
 		if (policy1->policy_id != policy2->policy_id)
 			return false;
-		if (policy1->cmd != policy2->cmd)
+		if (policy1->polcmd != policy2->polcmd)
 			return false;
 		if (policy1->hassublinks != policy2->hassublinks)
 			return false;