Some random comments after a quick read-through of the patch:

* Missing documentation. For a major feature like this, reference pages for the CREATE/DROP POLICY statements are not sufficient. We'll need a whole new chapter for this.

* In CREATE POLICY, the "USING" and "WITH CHECK" keywords are not very descriptive. I kind of understand WITH CHECK - it's applied to new rows like a CHECK constraint - but USING seems rather arbitrary and WITH CHECK isn't all that clear either. Perhaps "ON SELECT CHECK" and "ON UPDATE CHECK" or something like that would be better. I guess USING makes sense when that's the only expression given, so that it applies to both SELECTs and UPDATEs. But when a WITH CHECK expression is given together with USING, it gets confusing.

+                       if (is_from)
+                               ereport(ERROR,
+                                                errmsg("COPY FROM not supported 
with row security."),
+                                                errhint("Use direct INSERT 
statements instead.")));

Why is that not implemented? I think it should be.

* In src/backend/commands/createas.c:

@@ -420,6 +421,19 @@ intorel_startup(DestReceiver *self, int operation, 
TupleDesc typeinfo)
        ExecCheckRTPerms(list_make1(rte), true);

+        * Make sure the constructed table does not have RLS enabled.
+        *
+        * check_enable_rls() will ereport(ERROR) itself if the user has 
+        * something invalid, and otherwise will return RLS_ENABLED if RLS 
+        * be enabled here.  We don't actually support that currently, so throw
+        * our own ereport(ERROR) if that happens.
+        */
+       if (check_enable_rls(intoRelationId, InvalidOid) == RLS_ENABLED)
+               ereport(ERROR,
+                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                                (errmsg("policies not yet implemented for this 
+       /*
         * Tentatively mark the target as populated, if it's a matview and we're
         * going to fill it; otherwise, no change needed.

Hmm. So, if a table we just created with CREATE TABLE AS has row-level security enabled, we throw an error? Can that actually happen? AFAICS a freshly-created table shouldn't can't have row-level security enabled. Or is row-level security enabled/disabled status copied from the source table?

* Row-level security is not allowed for foreign tables. Why not? It's perhaps not a very useful feature, but I don't see any immediate reason to forbid it either. How about views?

* The new pg_class.relhasrowsecurity column is not updated lazily like most other relhas* columns. That's intentional and documented, but perhaps it would've been better to name the column differently, to avoid confusion. Maybe just "relrowsecurity"

* In rewrite/rowsecurity:

+ * Policies can be defined for specific roles, specific commands, or provided
+ * by an extension.

How do you define a policy for a specific role? There's a hook for the extensions in there, but I didn't see any documentation mentioning that an extension might provide policies, nor any developer documentation on how to use the hook.

* In pg_backup_archiver.c:

         * Enable row-security if necessary.
        if (!ropt->enable_row_security)
                ahprintf(AH, "SET row_security = off;\n");
                ahprintf(AH, "SET row_security = on;\n");

That makes pg_restore to throw an error if you try to restore to a pre-9.5 server. I'm not sure if using a newer pg_restore against an older server is supported in general, but without the above it works in simple cases, at least. It would be trivi

* The new --enable-row-security option to pg_restore is not documented in the user manual.

* in src/bin/psql/describe.c:

@@ -2529,6 +2640,11 @@ describeRoles(const char *pattern, bool verbose)
                        appendPQExpBufferStr(&buf, "\n, r.rolreplication");

+               if (pset.sversion >= 90500)
+               {
+                       appendPQExpBufferStr(&buf, "\n, r.rolbypassrls");
+               }
                appendPQExpBufferStr(&buf, "\nFROM pg_catalog.pg_roles r\n");

The rolbypassrls column isn't actually used for anything in this function.

In addition to the above, attached is a patch with some trivial fixes.

- Heikki

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 76d6405..bae08ee 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1943,6 +1943,7 @@
+      <entry></entry>
        True if table has row-security enabled; see
        <link linkend="catalog-pg-rowsecurity"><structname>pg_rowsecurity</structname></link> catalog
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 70e47aa..9494439 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -5457,9 +5457,9 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
         The allowed values of <varname>row_security</> are
-        <literal>on</> (apply normally- not to superuser or table owner),
+        <literal>on</> (apply normally - not to superuser or table owner),
         <literal>off</> (fail if row security would be applied), and
-        <literal>force</> (apply always- even to superuser and table owner).
+        <literal>force</> (apply always - even to superuser and table owner).
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 1b35756..b5ef09e 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -429,7 +429,7 @@ ALTER TABLE ALL IN TABLESPACE <replaceable class="PARAMETER">name</replaceable>
       These forms control the application of row security policies belonging
       to the table.  If enabled and no policies exist for the table, then a
       default-deny policy is applied.  Note that policies can exist for a table
-      even if row level security is disabled- in this case, the policies will
+      even if row level security is disabled - in this case, the policies will
       NOT be applied and the policies will be ignored.
       See also
       <xref linkend="SQL-CREATEPOLICY">.
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index 2f4df48..fc430b3 100644
--- a/src/backend/commands/policy.c
+++ b/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;
-		elog(ERROR, "Unregonized command.");
-		/* error unrecognized command */
+		elog(ERROR, "unregonized command");
 	return cmd;
@@ -484,7 +483,7 @@ CreatePolicy(CreatePolicyStmt *stmt)
 	if (rseccmd == ACL_INSERT_CHR && stmt->qual != NULL)
-				 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))
-				 errmsg("policy '%s' for does not exist on table %s",
+				 errmsg("policy \"%s\" on table \"%s\" does not exist",
@@ -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. */
diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c
index e1ccd12..b595cfa 100644
--- a/src/backend/rewrite/rowsecurity.c
+++ b/src/backend/rewrite/rowsecurity.c
@@ -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).
@@ -285,7 +285,6 @@ prepend_row_security_policies(Query* root, RangeTblEntry* rte, int rt_index)
  * Returns the list of policies to be added for this relation, based on the
  * type of command and the roles to which it applies, from the relation cache.
- *
 static List *
 pull_row_security_policies(CmdType cmd, Relation relation, Oid user_id)
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 1c1b80f..21715dc 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -463,7 +463,7 @@ usage(const char *progname)
 	printf(_("  -x, --no-privileges          skip restoration of access privileges (grant/revoke)\n"));
 	printf(_("  -1, --single-transaction     restore as a single transaction\n"));
 	printf(_("  --disable-triggers           disable triggers during data-only restore\n"));
-	printf(_("  --enable-row-security         enable row level security\n"));
+	printf(_("  --enable-row-security        enable row level security\n"));
 	printf(_("  --if-exists                  use IF EXISTS when dropping objects\n"));
 	printf(_("  --no-data-for-failed-tables  do not restore data of tables that could not be\n"
 			 "                               created\n"));
diff --git a/src/include/commands/policy.h b/src/include/commands/policy.h
index 95d8a6d..8ca4cb0 100644
--- a/src/include/commands/policy.h
+++ b/src/include/commands/policy.h
@@ -24,10 +24,10 @@ extern void RemovePolicyById(Oid policy_id);
 extern Oid CreatePolicy(CreatePolicyStmt *stmt);
 extern Oid AlterPolicy(AlterPolicyStmt *stmt);
-Oid get_relation_policy_oid(Oid relid,
-							const char *policy_name, bool missing_ok);
+extern Oid get_relation_policy_oid(Oid relid, const char *policy_name,
+						bool missing_ok);
-Oid rename_policy(RenameStmt *stmt);
+extern Oid rename_policy(RenameStmt *stmt);
 #endif   /* POLICY_H */
Sent via pgsql-hackers mailing list (
To make changes to your subscription:

Reply via email to