Re: WIP patch (v2) for updatable security barrier views - Mailing list pgsql-hackers

> I've been thinking about this some more and I don't think you can
> avoid the need to remap vars and attrs.

I agree. I was hoping to let expand_targetlist / preprocess_targetlist
do the job, but I'm no longer convinced that'll do the job without
mangling them in the process.

> AIUI, your modified version of rewriteTargetView() will turn an UPDATE
> query with a rangetable of the form

The patch I posted is clearly incorrect in several ways.  Unfortunately
I'm still coming up to speed with the rewriter / planner / executor at
the same time as trying to make major modifications to them. This tends
to produce some false starts.

(It no longer attempts to use expand_targetlist in the rewriter, btw,
that's long gone).

Since the posted patch I've sorted out RETURNING list rewriting and a
few other issues. There are some major issues remaining with the
approach though:

- If we have nested views, we need to pass the tuple ctid up the chain of expanded view subqueries so ExecModifyTable
canconsume it. This is proving to be a lot harder than I'd hoped.
 

- expand_targetlist / preprocess_targetlist operate on the resultRelation. With updatable s.b. views, the
resultRelationis no longer the relation from which tuples are being read for input into ExecModifyTable.
 

- In subqueries, resultRelation is only set for the outermost layer. So preprocess_targetlist doesn't usefully add
tlistentries for the inner subquery layers at all.
 

- It is necessary to expand DEFAULTs into expression tlist entries in the innermost query layer so that Vars added
furtherup in the subquery chain can refer to them. In the current experimental patch DEFAULTs aren't populated
correctly.

So we have the following needs:

- passing ctids up through layers of subqueries

- target-list expansion through layers of subqueries

- Differentiating between resultRelation to heapmodifytuple and the
source of the tuples to feed into heapmodifytuple now that these are no
longer the same thing.


I was originally hoping all this could be dealt with in the rewriter, by
changing resultRelation to point to the next-inner-most RTE whenever a
target view is expanded. This turns out to be too simplistic:

- There is no ctid col on a view, so if we have v2 over v1 over table t,
when we expand v2 there's no way to create a tlist entry to point to
v1's ctid. It won't have one until we've expanded v1 into t in the next
pass. The same issue applies to expanding the tlist to carry cols of "t"
up the subquery chain in the rewrite phase.

- Rowmarks are added to point to resultrelation after rewrite, and these
then add tlist entries in preprocess_targetlist. These TLEs will point
to the base resultRelation, which isn't correct when we're updating
nested subqueries.

So we just can't do this during recursive rewrite, because the required
information is only available once the target view is fully expanded
into nested subqueries.

It seems that tlist fixup and injection of the ctid up the subquery
chain must be done after all recursive rewriting.

To do these fixups later, we need to keep track of which nested series
of subqueries is the "target", i.e. produces tuples including resjunk
ctid cols to feed into ExecModifyTuple. Currently this information is
"resultRelation"



> The more I think about this, the more I think that the approach of my
> original patch was neater. The idea was to have 2 new pieces of code:
> 
> 1). In rewriteTargetView() decorate the target RTE with any security
>     barrier quals (in the new rte->securityQuals field), instead of
>     merging them with the main query's quals. So the output of this
>     stage of rewriting would be something like
> 
>   rtable:
>     1: relation RTE (base table) <- resultRelation
>         - securityQuals = [view quals]
>   fromList: [1]

So you're proposing to still flatten views during rewrite, as the
current code does, but just keep track of s.b. quals separately?

If so, what about multiple S.B. views may be nested and their quals must
be isolated from each other, not just from the outer query?

That's why I see subqueries as such a useful model.

> 2). After all rewriting is complete, scan the query and turn all
>     relation RTEs with non-empty securityQuals into subquery RTEs
>     (making a copy of the original RTE in the case of the result
>     relation).

I'm not sure I understand how this would work in the face of multiple
levels of nesting s.b. views. Are you thinking of doing recursive expansion?

> Another ugly
> feature of my earlier patch was the changes it made to
> expand_target_list() and the need to track the query's sourceRelation.

I've been fighting the need to add the concept of a "sourceRelation" for
this purpose too.

> Both of those things can be avoided simply by moving the subquery
> expansion code (2) to after expand_target_list(), and hence also after
> expand_inherited_tables().

That's certainly interesting. I'm reading over the patch now.

> There is still a lot more testing to be done with my patch, so there
> may well be unforeseen problems, but it feels like a cleaner, more
> straightforward approach.
> 
> Thoughts?

I'm reading it now, but in general, how do you see it solving the issue
of getting the ctid (and any table attrs not present in the views) up
two or more layers of nested view? And default expansion?

-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services