On 5 August 2017 at 10:03, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>
> Hello Rod,
>
> Patch applies cleanly, make html ok, new table looks good to me.
>
> I've turned it "Ready for Committer".
>
I didn't really finish committing this in the last commitfest, before
getting distracted by a security issue, so returning to it now...
I like Rod's original idea of a summary table here, to save reading
through a lot of text, and I think it's handy to be able to see at a
glance which polices apply to which commands, and vice versa. However,
I'm not entirely convinced by the contents of the table, as proposed.
The meaning of the contents of the table cells isn't entirely clear.
For example, the SELECT/FOR ALL USING cell contains "WHERE clause",
which I presume means the policy expressions are added to the query's
WHERE clause. But the INSERT/FOR ALL USING cell contains "RETURNING
clause", meaning the policy is used if there is a RETURNING clause.
Then the INSERT/FOR ALL WITH CHECK cell contains "new tuple". So the
table cells seem to be providing answers to different orthogonal
questions.
I think the contents of each cell should provide the answer to a
single question, or a consistent set of questions. IMO the principal
question is "does this policy apply to this command?", although that
can be expanded to include "... and if so, which tuple is tested?" so
then the principal content of each cell would be "Existing row" or
"New row", or blank if the policy doesn't apply.
There's a minor complication that some cases like SELECT policies for
UPDATE commands don't always apply. I think that can be covered by a
suitable footnote.
The set of table columns (commands) currently only includes SELECT,
INSERT, UPDATE and DELETE. I think that should be expanded to include
SELECT FOR UPDATE/SHARE and ON CONFLICT DO UPDATE. I think it also
makes sense to include INSERT ... RETURNING as a separate command,
since it almost always requires SELECT permissions, whereas a bare
INSERT never does. I don't think it's worth including the RETURNING
variants of the other commands, since they typically require SELECT
permissions even without a RETURNING clause.
The set of table rows (policy types) currently includes FOR ALL as a
separate policy type. Whilst that might be technically correct, I
think it's more useful to think of the set of policy types as
SELECT/ALL, INSERT/ALL, UPDATE/ALL and DELETE/ALL because ALL policies
effectively just behave like one or more of the other policy types
depending on the context. Doing it that way then ties in better with
the way multiple policies are combined. For example, for an UPDATE
command, there are 2 sets of policies that get applied (combined using
AND) -- the SELECT/ALL policies and the UPDATE/ALL policies. It's not
so useful to regards ALL as a separate type, and it would be wrong to
AND together the ALL policies with the SELECT policies and the UPDATE
policies.
With the above changes, there are more command types than policy
types, so it also makes sense to swap the orientation of the table.
Here's an updated patch, with the table done that way. Comments?
Regards,
Dean
