Re: WIP patch (v2) for updatable security barrier views

On 21 January 2014 01:09, KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
> (2014/01/13 22:53), Craig Ringer wrote:
>>
>> On 01/09/2014 11:19 PM, Tom Lane wrote:
>>>
>>> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
>>>>
>>>> My first thought was that it should just preprocess any security
>>>> barrier quals in subquery_planner() in the same way as other quals are
>>>> preprocessed. But thinking about it further, those quals are destined
>>>> to become the quals of subqueries in the range table, so we don't
>>>> actually want to preprocess them at that stage --- that will happen
>>>> later when the new subquery is planned by recursion back into
>>>> subquery_planner(). So I think the right answer is to make
>>>> adjust_appendrel_attrs() handle recursion into sublink subqueries.
>>>
>>>
>>> TBH, this sounds like doubling down on a wrong design choice. I see
>>> no good reason that updatable security views should require any
>>> fundamental rearrangements of the order of operations in the planner.
>>
>>
>> In that case, would you mind offerign a quick sanity check on the
>> following alternative idea:
>>
>> - Add "sourceRelation" to Query. This refers to the RTE that supplies
>> tuple projections to feed into ExecModifyTable, with appropriate resjunk
>> "ctid" and (if requ'd) "oid" cols present.
>>
>> - When expanding a target view into a subquery, set "sourceRelation" on
>> the outer view to the index of the RTE of the newly expanded subquery.
>>
> I have sane opinion. Existing assumption, "resultRelation" is RTE of the
> table to be read not only modified, makes the implementation complicated.
> If we would have a separate "sourceRelation", it allows to have flexible
> form including sub-query with security_barrier on the reader side.
>
> Could you tell me the direction you're inclined right now?
> I wonder whether I should take the latest patch submitted on 09-Jan for
> a deep code reviewing and testing.
>

Yes, please review the patch from 09-Jan
(http://www.postgresql.org/message-id/CAEZATCUiKxOg=vOOvjA2S6G-sixzzxg18ToTggP8zOBq6QnQHQ@mail.gmail.com).

The idea behind that patch is to avoid a lot of the complication that
leads to and then arises from having a separate "sourceRelation" in
the query.

If you go down the route of expanding the subquery in the rewriter and
making it the "sourceRelation", then you have to make extensive
changes to preprocess_targetlist so that it recursively descends into
the subquery to pull out variables needed by an update.

Also you would probably need additional changes in the rewriter so
that later stages didn't trample on the "sourceRelation", and
correctly updated it in the case of views on top of other views.

Also, you would need to make changes to the inheritance planner code,
because you'd now have 2 RTEs referring to the target relation
("resultRelation" and "sourceRelation" wrapping it in a subquery).
Referring to the comment in inheritance_planner():

* Source inheritance is expanded at the bottom of the
* plan tree (see allpaths.c), but target inheritance has to be expanded at
* the top.

except that in the case of the "sourceRelation", it is actually
effectively the target, which means it shouldn't be expanded,
otherwise you'd get plans like the ones I complained about upthread
(see the final explain output in
http://www.postgresql.org/message-id/CAEZATCU3VcDKJpnHGFkRMrkz0axKCUH4CE_kQq3Z2HzkNhi5iA@mail.gmail.com).

Perhaps all of that could be made to work, but I think that it would
end up being a far more invasive patch than my 09-Jan patch. My patch
avoids both those issues by not doing the subquery expansion until
after inheritance expansion, and after targetlist preprocessing.

Regards,
Dean