Re: RLS feature has been committed - Mailing list pgsql-hackers

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