RLS refactoring

This refactors rewrite/rowsecurity.c to simplify the handling of the
default deny case (reducing the number of places where we check for and
add the default deny policy from three to one) by splitting up the
retrival of the policies from the application of them.

This also allowed us to do away with the policy_id field.  A policy_name
field was added for WithCheckOption policies and is used in error
reporting, when available.

Patch by Dean Rasheed, with various mostly cosmetic changes by me.

Back-patch to 9.5 where RLS was introduced to avoid unnecessary
differences, since we're still in alpha, per discussion with Robert.

src/backend/commands/policy.c

@@ -186,9 +186,6 @@ 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.
- *
- * 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)
@@ -246,7 +243,6 @@ RelationBuildRowSecurity(Relation relation)
 			char	   *with_check_value;
 			Expr	   *with_check_qual;
 			char	   *policy_name_value;
-			Oid			policy_id;
 			bool		isnull;
 			RowSecurityPolicy *policy;
 
@@ -298,14 +294,11 @@ RelationBuildRowSecurity(Relation relation)
 			else
 				with_check_qual = NULL;
 
-			policy_id = HeapTupleGetOid(tuple);
-
 			/* Now copy everything into the cache context */
 			MemoryContextSwitchTo(rscxt);
 
 			policy = palloc0(sizeof(RowSecurityPolicy));
 			policy->policy_name = pstrdup(policy_name_value);
-			policy->policy_id = policy_id;
 			policy->polcmd = cmd_value;
 			policy->roles = DatumGetArrayTypePCopy(roles_datum);
 			policy->qual = copyObject(qual_expr);
@@ -326,40 +319,6 @@ RelationBuildRowSecurity(Relation relation)
 
 		systable_endscan(sscan);
 		heap_close(catalog, AccessShareLock);
