From: | Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> |
---|---|
To: | Stephen Frost <sfrost(at)snowman(dot)net> |
Cc: | Peter Geoghegan <pg(at)heroku(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, "Brightwell, Adam" <adam(dot)brightwell(at)crunchydatasolutions(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>, Craig Ringer <craig(at)2ndquadrant(dot)com>, Yeb Havinga <yeb(dot)havinga(at)portavita(dot)nl> |
Subject: | Re: RLS feature has been committed |
Date: | 2014-09-23 08:31:06 |
Message-ID: | 54212FCA.5040604@vmware.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | 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
Attachment | Content-Type | Size |
---|---|---|
rls-trivial-fixes.patch | text/x-diff | 6.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Oskari Saarenmaa | 2014-09-23 10:50:28 | Re: better atomics - v0.6 |
Previous Message | Magnus Hagander | 2014-09-23 08:13:48 | Re: Should we excise the remnants of borland cc support? |