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.

1 files changed, 16 insertions, 15 deletions

diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c
index e1ccd1295e6..bb95b367198 100644
--- a/src/backend/rewrite/rowsecurity.c
+++ b/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;
 	}