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

(2014/01/21 18:18), Dean Rasheed wrote:
> 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.
>
Probably, I could get your point.

Even though the sub-query being replaced from a relation with
securityQuals is eventually planned according to the usual
manner, usual order as a part of regular sub-query planning,
however, adjust_appendrel_attrs() is called by inheritance_planner()
earlier than sub-query's planning.

Maybe, we originally had this problem but not appeared on the
surface, because child relations don't have qualifiers on this
phase. (It shall be distributed later.)
But now, this assumption was broken because of a relation with
securityQuals being replaced by a sub-query with SubLink.
So, I'm inclined to revise the assumption side, rather than
existing assertion checks.

Shall we investigate what assumption should be revised if child-
relation, being expanded at expand_inherited_tables(), would be
a sub-query having Sub-Link?

A minor comment is below:

! /*
! * Make sure that the query is marked correctly if the added qual
! * has sublinks.
! */
! if (!parsetree->hasSubLinks)
! parsetree->hasSubLinks = checkExprHasSubLink(viewqual);

Is this logic really needed? This flag says the top-level query
contains SubLinks, however, the above condition checks whether
a sub-query to be constructed shall contain SubLinks.
Also, securityQuals is not attached to the parse tree right now.

Thanks,
--
OSS Promotion Center / The PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>