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

(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.

Thanks,

> - In rewriteTargetView, as now, reassign resultRelation to the target
> view's base rel. This is required so that do any RETURNING and WITH
> CHECK OPTION fixups required to adjust the RETURNING list to the new
> result relation, so they act on the final tuple after any BEFORE
> triggers act. Do not flatten the view subquery and merge the quals (as
> currently happens); allow it to be expanded as a subquery by the
> rewriter instead. Don't mess with the view tlist at this point except by
> removing the whole-row Var added by rewriteTargetListUD.
>
> - When doing tlist expansion in preprocess_targetlist, when we process
> the outer Query (the only one for which query type is not SELECT, and
> the only one that has a non-zero resultRelation), if resultRelation !=
> sourceRelation recursively follow the chain of sourceRelation s to the
> bottom one with type RTE_RELATION. Do tlist expansion on that inner-most
> Query first, using sourceRelation to supply the varno for injected TLEs,
> including injecting "ctid", "oid" if req'd, etc. During call stack
> unwind, have each intermediate layer do regular tlist expansion, adding
> a Var pointing to each tlist entry of the inner subquery.
>
> At the outer level of preprocess_targetlist, sort the tlist, now
> expanded to include all required vars, into attribute order for the
> resultRelation. (this level is the only one that has resultRelation set).
>
> Avoid invoking preprocess_targetlist on the inner Query again when it's
> processed in turn, or just bail out when we see sourceRelation set since
> we know it's already been done.
>
> (Alternately, it might be possible to run preprocess_targetlist
> depth-first instead of the current outermost-first, but I haven't looked
> at that).
>
>
> The optimizer can still flatten non-security-barrier updatable views,
> following the chain of Vars as it collapses each layer. That's
> effectively what the current rewriteTargetView code is doing manually at
> each pass right now.
>
> I'm sure there are some holes in this outline, but it's struck me as
> possibly workable. The key is to set sourceRelation on every inner
> subquery in the target query chain, not just the outer one, so it can be
> easily followed from the outer query though the subqueries into the
> innermost query with RTE_RELATION type.
>
>
>
> The only alternative I've looked at is looking clumsier the longer I
> examine it: adding a back-reference in each subquery's Query struct, to
> the Query containing it and the RTI of the subquery within the outer
> Query. That way, once rewriting hits the innermost rel with RTE_RELATION
> type, the rewriter can walk back up the Query tree doing tlist
> rewriting. I'm not sure if this is workable yet, and it creates ugly
> pointer-based backrefs *up* the Query chain, making what was previously
> a tree of Query* into a graph. That could get exciting, though there'd
> never be any need for mutators to follow the parent query pointer so it
> wouldn't make tree rewrites harder.
>
>
>
>
>
>

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