Re: Proof of concept: auto updatable views

I've been looking at this further and I have made some improvements,
but also found a problem.

On 1 July 2012 23:35, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> I'm also aware that my new function ChangeVarAttnos() is almost
> identical to the function map_variable_attnos() that Tom recently
> added, but I couldn't immediately see a neat way to merge the two. My
> function handles whole-row references to the view by mapping them to a
> generic RowExpr based on the view definition. I don't think a
> ConvertRowtypeExpr can be used in this case, because the column names
> don't necessarily match.

I improved on this by reusing the existing function ResolveNew() which
reduced the size of the patch.


The problem, however, is that the original patch is not safe for
UPDATE/DELETE on security barrier views, because it mixes the user's
quals with those on the view. For example a query like

UPDATE some_view SET col=... WHERE user_quals

will get rewritten as

UPDATE base_table SET base_col=... WHERE user_quals AND view_quals

which potentially leaks data hidden by the view's quals.

So I think that it needs to use a subquery to isolate user_quals from
view_quals. The least invasive way I could see to do that was to
record the view quals in a new security barrier quals field on the
RTE, and then turn that into a subquery at the end of the rewriting
process. This approach avoids the extensive changes to the rewriter
that I think would otherwise be needed if the RTE were changed into a
subquery near the start of the rewriting process. It is also possible
that this code might be reusable in the row-level security patch by
Kohei KaiGai to handle modifications to tables with RLS quals,
although I haven't looked too closely at that patch yet.


Another complication is that the executor appears to have no
rechecking code for subqueries in nodeSubqueryscan.c, so it looks like
I need to lock rows coming from the base relation in the subquery. So
for example, given the following setup

CREATE TABLE private_t(a int, b text);
INSERT INTO private_t VALUES (1, 'Private');
INSERT INTO private_t
  SELECT i, 'Public '||i FROM generate_series(2,10000) g(i);
CREATE INDEX private_t_a_idx ON private_t(a);
ANALYSE private_t;

CREATE VIEW public_v WITH (security_barrier=true)
  AS SELECT b AS bb, a AS aa FROM private_t WHERE a != 1;

CREATE OR REPLACE FUNCTION snoop(b text)
RETURNS boolean AS
$$
BEGIN
  RAISE NOTICE 'b=%', b;
  RETURN false;
END;
$$
LANGUAGE plpgsql STRICT IMMUTABLE COST 0.000001;

an update on the view will get rewritten as an update on the base
table from a subquery scan as follows:

EXPLAIN (costs off) UPDATE public_v SET aa=aa WHERE snoop(bb) AND aa<=2;

 Update on private_t
   ->  Subquery Scan on private_t
         Filter: snoop(private_t.b)
         ->  LockRows
               ->  Index Scan using private_t_a_idx on private_t
                     Index Cond: (a <= 2)
                     Filter: (a <> 1)

The LockRows node appears to give the expected behaviour for
concurrently modified rows, but I'm not really sure if that's the
right approach.


None of this new code kicks in for non-security barrier views, so the
kinds of plans I posted upthread remain unchanged in that case. But
now a significant fraction of the patch is code added to handle
security barrier views. Of course we could simply say that such views
aren't updatable, but that seems like an annoying limitation if there
is a feasible way round it.

Thoughts?

Regards,
Dean