Code review for row security.

Buildfarm member tick identified an issue where the policies in the
relcache for a relation were were being replaced underneath a running
query, leading to segfaults while processing the policies to be added
to a query.  Similar to how TupleDesc RuleLocks are handled, add in a
equalRSDesc() function to check if the policies have actually changed
and, if not, swap back the rsdesc field (using the original instead of
the temporairly built one; the whole structure is swapped and then
specific fields swapped back).  This now passes a CLOBBER_CACHE_ALWAYS
for me and should resolve the buildfarm error.

In addition to addressing this, add a new chapter in Data Definition
under Privileges which explains row security and provides examples of
its usage, change \d to always list policies (even if row security is
disabled- but note that it is disabled, or enabled with no policies),
rework check_role_for_policy (it really didn't need the entire policy,
but it did need to be using has_privs_of_role()), and change the field
in pg_class to relrowsecurity from relhasrowsecurity, based on
Heikki's suggestion.  Also from Heikki, only issue SET ROW_SECURITY in
pg_restore when talking to a 9.5+ server, list Bypass RLS in \du, and
document --enable-row-security options for pg_dump and pg_restore.

Lastly, fix a number of minor whitespace and typo issues from Heikki,
Dimitri, add a missing #include, per Peter E, fix a few minor
variable-assigned-but-not-used and resource leak issues from Coverity
and add tab completion for role attribute bypassrls as well.

src/backend/catalog/heap.c

@@ -799,7 +799,7 @@ InsertPgClassTuple(Relation pg_class_desc,
 	values[Anum_pg_class_relhaspkey - 1] = BoolGetDatum(rd_rel->relhaspkey);
 	values[Anum_pg_class_relhasrules - 1] = BoolGetDatum(rd_rel->relhasrules);
 	values[Anum_pg_class_relhastriggers - 1] = BoolGetDatum(rd_rel->relhastriggers);
-	values[Anum_pg_class_relhasrowsecurity - 1] = BoolGetDatum(rd_rel->relhasrowsecurity);
+	values[Anum_pg_class_relrowsecurity - 1] = BoolGetDatum(rd_rel->relrowsecurity);
 	values[Anum_pg_class_relhassubclass - 1] = BoolGetDatum(rd_rel->relhassubclass);
 	values[Anum_pg_class_relispopulated - 1] = BoolGetDatum(rd_rel->relispopulated);
 	values[Anum_pg_class_relreplident - 1] = CharGetDatum(rd_rel->relreplident);

src/backend/catalog/system_views.sql

@@ -119,7 +119,7 @@ CREATE VIEW pg_tables AS
         C.relhasindex AS hasindexes,
         C.relhasrules AS hasrules,
         C.relhastriggers AS hastriggers,
-        C.relhasrowsecurity AS hasrowsecurity
+        C.relrowsecurity AS rowsecurity
     FROM pg_class C LEFT JOIN pg_namespace N ON (N.oid = C.relnamespace)
          LEFT JOIN pg_tablespace T ON (T.oid = C.reltablespace)
     WHERE C.relkind = 'r';

src/backend/commands/policy.c

@@ -108,7 +108,7 @@ parse_row_security_command(const char *cmd_name)
 	char cmd;
 
 	if (!cmd_name)
-		elog(ERROR, "Unregonized command.");
+		elog(ERROR, "unregonized command");
 
 	if (strcmp(cmd_name, "all") == 0)
 		cmd = 0;
@@ -121,8 +121,7 @@ parse_row_security_command(const char *cmd_name)
 	else if (strcmp(cmd_name, "delete") == 0)
 		cmd = ACL_DELETE_CHR;
 	else
-		elog(ERROR, "Unregonized command.");
-		/* error unrecognized command */
+		elog(ERROR, "unregonized command");
 
 	return cmd;
 }
@@ -422,8 +421,8 @@ RemovePolicyById(Oid policy_id)
 	heap_close(rel, AccessExclusiveLock);
 
 	/*
-	 * Note that, unlike some of the other flags in pg_class, relhasrowsecurity
-	 * is not just an indication of if policies exist.  When relhasrowsecurity
+	 * Note that, unlike some of the other flags in pg_class, relrowsecurity
+	 * is not just an indication of if policies exist.  When relrowsecurity
 	 * is set (which can be done directly by the user or indirectly by creating
 	 * a policy on the table), then all access to the relation must be through
 	 * a policy.  If no policy is defined for the relation then a default-deny
@@ -484,7 +483,7 @@ CreatePolicy(CreatePolicyStmt *stmt)
 	if (rseccmd == ACL_INSERT_CHR && stmt->qual != NULL)
 		ereport(ERROR,
 				(errcode(ERRCODE_SYNTAX_ERROR),
-				 errmsg("Only WITH CHECK expression allowed for INSERT")));
+				 errmsg("only WITH CHECK expression allowed for INSERT")));
 
 
 	/* Collect role ids */
