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,
+
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ 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
requested
+ * something invalid, and otherwise will return RLS_ENABLED if RLS
should
+ * 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
command"))));
+
+ /*
* 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");
else
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 @@
<row>
<entry><structfield>relhasrowsecurity</structfield></entry>
<entry><type>bool</type></entry>
+ <entry></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;
<para>
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).
</para>
<para>
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;
else
- 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)
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],
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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