-
-		/*
-		 * Check if no policies were added
-		 *
-		 * If no policies exist in pg_policy for this relation, then we need
-		 * to create a single default-deny policy.  We use InvalidOid for the
-		 * Oid to indicate that this is the default-deny policy (we may decide
-		 * to ignore the default policy if an extension adds policies).
-		 */
-		if (rsdesc->policies == NIL)
-		{
-			RowSecurityPolicy *policy;
-			Datum		role;
-
-			MemoryContextSwitchTo(rscxt);
-
-			role = ObjectIdGetDatum(ACL_ID_PUBLIC);
-
-			policy = palloc0(sizeof(RowSecurityPolicy));
-			policy->policy_name = pstrdup("default-deny policy");
-			policy->policy_id = InvalidOid;
-			policy->polcmd = '*';
-			policy->roles = construct_array(&role, 1, OIDOID, sizeof(Oid), true,
-											'i');
-			policy->qual = (Expr *) makeConst(BOOLOID, -1, InvalidOid,
-										   sizeof(bool), BoolGetDatum(false),
-											  false, true);
-			policy->with_check_qual = copyObject(policy->qual);
-			policy->hassublinks = false;
-
-			rsdesc->policies = lcons(policy, rsdesc->policies);
-
-			MemoryContextSwitchTo(oldcxt);
-		}
 	}
 	PG_CATCH();
 	{

src/backend/executor/execMain.c

@@ -1815,14 +1815,26 @@ ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo,
 					break;
 				case WCO_RLS_INSERT_CHECK:
 				case WCO_RLS_UPDATE_CHECK:
-					ereport(ERROR,
-							(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+					if (wco->polname != NULL)
+						ereport(ERROR,
+								(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+							 errmsg("new row violates row level security policy \"%s\" for \"%s\"",
+									wco->polname, wco->relname)));
+					else
+						ereport(ERROR,
+								(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 							 errmsg("new row violates row level security policy for \"%s\"",
 									wco->relname)));
 					break;
 				case WCO_RLS_CONFLICT_CHECK:
-					ereport(ERROR,
-							(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+					if (wco->polname != NULL)
+						ereport(ERROR,
+								(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+							 errmsg("new row violates row level security policy \"%s\" (USING expression) for \"%s\"",
+									wco->polname, wco->relname)));
+					else
+						ereport(ERROR,
+								(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 							 errmsg("new row violates row level security policy (USING expression) for \"%s\"",
 									wco->relname)));
 					break;

src/backend/nodes/copyfuncs.c

@@ -2168,6 +2168,7 @@ _copyWithCheckOption(const WithCheckOption *from)
 
 	COPY_SCALAR_FIELD(kind);
 	COPY_STRING_FIELD(relname);
+	COPY_STRING_FIELD(polname);
 	COPY_NODE_FIELD(qual);
 	COPY_SCALAR_FIELD(cascaded);
 

src/backend/nodes/equalfuncs.c

@@ -2455,6 +2455,7 @@ _equalWithCheckOption(const WithCheckOption *a, const WithCheckOption *b)
 {
 	COMPARE_SCALAR_FIELD(kind);
 	COMPARE_STRING_FIELD(relname);
+	COMPARE_STRING_FIELD(polname);
 	COMPARE_NODE_FIELD(qual);
 	COMPARE_SCALAR_FIELD(cascaded);
 

src/backend/nodes/outfuncs.c

@@ -2403,6 +2403,7 @@ _outWithCheckOption(StringInfo str, const WithCheckOption *node)
 
 	WRITE_ENUM_FIELD(kind, WCOKind);
 	WRITE_STRING_FIELD(relname);
+	WRITE_STRING_FIELD(polname);
 	WRITE_NODE_FIELD(qual);
 	WRITE_BOOL_FIELD(cascaded);
 }

src/backend/nodes/readfuncs.c

@@ -270,6 +270,7 @@ _readWithCheckOption(void)
 
 	READ_ENUM_FIELD(kind, WCOKind);
 	READ_STRING_FIELD(relname);
+	READ_STRING_FIELD(polname);
 	READ_NODE_FIELD(qual);
 	READ_BOOL_FIELD(cascaded);
 

src/backend/rewrite/rewriteHandler.c

@@ -1786,8 +1786,8 @@ fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
 		/*
 		 * Fetch any new security quals that must be applied to this RTE.
 		 */
-		get_row_security_policies(parsetree, parsetree->commandType, rte,
-								  rt_index, &securityQuals, &withCheckOptions,
+		get_row_security_policies(parsetree, rte, rt_index,
+								  &securityQuals, &withCheckOptions,
 								  &hasRowSecurity, &hasSubLinks);
 
 		if (securityQuals != NIL || withCheckOptions != NIL)
@@ -3026,6 +3026,7 @@ rewriteTargetView(Query *parsetree, Relation view)
 			wco = makeNode(WithCheckOption);
 			wco->kind = WCO_VIEW_CHECK;
 			wco->relname = pstrdup(RelationGetRelationName(view));
+			wco->polname = NULL;
 			wco->qual = NULL;
 			wco->cascaded = cascaded;
 

src/backend/utils/cache/relcache.c

@@ -859,8 +859,6 @@ equalPolicy(RowSecurityPolicy *policy1, RowSecurityPolicy *policy2)
 		if (policy2 == NULL)
 			return false;
 
-		if (policy1->policy_id != policy2->policy_id)
-			return false;
 		if (policy1->polcmd != policy2->polcmd)
 			return false;
 		if (policy1->hassublinks != policy2->hassublinks)

src/include/nodes/parsenodes.h

@@ -928,6 +928,7 @@ typedef struct WithCheckOption
 	NodeTag		type;
 	WCOKind		kind;			/* kind of WCO */
 	char	   *relname;		/* name of relation that specified the WCO */
+	char	   *polname;		/* name of RLS policy being checked */
 	Node	   *qual;			/* constraint qual to check */
 	bool		cascaded;		/* true for a cascaded WCO on a view */
 } WithCheckOption;

src/include/rewrite/rowsecurity.h

@@ -19,7 +19,6 @@
 
 typedef struct RowSecurityPolicy
 {
-	Oid			policy_id;		/* OID of the policy */
 	char	   *policy_name;	/* Name of the policy */
 	char		polcmd;			/* Type of command policy is for */
 	ArrayType  *roles;			/* Array of roles policy is for */
@@ -41,7 +40,7 @@ extern PGDLLIMPORT row_security_policy_hook_type row_security_policy_hook_permis
 
 extern PGDLLIMPORT row_security_policy_hook_type row_security_policy_hook_restrictive;
 
-extern void get_row_security_policies(Query *root, CmdType commandType,
+extern void get_row_security_policies(Query *root,
 						  RangeTblEntry *rte, int rt_index,
 						  List **securityQuals, List **withCheckOptions,
 						  bool *hasRowSecurity, bool *hasSubLinks);

src/test/modules/test_rls_hooks/expected/test_rls_hooks.out

@@ -83,7 +83,7 @@ SELECT * FROM rls_test_restrictive;
 INSERT INTO rls_test_restrictive VALUES ('r1','s1',10);
 -- failure
 INSERT INTO rls_test_restrictive VALUES ('r4','s4',10);
-ERROR:  new row violates row level security policy for "rls_test_restrictive"
+ERROR:  new row violates row level security policy "extension policy" for "rls_test_restrictive"
 SET ROLE s1;
 -- With only the hook's policies, both
 -- permissive hook's policy is current_user = username
@@ -124,7 +124,7 @@ EXPLAIN (costs off) SELECT * FROM rls_test_permissive;
                           QUERY PLAN                           
 ---------------------------------------------------------------
  Seq Scan on rls_test_permissive
-   Filter: (("current_user"() = username) OR ((data % 2) = 0))
+   Filter: (((data % 2) = 0) OR ("current_user"() = username))
 (2 rows)
 
 SELECT * FROM rls_test_permissive;
@@ -163,7 +163,7 @@ SELECT * FROM rls_test_restrictive;
 INSERT INTO rls_test_restrictive VALUES ('r1','s1',8);
 -- failure
 INSERT INTO rls_test_restrictive VALUES ('r3','s3',10);
-ERROR:  new row violates row level security policy for "rls_test_restrictive"
+ERROR:  new row violates row level security policy "extension policy" for "rls_test_restrictive"
 -- failure
 INSERT INTO rls_test_restrictive VALUES ('r1','s1',7);
 ERROR:  new row violates row level security policy for "rls_test_restrictive"
@@ -176,7 +176,7 @@ EXPLAIN (costs off) SELECT * FROM rls_test_both;
                                         QUERY PLAN                                         
 -------------------------------------------------------------------------------------------
  Subquery Scan on rls_test_both
-   Filter: (("current_user"() = rls_test_both.username) OR ((rls_test_both.data % 2) = 0))
+   Filter: (((rls_test_both.data % 2) = 0) OR ("current_user"() = rls_test_both.username))
    ->  Seq Scan on rls_test_both rls_test_both_1
          Filter: ("current_user"() = supervisor)
 (4 rows)
@@ -190,7 +190,7 @@ SELECT * FROM rls_test_both;
 INSERT INTO rls_test_both VALUES ('r1','s1',8);
 -- failure
 INSERT INTO rls_test_both VALUES ('r3','s3',10);
-ERROR:  new row violates row level security policy for "rls_test_both"
+ERROR:  new row violates row level security policy "extension policy" for "rls_test_both"
 -- failure
 INSERT INTO rls_test_both VALUES ('r1','s1',7);
 ERROR:  new row violates row level security policy for "rls_test_both"

src/test/modules/test_rls_hooks/test_rls_hooks.c

@@ -87,7 +87,6 @@ test_rls_hooks_permissive(CmdType cmdtype, Relation relation)
 	role = ObjectIdGetDatum(ACL_ID_PUBLIC);
 
 	policy->policy_name = pstrdup("extension policy");
-	policy->policy_id = InvalidOid;
 	policy->polcmd = '*';
 	policy->roles = construct_array(&role, 1, OIDOID, sizeof(Oid), true, 'i');
 
@@ -151,7 +150,6 @@ test_rls_hooks_restrictive(CmdType cmdtype, Relation relation)
 	role = ObjectIdGetDatum(ACL_ID_PUBLIC);
 
 	policy->policy_name = pstrdup("extension policy");
-	policy->policy_id = InvalidOid;
 	policy->polcmd = '*';
 	policy->roles = construct_array(&role, 1, OIDOID, sizeof(Oid), true, 'i');