Dean,
* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 14 September 2015 at 14:47, Stephen Frost <sfrost@snowman.net> wrote:
> > Attached is a git format-patch built series which includes both commits,
> > now broken out, for review.
>
> That looks OK to me.
Excellent.
> A minor point -- this comment isn't quite right:
>
> /*
> * For the target relation, when there is a returning list, we need to
> * collect up CMD_SELECT policies to add via add_security_quals and
> * add_with_check_options. This is because, for the RETURNING case, we
> * have to filter any records which are not visible through an ALL or SELECT
> * USING policy.
> *
> * We don't need to worry about the non-target relation case because we are
> * checking the ALL and SELECT policies for those relations anyway (see
> * above).
> */
>
> because the policies that are fetched there are only used for
> add_security_quals(), not for add_with_check_options(). It might be
> cleaner if the 'if' statement that follows were merged with the
> identical one a few lines down, and then those returning policies
> could be local to that block, with the 2 pieces of RETURNING handling
> done together. Similarly for the upsert block.
Hmm, ok, will take a look at doing that.
> Actually, it isn't necessary to test that rt_index ==
> root->resultRelation, because for all other relations commandType is
> set to CMD_SELECT higher up, so the 'returning' bool variable could
> just be replaced with 'root->returningList != NIL' throughout.
I had thought something similar originally and ran into a case where
that didn't quite work. That was a few revisions ago though, so perhaps
there was something else going on. I'll take a look at making this
change also (which was actually how I had implemented it initially).
I'll be offline for a few hours as I'm about to fly to Dallas, but I'll
get to this tomorrow morning, at the latest.
Thanks!
Stephen
