Re: RLS feature has been committed

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