On 13 September 2015 at 20:59, Stephen Frost <sfrost@snowman.net> 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?
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.
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.
Regards,
Dean
