Re: RLS open items are vague and unactionable

Dean,

On Sunday, September 13, 2015, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>
wrote:

> On 13 September 2015 at 20:59, Stephen Frost <sfrost(at)snowman(dot)net
> <javascript:;>> wrote:
> > I've been through this and definitely like it more than what we had
> > previously, as I had mentioned previously. I've also added the
> > RETURNING handling, per the discussion between you, Robert, Tom, and
> > Kevin.
> >
> > Would be great to get your feedback both on the relatively minor changes
> > which I made to your refactoring patch (mostly cosmetic) and how I added
> > in the handling for the RETURNING case, which was made much simpler and
> > cleaner by the refactoring. (Working through adding the RETURNING also
> > helped my understanding of the refactoring, which I feel comfortable
> > that I understand well now.)
> >
>
> Cool. I haven't looked in detail yet, but I expected the changes
> necessary to support RETURNING to be much simpler than that. Isn't it
> just a case of adding something like
>
> if (root->returningList &&
> commandType == CMD_UPDATE or CMD_DELETE)
> {
> get_policies_for_relation(rel, CMD_SELECT, ...)
> build_security_quals(...)
> }
>
> and then something similar with build_with_check_options() for the upsert
> case?

Not in front of my laptop and will review it when I get back in more
detail, but the original approach that I tried was changing
get_policies_for_relation to try and build everything necessary, which
didn't work as we need to OR the various ALL/SELECT policies together and
then AND the result to apply the filtering.

Seems like that might not be an issue with your proposed approach, but
wouldn't we need to either pass in fresh lists and then append them to the
existing lists, or change the functions to work with non-empty lists? (I
had thought about changing that anyway since there's often cases where it's
useful to be able to call a function which adds to an existing list).
Further, actually, we'd still have to figure out how to build up the OR'd
qual from the ALL/SELECT policies and then add that to the restrictive set.
That didn't appear easy to do from get_policies_for_relation as all the
ChangeVarNode work is handled in the build_* functions, which have the info
necessary.

> Then it isn't necessary to modify get_policies_for_relation(),
> build_security_quals() and build_with_check_options() to know anything
> specific about returning. They're just another set of permissive and
> restrictive policies to be fetched and added to the command.

The ALL/SELECT policies need to be combined with OR's (just like all
permissive sets of policies) and then added to the restrictive set of
quals, to ensure that they are evaluated as a restriction and not just
another set of permissive policies, which wouldn't provide the required
filtering.

> One change I thought about making was renaming build_security_quals()
> and build_with_check_options() to add_security_quals() and
> add_with_check_options(), because they're adding to lists rather than
> returning lists, and in general they are called multiple times to
> build up the complete lists.

That sounds reasonable to me.

Thanks!

Stephen