Dean,
* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> It shouldn't be necessary to change get_policies_for_relation() at all
> to support the RETURNING check. You just need to call it with
> CMD_SELECT.
Yup, that works well.
> BTW, your change to change get_policies_for_relation() has
> a bug -- if the policy is for ALL commands it gets added
> unconditionally to the list of returning_policies, regardless of
> whether it applies to the current user role. That's the risk of
> complicating the function by trying to make it do more than it was
> designed to do.
Yeah, I had added the lappend's directly under the individual commands
at first, thinking back to when we were doing the per-role check there
instead of later and when I realized how you changed it to happen later
I went back and updated them, but apparently missed the 'all' case.
> So for example, build^H^H^Hadd_security_quals() takes 2 lists of
> policies (permissive and restrictive), combines them in the right way
> using OR and AND, does the necessary varno manipulation, and adds the
> resulting quals to the passed-in list of security quals (thereby
> implicitly AND'ing the new quals with any pre-existing ones), which is
> precisely what's needed to support RETURNING.
Right, it just needs to be called twice to have that happen correctly.
> I think this should actually be 2 separate commits, since the
> refactoring and the support for RETURNING are entirely different
> things. It just happens that after the refactoring, the RETURNING
> patch becomes trivial (4 new executable lines of code wrapped in a
> couple of if statements, to fetch and then apply the new policies in
> the necessary cases). At least that's the theory :-)
I had been planning to do them as independent commits in the end, but
thought it easier to review one patch, particularly when the differences
were larger. I've now reworked adding the RETURNING handling without
changing the other functions, per discussion.
Attached is a git format-patch built series which includes both commits,
now broken out, for review.
Thanks!
Stephen
