Heikki, * Heikki Linnakangas (hlinnakan...@vmware.com) wrote: > Some random comments after a quick read-through of the patch:
Glad you were able to find a bit of time to take a look, thanks!
> * 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.
Peter mentioned this too and I've been working on adding documentation.
The general capability, at least in my view, is pretty clear but I agree
that examples and adding more about the trade-offs would be useful. Do
you have any opinion regarding additional documentation for updatable
security-barrier views? I'm trying to work out a way to share this new
documentation with that since there is overlap as they share a lot of
the caveats, given that they use the same code underneath.
> * 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.
Right, WITH CHECK matches up with the SQL standard 'WITH CHECK OPTION'.
> 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.
Right, USING was there in the original patch back in January (actually,
it might be farther back, there were versions of the patch prior to
that) while WITH CHECK was added more recently.
> >+ 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.
It's certainly an item that I was planning to look at addressing, though
only for completeness. COPY's focus is providing a faster interface and
the RLS checks would end up dwarfing the overall time and making COPY
not actually be usefully faster than INSERT.
> * 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?
I don't believe it actually can happen, but I wanted to be sure that the
case was covered in the event that support is added. Essentially, I
carefully went through looking at all of the existing ExecCheckRTPerms()
calls and made sure RLS was handled in all of those cases, in one way or
another. As I expect you noticed, I also updated the comments in
ExecCheckRTPerms() to reflect that RLS needs to be addressed as well as
normal checks when returning tuples or adding new tuples.
> 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?
It's not copied, no. That didn't make a lot of sense to me as we don't
even have an option to copy permissions.
> * 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 question about views came up but with views you could simply add the
quals into the view definition instead of combining views and RLS.
Still, I'm not against it, but the patch certainly seemed large enough
to me that it made sense to move forward with the basic, but complete,
implementation for tables. Foreign tables fell into the same bucket but
perhaps it shouldn't have.. I'll definitely take a look at what the
level of complexity there is.
> * 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"
Ok, fair enough. It had been lazy originally which is where the name
originated from but during the design discussion it was pointed out that
it'd be better to support turning on/off RLS, but I hadn't considered
changing the name for that.
> * 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.
For a specific role you use the 'TO <role_list>' option to CREATE
POLICY.. Is that not clear? For extensions, there's documentation
where the hook is defined about how the hook is to be used and how
policies from the hook integrate with policies defined in the catalog.
We don't typically provide documentation beyond that for hooks, do we?
I specifically recall looking for that, to add to it, and not finding
any- indeed, my vague recollection from days gone by is that it was
decided that comments *are* the way to document hooks as any hook
author is almost certainly going to be looking at the code anyway.
> * 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
I didn't think it was generally expected that pg_restore would work
against older versions; indeed, I didn't think we actually queried for
the version as we can't do anything for SQL scripts created by
pg_restore.
Ok, I see we do have a check for PQserverVersion() in
pg_backup_archiver.c to use TRUNCATE TABLE .. ONLY. I'll take a look at
adding a check to skip the SET row_security for 9.4-and-earlier.
> * The new --enable-row-security option to pg_restore is not
> documented in the user manual.
Argh. Right, sorry about that, it was added to pg_dump, of course, but
the two are (obviously) not the same.
> * 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.
Yup, Coverity noted that and it's on my list of things to address. I do
wish we could get Coverity and buildfarm coverage ahead of commits, but
that's a discussion for a different thread..
> In addition to the above, attached is a patch with some trivial fixes.
Great, thanks!
Stephen
signature.asc
Description: Digital signature
