Dean, * Dean Rasheed (dean.a.rash...@gmail.com) wrote: > On 14 September 2015 at 14:47, Stephen Frost <sfr...@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
signature.asc
Description: Digital signature