@@ -731,7 +730,7 @@ AlterPolicy(AlterPolicyStmt *stmt)
 	if (!HeapTupleIsValid(rsec_tuple))
 		ereport(ERROR,
 				(errcode(ERRCODE_UNDEFINED_OBJECT),
-				 errmsg("policy '%s' for does not exist on table %s",
+				 errmsg("policy \"%s\" on table \"%s\" does not exist",
 						stmt->policy_name,
 						RelationGetRelationName(target_table))));
 
@@ -850,7 +849,7 @@ rename_policy(RenameStmt *stmt)
 
 	pg_rowsecurity_rel = heap_open(RowSecurityRelationId, RowExclusiveLock);
 
-	/* First pass- check for conflict */
+	/* First pass -- check for conflict */
 
 	/* Add key - row security relation id. */
 	ScanKeyInit(&skey[0],
@@ -868,7 +867,7 @@ rename_policy(RenameStmt *stmt)
 							   RowSecurityRelidPolnameIndexId, true, NULL, 2,
 							   skey);
 
-	if (HeapTupleIsValid(rsec_tuple = systable_getnext(sscan)))
+	if (HeapTupleIsValid(systable_getnext(sscan)))
 		ereport(ERROR,
 				(errcode(ERRCODE_DUPLICATE_OBJECT),
 				 errmsg("row-policy \"%s\" for table \"%s\" already exists",

src/backend/commands/tablecmds.c

@@ -10647,7 +10647,7 @@ ATExecEnableRowSecurity(Relation rel)
 	if (!HeapTupleIsValid(tuple))
 		elog(ERROR, "cache lookup failed for relation %u", relid);
 
-	((Form_pg_class) GETSTRUCT(tuple))->relhasrowsecurity = true;
+	((Form_pg_class) GETSTRUCT(tuple))->relrowsecurity = true;
 	simple_heap_update(pg_class, &tuple->t_self, tuple);
 
 	/* keep catalog indexes current */
@@ -10674,7 +10674,7 @@ ATExecDisableRowSecurity(Relation rel)
 	if (!HeapTupleIsValid(tuple))
 		elog(ERROR, "cache lookup failed for relation %u", relid);
 
-	((Form_pg_class) GETSTRUCT(tuple))->relhasrowsecurity = false;
+	((Form_pg_class) GETSTRUCT(tuple))->relrowsecurity = false;
 	simple_heap_update(pg_class, &tuple->t_self, tuple);
 
 	/* keep catalog indexes current */

src/backend/rewrite/rowsecurity.c

@@ -61,7 +61,7 @@ static void process_policies(List *policies, int rt_index,
 							 Expr **final_qual,
 							 Expr **final_with_check_qual,
 							 bool *hassublinks);
-static bool check_role_for_policy(RowSecurityPolicy *policy, Oid user_id);
+static bool check_role_for_policy(ArrayType *policy_roles, Oid user_id);
 
 /*
  * hook to allow extensions to apply their own security policy
@@ -177,7 +177,7 @@ prepend_row_security_policies(Query* root, RangeTblEntry* rte, int rt_index)
 	 * all of them OR'd together.  However, to avoid the situation of an
 	 * extension granting more access to a table than the internal policies
 	 * would allow, the extension's policies are AND'd with the internal
-	 * policies.  In other words- extensions can only provide further
+	 * policies.  In other words - extensions can only provide further
 	 * filtering of the result set (or further reduce the set of records
 	 * allowed to be added).
 	 *
@@ -305,7 +305,8 @@ 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' && check_role_for_policy(policy, user_id))
+		if (policy->cmd == '\0' &&
+				check_role_for_policy(policy->roles, user_id))
 			policies = lcons(policy, policies);
 
 		/* Build the list of policies to return. */
@@ -313,23 +314,23 @@ pull_row_security_policies(CmdType cmd, Relation relation, Oid user_id)
 		{
 			case CMD_SELECT:
 				if (policy->cmd == ACL_SELECT_CHR
-					&& check_role_for_policy(policy, user_id))
+					&& 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
-					&& check_role_for_policy(policy, user_id))
+					&& check_role_for_policy(policy->roles, user_id))
 					policies = lcons(policy, policies);
 				break;
 			case CMD_UPDATE:
 				if (policy->cmd == ACL_UPDATE_CHR
-					&& check_role_for_policy(policy, user_id))
+					&& check_role_for_policy(policy->roles, user_id))
 					policies = lcons(policy, policies);
 				break;
 			case CMD_DELETE:
 				if (policy->cmd == ACL_DELETE_CHR
-					&& check_role_for_policy(policy, user_id))
+					&& check_role_for_policy(policy->roles, user_id))
 					policies = lcons(policy, policies);
 				break;
 			default:
@@ -473,7 +474,7 @@ check_enable_rls(Oid relid, Oid checkAsUser)
 {
 	HeapTuple		tuple;
 	Form_pg_class	classform;
-	bool			relhasrowsecurity;
+	bool			relrowsecurity;
 	Oid				user_id = checkAsUser ? checkAsUser : GetUserId();
 
 	tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
@@ -482,12 +483,12 @@ check_enable_rls(Oid relid, Oid checkAsUser)
 
 	classform = (Form_pg_class) GETSTRUCT(tuple);
 
-	relhasrowsecurity = classform->relhasrowsecurity;
+	relrowsecurity = classform->relrowsecurity;
 
 	ReleaseSysCache(tuple);
 
 	/* Nothing to do if the relation does not have RLS */
-	if (!relhasrowsecurity)
+	if (!relrowsecurity)
 		return RLS_NONE;
 
 	/*
@@ -537,19 +538,19 @@ check_enable_rls(Oid relid, Oid checkAsUser)
  * check_role_for_policy -
  *   determines if the policy should be applied for the current role
  */
-bool
-check_role_for_policy(RowSecurityPolicy *policy, Oid user_id)
+static bool
+check_role_for_policy(ArrayType *policy_roles, Oid user_id)
 {
 	int			i;
-	Oid		   *roles = (Oid *) ARR_DATA_PTR(policy->roles);
+	Oid		   *roles = (Oid *) ARR_DATA_PTR(policy_roles);
 
 	/* Quick fall-thru for policies applied to all roles */
 	if (roles[0] == ACL_ID_PUBLIC)
 		return true;
 
-	for (i = 0; i < ARR_DIMS(policy->roles)[0]; i++)
+	for (i = 0; i < ARR_DIMS(policy_roles)[0]; i++)
 	{
-		if (is_member_of_role(user_id, roles[i]))
+		if (has_privs_of_role(user_id, roles[i]))
 			return true;
 	}
 

src/backend/utils/adt/ri_triggers.c

@@ -2309,9 +2309,9 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel)
 	 * have RLS enabled.
 	 */
 	if (!has_bypassrls_privilege(GetUserId()) &&
-		((pk_rel->rd_rel->relhasrowsecurity &&
+		((pk_rel->rd_rel->relrowsecurity &&
 		  !pg_class_ownercheck(pkrte->relid, GetUserId())) ||
-		 (fk_rel->rd_rel->relhasrowsecurity &&
+		 (fk_rel->rd_rel->relrowsecurity &&
 		  !pg_class_ownercheck(fkrte->relid, GetUserId()))))
 		return false;
 

src/backend/utils/cache/relcache.c

@@ -847,6 +847,87 @@ equalRuleLocks(RuleLock *rlock1, RuleLock *rlock2)
 	return true;
 }
 
+/*
+ *		equalPolicy
+ *
+ *		Determine whether two policies are equivalent
+ */
+static bool
+equalPolicy(RowSecurityPolicy *policy1, RowSecurityPolicy *policy2)
+{
+	int			i;
+	Oid        *r1,
+	           *r2;
+
+	if (policy1 != NULL)
+	{
+		if (policy2 == NULL)
+			return false;
+
+		if (policy1->rsecid != policy2->rsecid)
+			return false;
+		if (policy1->cmd != policy2->cmd)
+			return false;
+		if (policy1->hassublinks != policy2->hassublinks);
+			return false;
+		if (strcmp(policy1->policy_name,policy2->policy_name) != 0)
+			return false;
+		if (ARR_DIMS(policy1->roles)[0] != ARR_DIMS(policy2->roles)[0])
+			return false;
+
+		r1 = (Oid *) ARR_DATA_PTR(policy1->roles);
+		r2 = (Oid *) ARR_DATA_PTR(policy2->roles);
+
+		for (i = 0; i < ARR_DIMS(policy1->roles)[0]; i++)
+		{
+			if (r1[i] != r2[i])
+				return false;
+		}
+
+		if (!equal(policy1->qual, policy1->qual))
+			return false;
+		if (!equal(policy1->with_check_qual, policy2->with_check_qual))
+			return false;
+	}
+	else if (policy2 != NULL)
+		return false;
+
+	return true;
+}
+
+/*
+ *		equalRSDesc
+ *
+ *		Determine whether two RowSecurityDesc's are equivalent
+ */
+static bool
+equalRSDesc(RowSecurityDesc *rsdesc1, RowSecurityDesc *rsdesc2)
+{
+	ListCell 	*lc,
+				*rc;
+
+	if (rsdesc1 == NULL && rsdesc2 == NULL)
+		return true;
+
+	if ((rsdesc1 != NULL && rsdesc2 == NULL) ||
+		(rsdesc1 == NULL && rsdesc2 != NULL))
+		return false;
+
+	if (list_length(rsdesc1->policies) != list_length(rsdesc2->policies))
+		return false;
+
+	/* RelationBuildRowSecurity should build policies in order */
+	forboth(lc, rsdesc1->policies, rc, rsdesc2->policies)
+	{
+		RowSecurityPolicy	   *l = (RowSecurityPolicy *) lfirst(lc);
+		RowSecurityPolicy	   *r = (RowSecurityPolicy *) lfirst(rc);
+
+		if (!equalPolicy(l,r))
+			return false;
+	}
+
+	return false;
+}
 
 /*
  *		RelationBuildDesc
@@ -967,7 +1048,7 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
 	else
 		relation->trigdesc = NULL;
 
-	if (relation->rd_rel->relhasrowsecurity)
+	if (relation->rd_rel->relrowsecurity)
 		RelationBuildRowSecurity(relation);
 	else
 		relation->rsdesc = NULL;
@@ -2104,6 +2185,7 @@ RelationClearRelation(Relation relation, bool rebuild)
 		Oid			save_relid = RelationGetRelid(relation);
 		bool		keep_tupdesc;
 		bool		keep_rules;
+		bool		keep_policies;
 
 		/* Build temporary entry, but don't link it into hashtable */
 		newrel = RelationBuildDesc(save_relid, false);
@@ -2117,6 +2199,7 @@ RelationClearRelation(Relation relation, bool rebuild)
 
 		keep_tupdesc = equalTupleDescs(relation->rd_att, newrel->rd_att);
 		keep_rules = equalRuleLocks(relation->rd_rules, newrel->rd_rules);
+		keep_policies = equalRSDesc(relation->rsdesc, newrel->rsdesc);
 
 		/*
 		 * Perform swapping of the relcache entry contents.  Within this
@@ -2165,6 +2248,8 @@ RelationClearRelation(Relation relation, bool rebuild)
 			SWAPFIELD(RuleLock *, rd_rules);
 			SWAPFIELD(MemoryContext, rd_rulescxt);
 		}
+		if (keep_policies)
+			SWAPFIELD(RowSecurityDesc *, rsdesc);
 		/* toast OID override must be preserved */
 		SWAPFIELD(Oid, rd_toastoid);
 		/* pgstat_info must be preserved */
@@ -3345,11 +3430,11 @@ RelationCacheInitializePhase3(void)
 		/*
 		 * Re-load the row security policies if the relation has them, since
 		 * they are not preserved in the cache.  Note that we can never NOT
-		 * have a policy while relhasrowsecurity is true-
+		 * have a policy while relrowsecurity is true,
 		 * RelationBuildRowSecurity will create a single default-deny policy
 		 * if there is no policy defined in pg_rowsecurity.
 		 */
-		if (relation->rd_rel->relhasrowsecurity && relation->rsdesc == NULL)
+		if (relation->rd_rel->relrowsecurity && relation->rsdesc == NULL)
 		{
 			RelationBuildRowSecurity(relation);