Automatically updatable security barrier views

From PostgreSQL wiki

Revision as of 04:13, 30 December 2013 by Ringerc (Talk | contribs)

Jump to: navigation, search

Contents

Automatically updatable security barrier views

As part of the Row-security work, and as a useful feature on its own, it is desirable to support automatically updatable security_barrier views. Ordinary views are updatable in 9.3, but security_barrier views aren't considered "simple" views and are not updatable.

Status in 9.3

"simple" views are made updatable in 9.3 by flattening the view quals into the outer query. So given:

 CREATE TABLE t AS 
 SELECT n AS id, 'secret'||n AS secret FROM generate_series(1,10) n;
 CREATE VIEW t_even AS SELECT * FROM t WHERE id % 2 = 0;
 CREATE VIEW t_even_sb WITH (security_barrier) AS SELECT * FROM t WHERE id % 2 = 0;

you can update the simple non-security-barrier view, but can also steal values:

 CREATE OR REPLACE FUNCTION f_leak(text) RETURNS boolean AS $$
 BEGIN
   RAISE NOTICE 'Saw secret=%',$1;
   RETURN true;
 END;
 $$ LANGUAGE plpgsql COST 1;
 test=>   UPDATE t_even SET id = id WHERE f_leak(secret);
 NOTICE:  Saw secret=secret1
 NOTICE:  Saw secret=secret3
 NOTICE:  Saw secret=secret5
 NOTICE:  Saw secret=secret7
 NOTICE:  Saw secret=secret9
 NOTICE:  Saw secret=secret2
 NOTICE:  Saw secret=secret4
 NOTICE:  Saw secret=secret6
 NOTICE:  Saw secret=secret8
 NOTICE:  Saw secret=secret10
 UPDATE 5

You can't update the security barrier view at all:

 test=> UPDATE t_even_sb SET id = id WHERE f_leak(secret);
 ERROR:  cannot update view "t_even_sb"
 DETAIL:  Security-barrier views are not automatically updatable.
 HINT:  To enable updating the view, provide an INSTEAD OF UPDATE trigger or an unconditional ON UPDATE DO INSTEAD rule.

so there's no secure way to give a user update on only some rows of table. That's something we need for row-security and it's a feature that would be useful for users in general.

Issue with security barrier support

The existing updatable view code relies on flattening a view's quals into the outer query. So with the above example:

 UPDATE t_even SET id = id WHERE f_leak(secret);

gets view-expanded into something logically like:

 UPDATE (SELECT * FROM t WHERE id % 2 = 0) SET id = id WHERE f_leak(secret)

but the executor doesn't know how to deal with that so it can't be passed through as-is. Instead the subquery gets flattened out, producing:

 UPDATE t SET id = id WHERE id % 2 = 0 AND f_leak(secret)

which the executor can deal with, since it's a simple update of a relation. You can see this in the plan:

 test=# explain  UPDATE t_even SET id = id WHERE f_leak(secret);
                        QUERY PLAN                        
 ---------------------------------------------------------
  Update on t  (cost=0.00..31.53 rows=2 width=42)
    ->  Seq Scan on t  (cost=0.00..31.53 rows=2 width=42)
          Filter: (f_leak(secret) AND ((id % 2) = 0))
 (3 rows)

See how the f_leak(secret) qual and id % 2 = 0 are at the same level? The executor will tend to pick f_leak(secret) to run first because its COST is artificially low.

This won't work with security-barrier views, since we have to make sure that the predicate id % 2 = 0 filters rows before any are passed to the user-supplied predicate, in this case f_leak(secret). So the current code refuses to operate on security_barrier views.

How automatically updatable views work in 9.3

Automatically updatable views were introduced by Dean Rasheed's code, committed by Tom in a99c42f291421572aef2b0a9360294c7d89b8bc7.

This code extends src/backend/rewrite/rewriteHandler.c, adding functions to:

  • get_view_query: Get the _RETURN rule from a view
  • view_has_instead_trigger: Check whether there's an INSTEAD trigger that should supersede auto-upd view
  • view_is_auto_updatable: Check whether a view is "simple" enough to be auto-upd. Rejects sb views.
  • relation_is_updatable: Used by information_schema. Only interesting for real views.
  • adjust_view_column_set: Map column permissions from view to sub-table. Only interesting if it is a real view.
  • rewriteTargetView: The guts. Rewrite a view into the outer query. It:
    • Makes sure the view is auto-updatable with view_is_auto_updatable
    • Finds the RTE for the view in the outer query
    • Gets the query that defines the view from its _RETURN rule using get_view_query
    • Locks the base relation (might be another view)update resnos in the targetlist to refer to columns of the base relation
    • Pull up the view (should be merged with optimizer pull-up code if possible):
    • Create a new target RTE describing the base rel and add it to the outer query's range table
      • Copy the quals to the outer query's qual list, fixing up varnos to point to the new target
      • Deal with permissions where view owner != query caller
      • Deal with column permissions
    • Fix up vars in outer rel to point to vars in new base rel instead of view, using ReplaceVarsFromTargetList (TODO: can we re-use this approach for RLS?)
    • Fix up everything else that references the view to point to the new base rel using ChangeVarNodes
    • Update resnos from target list to point to cols of base rel (UPDATE / INSERT) only.
    • Add the rewritten update/insert/delete, either to the beginning of the list of rewritten queries + rules (for insert) or after (for update/delete).

Also changes fireRIRrules

Notably, this means that the updatable view code doesn't actually add support for updating a view. Instead, it adds support for rewriting simple views to pull their quals up into the outer query and flatten the subquery.

We can't do that for updatable security barrier views.

The path to updatable s.b. views

To support updatable security barrier views we have to support UPDATE directly on a subquery without flattening the query. That's because we must still enforce the order of qual execution imposed by the security_barrier flag on the subquery. Anything else would require implementing a different approach to security barriers and introduce its own problems.

How does updating a join work?

UPDATE ... FROM already supports acting on a join, as does DELETE ... USING.

We need to work out how this is executed, and determine how the update/delete is executed using the join node. Multiple join types are supported:

regress=> explain delete from t using t t2 where t.id = t2.id;
                                QUERY PLAN                                 
---------------------------------------------------------------------------
 Delete on t  (cost=170.85..290.46 rows=7564 width=12)
   ->  Merge Join  (cost=170.85..290.46 rows=7564 width=12)
         Merge Cond: (t.id = t2.id)
         ->  Sort  (cost=85.43..88.50 rows=1230 width=10)
               Sort Key: t.id
               ->  Seq Scan on t  (cost=0.00..22.30 rows=1230 width=10)
         ->  Sort  (cost=85.43..88.50 rows=1230 width=10)
               Sort Key: t2.id
               ->  Seq Scan on t t2  (cost=0.00..22.30 rows=1230 width=10)
(9 rows)
regress=> explain delete from t using t t2 where t.id = t2.id;
                               QUERY PLAN                                
-------------------------------------------------------------------------
 Delete on t  (cost=3.25..6.62 rows=100 width=12)
   ->  Hash Join  (cost=3.25..6.62 rows=100 width=12)
         Hash Cond: (t.id = t2.id)
         ->  Seq Scan on t  (cost=0.00..2.00 rows=100 width=10)
         ->  Hash  (cost=2.00..2.00 rows=100 width=10)
               ->  Seq Scan on t t2  (cost=0.00..2.00 rows=100 width=10)
(6 rows)
regress=> SET enable_hashjoin = off;
SET
regress=> SET enable_mergejoin = off;
SET
regress=> explain delete from t using t t2 where t.id = t2.id;
                               QUERY PLAN                                
-------------------------------------------------------------------------
 Delete on t  (cost=0.00..154.25 rows=100 width=12)
   ->  Nested Loop  (cost=0.00..154.25 rows=100 width=12)
         Join Filter: (t.id = t2.id)
         ->  Seq Scan on t  (cost=0.00..2.00 rows=100 width=10)
         ->  Materialize  (cost=0.00..2.50 rows=100 width=10)
               ->  Seq Scan on t t2  (cost=0.00..2.00 rows=100 width=10)
(6 rows)

so it's clearly at least somewhat generic.

How does this work at the code level, though?

CREATE TABLE t2 AS SELECT * FROM t;
SET debug_print_plan = on;
SET debug_print_parse = on;
SET debug_print_rewritten = on;
SET client_min_messages = debug;
DELETE FROM t USING t2 WHERE t.id = t2.id;

produces [1] - parse, rewrite and plan tree set.

The parse tree shows two range-table entries, one for t and one for t2. They have different :requiredPerms but are otherwise the same. The parse-tree has :commandType 4 (CMD_DELETE)

By contrast, in a simple delete doesn't differ except for :requiredPerms 8.


The diff between the two parse, plan and rewrite trees may be informative.

Parse tree

  • :requiredPerms on the first RTE, for t, is 8 for the simple delete, 10 for the join delete. The extra bit, 0x02, is defined in include/nodes/parsenodes.h as #define ACL_SELECT (1<<1).
  • :selectedCols on the first RTE has an extra entry 9 in the join delete. This is a resjunk column used to hold the join key?
  • :modifiedCols (b) is on the first RTE for both parse trees
  • There's a second RTE added by the join plan, for t aliased to t2. Other than the alias it is the same as the first RTE, for the base table.

... and that's it for the parse tree. It looks like it really is a simple delete on a join. The only indication that the target table is the first RTE appears to be that it's RTE index 1.

Rewritten tree

diff starts here.

The rewritten tree is the same as the parse tree in both cases; no change.

Plan tree

diff starts here.

Here we should see how the planner intends on actually executing this delete.

The costs and estimates are different, but we can ignore that, it's not interesting.

At the top level QUERY node the key entry (thanks RhodiumToad) appears:

   :resultRelation 1 

where 1 is a RTI.

  • In both cases the top node is MODIFYTABLE with identical parameters, the interesting ones being:
      :extParam (b)
      :allParam (b)
      :operation 4 
      :canSetTag true 
      :resultRelations (i 1)
      :resultRelIndex 0

IOW it refers to the result relation(s). It's the same for both the simple and join deletes. To focus on are :resultRelations and resultRelIndex. TODO

There is a HASHJOIN node instead of a SEQSCAN node at the first sublevel of the plan.

The HASHJOIN node has two TARGETENTRY nodes, not just the one for a SEQSCAN. The only differences are :varnoold 2 instead of 1 for the expr of the 2nd node, resno 2 instead of 1, resname is ctid1 instead of ctid. Both are resjunk columns. Under the HASHJOIN are left and right plan trees, each containing a seqscan node. The nested hash op and scans are included under it.

Finally, the join plan adds:

   :rowMarks (
      {PLANROWMARK 
      :rti 2 
      :prti 2 
      :rowmarkId 1 
      :markType 4 
      :noWait false 
      :isParent false
      }
    :relationOids (o 16387 16402)
   )

Here :relationOids are for t and t2 respectively.

The :rowMarks clause appears to refer to a FOR UPDATE clause. markType is defined in include/nodes/plannodes.h as RowMarkType. markType 4 is ROW_MARK_REFERENCE /* just fetch the TID */. The comment there says that a rowMark is added for each non-target relation and that if it isn't FOR UPDATE it's flagged ROW_MARK_REFERENCE. So this is the ref to t2, which fits given rti being the index of range table entry 2, t2.

The rowmarkId is for resjunk cols, referring to the unique resjunk col id :resno? If so, it seems to refer to the left side of the join.

In the code

  • src/backend/executor/README.
  • backend/parser/README
  • Target relation of update is Query->resultRelation
  • UPDATE ... FROM on a simple table target is acted on in preprocess_targetlist and expand_targetlist in backend/optimizer/prep/preptlist.c.
  • Updatable views work via:
    • INSTEAD OF trigger, in rewriteTargetListIU add whole-view resjunk col to be passed to trigger
    • For INSTEAD OF rules, application of the rule directly substitutes the query in fireRules
    • For automatically updatable views, in rewriteTargetView


Regular views instead work via subquery expansion, then subquery flattening in the optimizer. See this parse, rewrite, and plan tree.

IRC chat:

<RhodiumToad> ringerc: update is basically planned like this: given  update x set a = ... from y where ...;
<RhodiumToad> ringerc: it's treated as a query  select x.foo, ..., x.bar, x.ctid, ... from x,y where ...;
<RhodiumToad> ringerc: where the initial result columsn of the select are matched up to the new row of x
<RhodiumToad> ringerc: so fields that don't change are fetched from the old x.* row, and ones that do change just have the new value expression in the select list
<RhodiumToad> ringerc: in preptlist, the select list is adjusted to add the necessary entries

Current limitations:

  • in preprocess_targetlist an explicit check is made to reject a result_relation that is a subquery.
  • heap_form_tuple for the new tuple requires that the tlist be in the same order as the tuple attributes. (Should not affect join).

How RETURNING works in 9.3

RETURNING in 9.3 is the same on a non-S.B. view as on any simple statement, as the updatable views code unconditionally flattens out subqueries in views.

In the parser

At this point we have no idea it's on a view yet, it's just on a relation token.

  • RETURNING target_list becomes returning_clause in gram.y
  • returning_clause isn't a struct and isn't defined directly anywhere; it's part of the structure of DeleteStmt, InsertStmt and UpdateStmt in gram.y, where its target list is added to the n->returningList of the relevant blahStmt parse output node. The target list is a List of target_el, which seems to generate a list of ResTarget nodes.
  • InsertStmt etc are defined in src/include/nodes/parsenodes.h as List *returningClause, and created in gram.y by the parser rules.
  • TransformReturningList in src/backend/parser/analyze.c then gets the list and invokes transformTargetList with it. The comment on transformTargetList notes that it "Turns a list of ResTarget's into a list of TargetEntry's.". This is where wildcard * and blah.* expansion happens, and conversion from ResTarget to TargetEntry. Replacement appears to be in-place.

In the rewriter

We're going to do view expansion here. The RETURNING list is in the query's returningList as a List of TargetEntry.

It looks like most of the work on returning lists happens in rules, but also some in RewriteQuery, where line 3027 enforces that an INSTEAD rule must have a RETURNING if the outer query has RETURNING, so it's not interesting to us. All the rest of the guts is in rewriteRuleAction.

In the (new rewriter)

In the current updatable views, this won't get called because we've already flattened the view away. (Confirmed by breaking on rewriteRuleAction). So it isn't interesting, but will diverge in the new updatable s.b. code. Except that it doesn't get hit, because the view query doesn't have a RETURNING clause, it's a SELECT. The RETURNING only exists on the outer query at this point.

Given that, where are the extra tlist entries in the inner query coming from; the plans output from the rewriter are different on the inner plan when there's a returning list. Is expand_targetlist doing that? Doesn't look like it, because it's invoked before view expansion happens. (Separately, expand_targetlist should be dealing with injecting the default expression - TODO)

So the expansion looks like it occurs during sometime after.

It's still correct at RewriteQuery:3013.

It's still correct at the next rewrite pass.

The next rewrite pass does view expansion. At this point the inner tlist has two entries, for varno 3 (the base table, as varno 1 and 2 are old and new, repectively) for varattno 1 and 2. Sane and sensible.

The outer query has two targetentries too, both CONSTs. They don't reference the inner query; the one explicitly specified by the user should not, but the one that was added by expand_targetlist should now point to the inner query. How does this happen with a normal (SELECT) view? Probably doesn't matter, since there are no DEFAULT, and after all the tlist really is from the outer query.

The RETURNING entry is still varno=1, i.e the inner query, with varattno=2 ("secret") at this point. So again, correct at rewriteHandler.c:3198.

Still correct at end of postgres.c:974, after rewrite. Looks like it isn't the rewriter at fault. Though this is where we need to implement DEFAULT handling, possibly by seeing whether the original caller of expand_targetlist dealt with that and whether we can borrow something from there.

In the planner

By now the original code has totally diverged; 9.3 has a flat update here, where updatable s.b.v has a subquery.

breakpoint is pg_plan_queries

Since it looks like rewriter output is correct other than the default, this is the main suspect.

In the planner (new code)

After planner called from postgres.c:753, plan tree has a single outer tlist entry for the update, with a Var referencing the inner query (65001) varattno 3. varattno 3 is newly injected into the subplan and references varno 1, varattno 2. The inner plan is now just a RESULT, presumably becuause it was only const expressions, but it has a Var in it.

So our culprit looks like planner(...).

The resultRelation is RTI 2; RTI 1 is referenced by the inner RESULT query's TLE 3, the Var. It is resjunk, referencing varattno 2 (secret) of vartype 25 (text). This is referenced by both the modifytable and returning nodes in the outer tlist.

That in turn is produced by subquery_planner.

The faulty plan seems to be introduced by grouping_planner.

At planner.c:1512 the sub_tlist is set as the plan's targetlist. This sub_tlist contains the Var that we're concerned about. The sub_tlist comes from earlier in grouping_planner, at planner.c:1185:

        /*
         * Generate appropriate target list for subplan; may be different from
         * tlist if grouping or aggregation is needed.
         */
        sub_tlist = make_subplanTargetList(root, tlist,
                                           &groupColIdx, &need_tlist_eval);

Wondering if the real culprit is earlier, though, at:

        /* Preprocess targetlist */
        tlist = preprocess_targetlist(root, tlist);

Before the call the tlist is:

(
   {TARGETENTRY 
   :expr 
      {CONST 
      :consttype 23 
      :consttypmod -1 
      :constcollid 0 
      :constlen 4 
      :constbyval true 
      :constisnull false 
      :location 40 
      :constvalue 4 [ 102 0 0 0 0 0 0 0 ]
      }
   :resno 1 
   :resname id 
   :ressortgroupref 0 
   :resorigtbl 0 
   :resorigcol 0 
   :resjunk false
   }
   {TARGETENTRY 
   :expr 
      {CONST 
      :consttype 25 
      :consttypmod -1 
      :constcollid 100 
      :constlen -1 
      :constbyval false 
      :constisnull false 
      :location 45 
      :constvalue 8 [ 32 0 0 0 102 114 101 100 ]
      }
   :resno 2 
   :resname secret 
   :ressortgroupref 0 
   :resorigtbl 0 
   :resorigcol 0 
   :resjunk false
   }
)

and after it is:

(
   {TARGETENTRY 
   :expr 
      {CONST 
      :consttype 23 
      :consttypmod -1 
      :constcollid 0 
      :constlen 4 
      :constbyval true 
      :constisnull false 
      :location 40 
      :constvalue 4 [ 102 0 0 0 0 0 0 0 ]
      }
   :resno 1 
   :resname id 
   :ressortgroupref 0 
   :resorigtbl 0 
   :resorigcol 0 
   :resjunk false
   }
   {TARGETENTRY 
   :expr 
      {CONST 
      :consttype 25 
      :consttypmod -1 
      :constcollid 100 
      :constlen -1 
      :constbyval false 
      :constisnull false 
      :location 45 
      :constvalue 8 [ 32 0 0 0 102 114 101 100 ]
      }
   :resno 2 
   :resname secret 
   :ressortgroupref 0 
   :resorigtbl 0 
   :resorigcol 0 
   :resjunk false
   }
   {TARGETENTRY 
   :expr 
      {VAR 
      :varno 1 
      :varattno 1 
      :vartype 23 
      :vartypmod -1 
      :varcollid 0 
      :varlevelsup 0 
      :varnoold 1 
      :varoattno 1 
      :location 63
      }
   :resno 3 
   :resname <> 
   :ressortgroupref 0 
   :resorigtbl 0 
   :resorigcol 0 
   :resjunk true
   }
   {TARGETENTRY 
   :expr 
      {VAR 
      :varno 1 
      :varattno 2 
      :vartype 25 
      :vartypmod -1 
      :varcollid 100 
      :varlevelsup 0 
      :varnoold 1 
      :varoattno 2 
      :location 63
      }
   :resno 4 
   :resname <> 
   :ressortgroupref 0 
   :resorigtbl 0 
   :resorigcol 0 
   :resjunk true
   }
)

Ha! GOTCHA Y BAST. What's happening is that preprocess_targetlist is adding Var nodes to the target list of the subquery so that the outer query's returning list can reference them. The returning list is:

   :returningList (
      {TARGETENTRY 
      :expr 
         {VAR 
         :varno 1 
         :varattno 1 
         :vartype 23 
         :vartypmod -1 
         :varcollid 0 
         :varlevelsup 0 
         :varnoold 1 
         :varoattno 1 
         :location 63
         }
      :resno 1 
      :resname id 
      :ressortgroupref 0 
      :resorigtbl 16392 
      :resorigcol 1 
      :resjunk false
      }
      {TARGETENTRY 
      :expr 
         {VAR 
         :varno 1 
         :varattno 2 
         :vartype 25 
         :vartypmod -1 
         :varcollid 100 
         :varlevelsup 0 
         :varnoold 1 
         :varoattno 2 
         :location 63
         }
      :resno 2 
      :resname secret 
      :ressortgroupref 0 
      :resorigtbl 16392 
      :resorigcol 2 
      :resjunk false
      }
   )

It points to varno 1 but resultRelation is 2. So it's "helpfully" inserting vars pointing to the inner query.

Maybe that's ok at this point, then a later transformation (producing a simple result plan from this tlist, maybe) screws it up? Figure it out.

pull_var_clause returns:

(
   {VAR 
   :varno 1 
   :varattno 1 
   :vartype 23 
   :vartypmod -1 
   :varcollid 0 
   :varlevelsup 0 
   :varnoold 1 
   :varoattno 1 
   :location 63
   }
   {VAR 
   :varno 1 
   :varattno 2 
   :vartype 25 
   :vartypmod -1 
   :varcollid 100 
   :varlevelsup 0 
   :varnoold 1 
   :varoattno 2 
   :location 63
   }
)

which we then loop over. They do NOT get excluded because the resultrelation isn't equal to the origin relation (correctly, since the origin rel is the subquery, and the resultrel isn't).

This is really wrong. The resultrel *should* be the subquery in this context; as far as this code is concerned that'd be perfectly correct. However, it's wrong in other contexts - when we're expanding the targetlist, when we're actually doing modifytuple, etc.

Maybe we do need to split "result relation" into "source relation" and "target relation" after all. There are other circumstances where it'd be nice to know that the subquery was the result relation. OTOH, what about multiple levels of nesting? Will that be a problem?

Jumping over the addition of the var to the tlist results in:

ERROR:  variable not found in subplan target lists

... because the returning list gets transformed to refer to these new Vars, not the perfectly good consts already present. So we need to teach expand_targetlist not to add these Vars, and whatever's mangling the target list to refer to the originals instead. It's trying to solve a problem that doesn't exist - assuming that the desired vars aren't in the target rel when we know they really are.

So we need to figure out what it is that's doing the tlist rewriting. Fun times. The parsetree tlist sure isn't copied verbatim. I'm suspicious of set_plan_refs in setrefs.c, case T_ModifyTable:. It takes as input the regular tlist:

((
   {TARGETENTRY 
   :expr 
      {VAR 
      :varno 1 
      :varattno 1 
      :vartype 23 
      :vartypmod -1 
      :varcollid 0 
      :varlevelsup 0 
      :varnoold 1 
      :varoattno 1 
      :location 63
      }
   :resno 1 
   :resname id 
   :ressortgroupref 0 
   :resorigtbl 16392 
   :resorigcol 1 
   :resjunk false
   }
   {TARGETENTRY 
   :expr 
      {VAR 
      :varno 1 
      :varattno 2 
      :vartype 25 
      :vartypmod -1 
      :varcollid 100 
      :varlevelsup 0 
      :varnoold 1 
      :varoattno 2 
      :location 63
      }
   :resno 2 
   :resname secret 
   :ressortgroupref 0 
   :resorigtbl 16392 
   :resorigcol 2 
   :resjunk false
   }
))

and hands that off to set_returning_clause_references. That is indeed the culprit! Woo!

(It also highlights a bug: our RETURNING would've returned results from before the effect of BEFORE triggers. We should be showing the final result tuple.)

So we need to modify the plan structure to add separate source and target relations (KaiGai was right, just approached it really weirdly) and then teach expand_targetlist and set_returning_clause_references about them. So the returning Var **must** refer to the result relation, not the source relation, but not in an inner subquery, since it's not defined there.

expand_targetlist shouldn't be adding any Vars if the rti of the returning clause is that of the source relation. Those vars will get transformed to refer to the result relation by set_returning_clause_references.

HANG ON! Why do we need sourcerelation after all? Don't we just rewrite the returning-list when we expand the view, so the returning entries point to the final RTE? CHECK APPROACH. First expand_targetlist invocation won't think there's any funny business because we'll only change what resultRelation points to after we call it or we'll fix the returning list before calling it. Second won't see any funny business because the returning list will be rewritten to point to the result relation by then. set_returning_clause_references won't need to transform the returning list to point to the underlying result table because it already will.

Planner outputs

The final plan returned by subquery_planner is:


   {RESULT 
   :startup_cost 0.00 
   :total_cost 0.01 
   :plan_rows 1 
   :plan_width 0 
   :targetlist (
      {TARGETENTRY 
      :expr 
         {CONST 
         :consttype 23 
         :consttypmod -1 
         :constcollid 0 
         :constlen 4 
         :constbyval true 
         :constisnull false 
         :location 40 
         :constvalue 4 [ 102 0 0 0 0 0 0 0 ]
         }
      :resno 1 
      :resname id 
      :ressortgroupref 0 
      :resorigtbl 0 
      :resorigcol 0 
      :resjunk false
      }
      {TARGETENTRY 
      :expr 
         {CONST 
         :consttype 25 
         :consttypmod -1 
         :constcollid 100 
         :constlen -1 
         :constbyval false 
         :constisnull false 
         :location 45 
         :constvalue 8 [ 32 0 0 0 102 114 101 100 ]
         }
      :resno 2 
      :resname secret 
      :ressortgroupref 0 
      :resorigtbl 0 
      :resorigcol 0 
      :resjunk false
      }
      {TARGETENTRY 
      :expr 
         {VAR 
         :varno 1 
         :varattno 1 
         :vartype 23 
         :vartypmod -1 
         :varcollid 0 
         :varlevelsup 0 
         :varnoold 1 
         :varoattno 1 
         :location 63
         }
      :resno 3 
      :resname <> 
      :ressortgroupref 0 
      :resorigtbl 0 
      :resorigcol 0 
      :resjunk true
      }
      {TARGETENTRY 
      :expr 
         {VAR 
         :varno 1 
         :varattno 2 
         :vartype 25 
         :vartypmod -1 
         :varcollid 100 
         :varlevelsup 0 
         :varnoold 1 
         :varoattno 2 
         :location 63
         }
      :resno 4 
      :resname <> 
      :ressortgroupref 0 
      :resorigtbl 0 
      :resorigcol 0 
      :resjunk true
      }
   )
   :qual <> 
   :lefttree <> 
   :righttree <> 
   :initPlan <> 
   :extParam (b)
   :allParam (b)
   :resconstantqual <>
   }

and the overall plan including outer from planner is:

</pre>

  {PLANNEDSTMT 
  :commandType 3 
  :queryId 0 
  :hasReturning true 
  :hasModifyingCTE false 
  :canSetTag true 
  :transientPlan false 
  :planTree 
     {MODIFYTABLE 
     :startup_cost 0.00 
     :total_cost 0.01 
     :plan_rows 1 
     :plan_width 0 
     :targetlist (
        {TARGETENTRY 
        :expr 
           {VAR 
           :varno 65001 
           :varattno 3 
           :vartype 23 
           :vartypmod -1 
           :varcollid 0 
           :varlevelsup 0 
           :varnoold 1 
           :varoattno 1 
           :location 63
           }
        :resno 1 
        :resname id 
        :ressortgroupref 0 
        :resorigtbl 16392 
        :resorigcol 1 
        :resjunk false
        }
        {TARGETENTRY 
        :expr 
           {VAR 
           :varno 65001 
           :varattno 4 
           :vartype 25 
           :vartypmod -1 
           :varcollid 100 
           :varlevelsup 0 
           :varnoold 1 
           :varoattno 2 
           :location 63
           }
        :resno 2 
        :resname secret 
        :ressortgroupref 0 
        :resorigtbl 16392 
        :resorigcol 2 
        :resjunk false
        }
     )
     :qual <> 
     :lefttree <> 
     :righttree <> 
     :initPlan <> 
     :extParam (b)
     :allParam (b)
     :operation 3 
     :canSetTag true 
     :resultRelations (i 2)
     :resultRelIndex 0 
     :plans (
        {RESULT 
        :startup_cost 0.00 
        :total_cost 0.01 
        :plan_rows 1 
        :plan_width 0 
        :targetlist (
           {TARGETENTRY 
           :expr 
              {CONST 
              :consttype 23 
              :consttypmod -1 
              :constcollid 0 
              :constlen 4 
              :constbyval true 
              :constisnull false 
              :location 40 
              :constvalue 4 [ 102 0 0 0 0 0 0 0 ]
              }
           :resno 1 
           :resname id 
           :ressortgroupref 0 
           :resorigtbl 0 
           :resorigcol 0 
           :resjunk false
           }
           {TARGETENTRY 
           :expr 
              {CONST 
              :consttype 25 
              :consttypmod -1 
              :constcollid 100 
              :constlen -1 
              :constbyval false 
              :constisnull false 
              :location 45 
              :constvalue 8 [ 32 0 0 0 102 114 101 100 ]
              }
           :resno 2 
           :resname secret 
           :ressortgroupref 0 
           :resorigtbl 0 
           :resorigcol 0 
           :resjunk false
           }
           {TARGETENTRY 
           :expr 
              {VAR 
              :varno 1 
              :varattno 1 
              :vartype 23 
              :vartypmod -1 
              :varcollid 0 
              :varlevelsup 0 
              :varnoold 1 
              :varoattno 1 
              :location 63
              }
           :resno 3 
           :resname <> 
           :ressortgroupref 0 
           :resorigtbl 0 
           :resorigcol 0 
           :resjunk true
           }
           {TARGETENTRY 
           :expr 
              {VAR 
              :varno 1 
              :varattno 2 
              :vartype 25 
              :vartypmod -1 
              :varcollid 100 
              :varlevelsup 0 
              :varnoold 1 
              :varoattno 2 
              :location 63
              }
           :resno 4 
           :resname <> 
           :ressortgroupref 0 
           :resorigtbl 0 
           :resorigcol 0 
           :resjunk true
           }
        )
        :qual <> 
        :lefttree <> 
        :righttree <> 
        :initPlan <> 
        :extParam (b)
        :allParam (b)
        :resconstantqual <>
        }
     )
     :withCheckOptionLists <> 
     :returningLists ((
        {TARGETENTRY 
        :expr 
           {VAR 
           :varno 65001 
           :varattno 3 
           :vartype 23 
           :vartypmod -1 
           :varcollid 0 
           :varlevelsup 0 
           :varnoold 1 
           :varoattno 1 
           :location 63
           }
        :resno 1 
        :resname id 
        :ressortgroupref 0 
        :resorigtbl 16392 
        :resorigcol 1 
        :resjunk false
        }
        {TARGETENTRY 
        :expr 
           {VAR 
           :varno 65001 
           :varattno 4 
           :vartype 25 
           :vartypmod -1 
           :varcollid 100 
           :varlevelsup 0 
           :varnoold 1 
           :varoattno 2 
           :location 63
           }
        :resno 2 
        :resname secret 
        :ressortgroupref 0 
        :resorigtbl 16392 
        :resorigcol 2 
        :resjunk false
        }
     ))
     :fdwPrivLists (<>)
     :rowMarks <> 
     :epqParam 0
     }
  :rtable (
     {RTE 
     :alias <> 
     :eref 
        {ALIAS 
        :aliasname t_even 
        :colnames ("id" "secret")
        }
     :rtekind 1 
     :subquery <> 
     :security_barrier false 
     :lateral false 
     :inh false 
     :inFromCl false 
     :requiredPerms 0 
     :checkAsUser 0 
     :selectedCols (b)
     :modifiedCols (b)
     }
     {RTE 
     :alias <> 
     :eref 
        {ALIAS 
        :aliasname t 
        :colnames ("id" "secret")
        }
     :rtekind 0 
     :relid 16384 
     :relkind r 
     :lateral false 
     :inh false 
     :inFromCl true 
     :requiredPerms 3 
     :checkAsUser 10 
     :selectedCols (b 9 10)
     :modifiedCols (b 9 10)
     }
  )
  :resultRelations (i 2)
  :utilityStmt <> 
  :subplans <> 
  :rewindPlanIDs (b)
  :rowMarks <> 
  :relationOids (o 16384)
  :invalItems <> 
  :nParamExec 1
  }

</pre>



Useful: info locals, list.

Glossary

What's a var? RangeTable? RTE? Attr? See Source Glossary and Tom's Intro to hacking the query planner.

Adding security_barrier support

Doing it cleanly

To add security_barrier support for updatable views we need to teach UPDATE and DELETE to operate on a subquery. This would be very useful for row-security.

This isn't unprecedented; we support UPDATE ... FROM already:

test=# explain UPDATE t SET id = gs.n FROM (SELECT generate_series(1,10)) gs(n) WHERE t.id = gs.n;
                                  QUERY PLAN                                  
------------------------------------------------------------------------------
 Update on t  (cost=150.26..247.51 rows=6150 width=70)
   ->  Merge Join  (cost=150.26..247.51 rows=6150 width=70)
         Merge Cond: (gs.n = t.id)
         ->  Sort  (cost=64.84..67.34 rows=1000 width=32)
               Sort Key: gs.n
               ->  Subquery Scan on gs  (cost=0.00..15.01 rows=1000 width=32)
                     ->  Result  (cost=0.00..5.01 rows=1000 width=0)
         ->  Sort  (cost=85.43..88.50 rows=1230 width=42)
               Sort Key: t.id
               ->  Seq Scan on t  (cost=0.00..22.30 rows=1230 width=42)
(10 rows)

where the root node of the UPDATE is a MergeJoin not a direct table or index scan.

Can that be extended to a SubqueryScan? Robert thinks it probably can.

Prior approaches

The 9.4 RLS patch implements an equivalent feature by internally replacing the RTE_RELATION range-table entry for the RLS-affected table with an RTE_SUBQUERY. It then has to do fixup for resjunk columns (temporary sort keys not output in the final result set, ctid, etc). A bunch of fixups are required to remap Vars and attribute numbers between the base table and the subquery. Additionally, changes are required to teach UPDATE that the relation it is scanning isn't necessarily the same as the relation it is updating.

Dean Rasheed separated the RLS code into a prototype patch to implement updatable s.b. views. This patch has the same issues as RLS.


How to proceed

QUERY
* :targetRelation (rti 1)
* ModifyTable
** SubqueryScan (rti 2)
*** SeqScan (ref rti 1)
**** Filter
* RangeTable
** RTE_RELATION underlying target-relation
** RTE_SUBQUERY subquery source

here the target relation is still a RTE_RELATION.

See mailing list post with proposed approach

Need to:

  • Remove security barrier check in view_query_is_auto_updatable
  • Omit/replace/rewrite rewriteTargetView
    • After new_rt_index = list_length(parsetree->rtable);, assign new index to parsetree->resultRelation
    • Add missing columns (marked resjunk?) to the view subquery plan, so we can see all old values re-used in UPDATE. Can omit any values already being set, but won't bother with that. (TODO: Ensure that this doesn't leak the column values via whole-tuple reference to inner subquery passed to function).
      • Delete copying of view targetlist. Should be rewritten in place instead.
      • Adapt the per-column permission bits code so it sets the bits in-place on the view query, not on the copy
      • Delete all the rest of the flattening code
      • Add FOR UPDATE clause to view

Can it really be that easy? Think. This should crash, surely. Yes, it must fail because we haven't injected the ctid into the view, so when the ModifyRelation node is reached it'll fail to find the ctid in the input.

Nope. Fails earlier, because the RTE for the inner query is added at the bottom level, needs to be added at the outer level in order for it to be valid to refer to it in :resultRelation. It must be outside the subquery. Weird, because that looks like what our code does, adds it to the *outer* query.

It looks rather a lot like ApplyRetrieveRule does the work that should gets done for auto-updatable views in rewriteTargetView, and does so more simply. Not quite the same though:

  • Need to set write permission checks on updated view cols, so the view owner is checked for the right to update cols via the view
  • Need to copy ONLY flag (inheritance) for UPDATE, DELETE
  • Need to inject ctid, any cols from base rel not already present in view (as resjunk?)

so rather than ApplyRetrieveRule it looks more like we want to enhance the view expansion rewrite code instead. What causes RewriteQuery to call rewriteTargetView instead of normal view expansion?

Answer appears to be backend/rewrite/rewriteHandler.c line 2839. We should use the standard view expansion code on this instead, cons it to the product, then set the "instead" flag, replacing rewriteTargetView with the regular view rewriting code. The "view expansion code" is the rules code, since views are internally just _RETURN rules. So how do we handle that - we'd be returning a new SELECT query? Can we wrap that up into a subquery? Isn't that just what rewriteTargetView does - gets the rule and bundles it into a subquery then fixes it up?

View rules have is_instead=1, ev_type=1 (select), and an ev_action.

It looks like rewriteTargetListUD already adds the ctid when the target is a rel. When it's a view, it adds a whole-row reference Var to contain the "old" rows. (How do we get the ctid then? Answer: it doesn't, it passes the "old" row to the INSTEAD OF trigger.)

So, we need to:

  • Alter rewriteTargetListUD to inject a reference to ctid down the chain to the base rel of the view. Might be tricky because view expansion is done recursing down the view tree, not up it.
  • Ensure that views are expanded as subqueries, not initially flattened. It's fine if the optimizer flattens them later.
  • Make sure view permissions checks are honoured still.
  • respect ONLY flag for insert/update by turning the view into a SELECT ... FROM ONLY where appropriate. (Is this correct?)
  • Must inject ctid into view as resjunk. Current code doesn't need it since it passes the old tuple to the view instead of trigger instead.

Code flow (problem 1)

crash in find_base_rel (SOLVED)

  • PostgresMain
    • exec_simple_query
      • querytree_list = pg_analyze_and_rewrite(parsetree, ...)
      • plantree_list = pg_plan_queries(querytree_list, ...)
        • pg_plan_query
          • planner
            • standard_planner
                • subquery_planner
                  • grouping_planner (from planner.c:572)
                    • query_planner (from planner.c:1236) - generates paths
                      • setup_simple_rel_arrays (from planmain.c:113)
                      • add_base_rels_to_query (recursive, then invokes:)
                        • build_simple_rel (at relnode.c:94)
                      • build_base_rel_tlists
                        • add_vars_to_targetlist
                          • find_base_rel

find_base_rel fails because the root->simple_rel_array entry for relid 2 doesn't exist.

root->simple_rte_array is populated by setup_simple_rel_arrays. setup_simple_rel_arrays seems to fully populate root->simple_rte_array. It initializes root->simple_rel_array too, but leaves all entries null.

root->simple_rel_array is populated by build_simple_rel via add_base_rels_to_query.

At the moment build_simple_rel only gets called for relid 5.

The lookup is failing in build_base_rel_tlists because the simple_rel_array entry is null for that RangeTblEntry.

It looks like add_base_rels_to_query doesn't ever see relid=2, because it's an RTE that isn't referenced inside the query tree. We need to do a post-check to make sure that resultRelation's RTE is defined and if not, set it up.

Fixed with:

    /*  
     * The top query's resultRelation RTI may not be referenced in the query
     * tree its self, so we need to ensure it gets cached too. Curently arises
     * when we're doing DML on a view, where the injected top-level RTE for the
     * view's base rel isn't referenced in the query tree.
     */
    if (root->parse->resultRelation && root->simple_rel_array[root->parse->resultRelation] == NULL)
        (void) build_simple_rel(root, root->parse->resultRelation, RELOPT_BASEREL);

    Assert(root->parse->resultRelation == 0 || root->simple_rel_array[root->parse->resultRelation] != NULL);

after add_base_rels_to_query in query_planner(...)

Problem 2, make_one_rel (SOLVED)

Now crashes at make_one_rel:

    /*
     * Ready to do the primary planning.
     */
    final_rel = make_one_rel(root, joinlist);

Fails at:

#3  0x00000000005f15b0 in make_one_rel (root=root@entry=0x16c9d30, joinlist=joinlist@entry=0x16d32f8) at allpaths.c:148
148             Assert(bms_equal(rel->relids, root->all_baserels));

Simply commenting this assertion out makes it work! Woo yeah!

The assertion fails because the list of all_baserels contains the resultRelation, but the list of rels found by traversing the plan join list doesn't. Not clear what the right answer is. It's the same rel, but it appears more than once with different access rights and at different query levels.

If we set the relation in fix1 to RELOPT_DEADREL then there's no need to remove this assertion as it's no longer expected to be part of the join tree. Should maybe create a RELOPT_RESULTREL?

Missing DEFAULT

DEFAULT isn't respected. whatever we're doing with expand_targetlist in the view subquery expansion needs to handle default too; perhaps we should be using preprocess_targetlist after all?

RETURNING crash in add_rtes_to_flat_rtable (SOLVED)

  • INSERT ... RETURNING is broken (crash)
  • UPDATE ... RETURNING is broken (crash)

It crashes at

Program received signal SIGSEGV, Segmentation fault.
0x000000000060e54f in add_rtes_to_flat_rtable (root=root@entry=0x226c508, recursing=recursing@entry=0 '\000') at setrefs.c:285
285                             RelOptInfo *rel = root->simple_rel_array[rti];

with stack

(gdb) bt
#0  0x000000000060e54f in add_rtes_to_flat_rtable (root=root@entry=0x226c508, recursing=recursing@entry=0 '\000') at setrefs.c:285
#1  0x000000000060fe39 in set_plan_references (root=0x226c508, plan=plan@entry=0x2272950) at setrefs.c:208
#2  0x000000000060dde7 in standard_planner (parse=0x2222730, cursorOptions=0, boundParams=<optimized out>) at planner.c:227
#3  0x000000000060df55 in planner (parse=parse@entry=0x2222730, cursorOptions=cursorOptions@entry=0, boundParams=boundParams@entry=0x0) at planner.c:139
#4  0x000000000067d40b in pg_plan_query (querytree=0x2222730, cursorOptions=cursorOptions@entry=0, boundParams=boundParams@entry=0x0) at postgres.c:753
#5  0x000000000067d493 in pg_plan_queries (querytrees=<optimized out>, cursorOptions=cursorOptions@entry=0, boundParams=boundParams@entry=0x0) at postgres.c:812
#6  0x000000000067d6f2 in exec_simple_query (query_string=query_string@entry=0x22214d8 "insert into t_even(id, secret) values (default, 'freddd') returning *;")
    at postgres.c:977
#7  0x000000000067f244 in PostgresMain (argc=<optimized out>, argv=argv@entry=0x21bf338, dbname=0x21bf198 "postgres", username=<optimized out>) at postgres.c:3992
#8  0x0000000000634bc7 in BackendRun (port=port@entry=0x21dd050) at postmaster.c:4085
#9  0x00000000006364e2 in BackendStartup (port=port@entry=0x21dd050) at postmaster.c:3774
#10 0x0000000000636746 in ServerLoop () at postmaster.c:1585
#11 0x000000000063789e in PostmasterMain (argc=argc@entry=4, argv=argv@entry=0x21bc700) at postmaster.c:1240
#12 0x00000000005cb0b0 in main (argc=4, argv=0x21bc700) at main.c:196

because root->simple_rel_array is null.

It gets set at setup_simple_rel_arrays in src/backend/optimizer/util/relnode.c:67. Unconditionally. So that must not have been called when it should've - maybe because there's no join tree, no relation with RELOPT_BASEREL?

Looks like that's because query_planner falls through early if parse->jointree->fromlist == NIL. We can't just test for non-zero result relation because it's OK for insert, etc.

But. Need to verify this is actually problem site. Confirmed we do enter this case and that parse->jointree->fromlist is truly NIL. But under what circumstances isn't it simple?
  • RETURNING? No, because we hit this case in "INSERT ... VALUES ... RETURNING" ?

Yes, but that doesn't crash in add_rtes_to_flat_rtable because RTEKIND != RTE_SUBQUERY so the code to add RTEs for dead subqueries doesn't get invoked. Maybe it should not be invoked at all here, the subquery isn't dead?

It works fine for simple INSERT ... VALUES ... RETURNING. Why? It still enters the simple case and doesn't touch root->simple_rel_array. It doesn't crash because it has no RTE_SUBQUERY nodes, so it never enters the code that expects it to be inited.

Can we skip this RTE_SUBQUERY since we know it isn't dead and we have a simple plan? Maybe, but we can't skip all RTE_SUBQUERY nodes, since they might genuinely be dead subqueries from WHERE clause etc.

Basically we need to ensure that build_simple_rel gets invoked and sets RELOPT_RESULTREL to add the rel and its children (if any). OR a way to avoid the need.

For insert, the view subquery really is dead. It doesn't appear in the tlist (which contains only constants and / or refs to other join elements) and the resultRelation now just points to the underlying RTE_RELATION. We already do permissions checks for writing to it in the top level RTE_RELATION, but we also need to do checks for any nested view RTEs, and that means pulling up the RTEs into the top query.

To pull them up we currently need a simple_rel_array. We can't really just skip pull-up, since we'd fail to check permissions where the user has rights on the base table but not the view. Or can we just skip pull-up? Isn't it already referenced via the tlist? The code says:

                                 * Otherwise, it should be represented by a SubqueryScan node
                                 * somewhere in our plan tree, and we'll pull up its RTEs when
                                 * we process that plan node.

and this should be true for us; it's a SubqueryScan under the ModifyTable. Unless we're doing an INSERT, in which case it isn't, and we still need to check its perms. Damn. Pull-up is required:

postgres=# explain insert into t_even(id, secret) values (default, 'freddd');
                   QUERY PLAN                   
------------------------------------------------
 Insert on t  (cost=0.00..0.01 rows=1 width=0)
   ->  Result  (cost=0.00..0.01 rows=1 width=0)
(2 rows)

... yet it seems to work anyway. Um. Seems to be a perms check earlier on.

#0  aclcheck_error (aclerr=aclerr@entry=ACLCHECK_NO_PRIV, objectkind=objectkind@entry=ACL_KIND_CLASS, objectname=0x2baff20 "t_even") at aclchk.c:3362
#1  0x0000000000598fe5 in ExecCheckRTPerms (rangeTable=rangeTable@entry=0x2bb36b0, ereport_on_violation=ereport_on_violation@entry=1 '\001') at execMain.c:520
#2  0x00000000005997da in InitPlan (queryDesc=queryDesc@entry=0x2c04b68, eflags=eflags@entry=0) at execMain.c:730
#3  0x0000000000599d29 in standard_ExecutorStart (queryDesc=0x2c04b68, eflags=0) at execMain.c:208
#4  0x0000000000599d65 in ExecutorStart (queryDesc=queryDesc@entry=0x2c04b68, eflags=eflags@entry=0) at execMain.c:122
#5  0x000000000067fffb in ProcessQuery (plan=plan@entry=0x2bb3910, sourceText=0x2b53508 "insert into t_even(secret) values ('arg');", params=0x0, dest=dest@entry=0x2bb3a28, 
    completionTag=completionTag@entry=0x7fffb2ec2770 "") at pquery.c:180
#6  0x00000000006801e9 in PortalRunMulti (portal=portal@entry=0x2b94b58, isTopLevel=isTopLevel@entry=1 '\001', dest=dest@entry=0x2bb3a28, altdest=altdest@entry=0x2bb3a28, 
    completionTag=completionTag@entry=0x7fffb2ec2770 "") at pquery.c:1279
#7  0x0000000000680ecd in PortalRun (portal=portal@entry=0x2b94b58, count=count@entry=9223372036854775807, isTopLevel=isTopLevel@entry=1 '\001', dest=dest@entry=0x2bb3a28, 
    altdest=altdest@entry=0x2bb3a28, completionTag=completionTag@entry=0x7fffb2ec2770 "") at pquery.c:816
#8  0x000000000067d7eb in exec_simple_query (query_string=query_string@entry=0x2b53508 "insert into t_even(secret) values ('arg');") at postgres.c:1048
#9  0x000000000067f244 in PostgresMain (argc=<optimized out>, argv=argv@entry=0x2b0f358, dbname=0x2b0f1b8 "postgres", username=<optimized out>) at postgres.c:3992
#10 0x0000000000634bc7 in BackendRun (port=port@entry=0x2b11060) at postmaster.c:4085
#11 0x00000000006364e2 in BackendStartup (port=port@entry=0x2b11060) at postmaster.c:3774
#12 0x0000000000636746 in ServerLoop () at postmaster.c:1585
#13 0x000000000063789e in PostmasterMain (argc=argc@entry=4, argv=argv@entry=0x2aee700) at postmaster.c:1240
#14 0x00000000005cb0b0 in main (argc=4, argv=0x2aee700) at main.c:196

is where it gets checked. What about column permissions though? They're part of the RTE so they should get checked too.

Confirmed.

So the rewriter is doing the checks we need. We don't need to do RTE pull-up, and in fact the subquery RTE could be entirely gone and we would not care.

We only hit this on RETURNING because the subquery gets eliminated otherwise.

On RETURNING we need to make sure we check *read* permissions; we're possibly reading DEFAULTs. That's consistent with INSERT ... RETURNING into a simple relation; that needs read rights.

For now this change in setrefs.c seems to do the trick, though it feels ugly:

                 * FIXME: Testing root->simple_rel_array here is a dirty hack. What we
                 * really want to say is "if there are no join relations and this is a trivial
                 * plan we don't need to do this."
                 */
                if (rte->rtekind == RTE_SUBQUERY && !rte->inh && root->simple_rel_array_size != 0)

The cleaner solution would probably be to initialize `simple_rel_array` if there's any subquery or rel in the join tree. Doing this without an impact on performance elsewhere might be a bit messy, though; that's a very hot code path, and allocations are expensive. If we ever fix the issue with dead subqueries in WHERE being ignored that'll be necessary anyway; consider the test case I posted on hackers:

[3:24:47 PM] Craig Ringer: I think I might've found a trivial security bug, interested in comments. It's trivial because I can't imagine anyone caring; it's security because it relates to permissions handling.
[3:25:48 PM] Craig Ringer: Given table "t" that the current user has INSERT rights on, and table "e" that they have no rights whatsoever for, the following should fail because the user has no permission on table "e":
[3:26:01 PM] Craig Ringer: postgres=> insert into t(id,secret) values (1111, coalesce('test'::text, (SELECT blah FROM e OFFSET 0 LIMIT 1))) RETURNING *;
  id  | secret 
------+--------
 1111 | test
(1 row)

INSERT 0 1
postgres=> select * from e;
ERROR:  permission denied for relation e
[3:27:25 PM] Craig Ringer: .... but as you can see, it succeeds. Dead subquery elimination cuts out the subquery that references 'e' entirely ... and its FROM list is not pulled up into the outer query to preserve its permissions checks. This constrasts with other places where we DO permissions checks on dead relations; in particular, we DO pull-up the RTEs from a dead subquery in FROM.
[3:30:53 PM] Craig Ringer: Demo on 9.3.1:

craig=> CREATE TABLE t(id serial primary key, secret text);
CREATE TABLE
craig=> CREATE TABLE e(blah text);
CREATE TABLE
craig=> GRANT INSERT ON t TO testing;
GRANT
craig=> \c craig testing
You are now connected to database "craig" as user "testing".
craig=> insert into t(id,secret) values (1111, coalesce('test'::text, (SELECT blah FROM e OFFSET 0 LIMIT 1)));
INSERT 0 1
craig=> select * from e;
ERROR:  permission denied for relation e
[3:31:16 PM] Craig Ringer: .
[3:35:19 PM] Craig Ringer: Hit this while trying to figure out why I was crashing in upd. s.b. views when a RETURNING clause was added. Turns out that for a "trivial" plan root->simple_rel_array is not set and the dead subquery pull-up code crashes if there's a subquery in the rangetable ... but it looks like it wasn't triggered before because subqueries in WHERE (as opposed to FROM) get discarded entirely, without the code in add_rtes_to_flat_rtable(...) being invoked to try to pull-up the RTEs of dead subqueries.
[3:35:31 PM] Craig Ringer: Am I just totally misunderstanding something?
[3:38:50 PM] Craig Ringer: Anyway, struggling to work out how to make the dead subquery elimination code cleanly survive running in a "simple" plan where query_planner never invokes setup_simple_rel_arrays and add_base_rels_to_query. Can just test for null add_base_rels_to_query, but that feels like a nasty hack.
[4:28:59 PM] Andres Freund: We've discussed the issue onlist sometime back and decided it's not really too bad and not trivial to fix.
[4:29:33 PM] Andres Freund: And it's not like it allows you to do anything real bad.
[4:29:36 PM] Craig Ringer: OK, so it's known to be real, and not cared about. Fair enough frankly.
[4:29:42 PM] Craig Ringer: Agreed, that's why I said trivial ;-)

RETURNING crash in slot_getsomeattrs with null slot (SOLVED)

Immediate cause is econtext->ecxt_scantuple is NULL, but projInfo->pi_lastScanVar is non-zero, causing slot_getsomeattrs to be invoked with a null ecxt_scantuple.

(gdb) bt
#0  slot_getsomeattrs (slot=0x0, attnum=2) at heaptuple.c:1280
#1  0x00000000005a1dbf in ExecProject (projInfo=0x1b3ecf8, isDone=isDone@entry=0x7fffbfe1c31c) at execQual.c:5404
#2  0x00000000005b3209 in ExecResult (node=node@entry=0x1b3e6e8) at nodeResult.c:155
#3  0x000000000059afd5 in ExecProcNode (node=node@entry=0x1b3e6e8) at execProcnode.c:373
#4  0x00000000005b1807 in ExecModifyTable (node=node@entry=0x1b3e590) at nodeModifyTable.c:938
#5  0x000000000059afe5 in ExecProcNode (node=node@entry=0x1b3e590) at execProcnode.c:377
#6  0x000000000059858a in ExecutePlan (estate=estate@entry=0x1b3e1e8, planstate=0x1b3e590, operation=operation@entry=CMD_INSERT, sendTuples=sendTuples@entry=1 '\001', 
    numberTuples=numberTuples@entry=0, direction=direction@entry=ForwardScanDirection, dest=dest@entry=0x1b26600) at execMain.c:1474
#7  0x0000000000598c38 in standard_ExecutorRun (queryDesc=0x1b26698, direction=ForwardScanDirection, count=0) at execMain.c:308
#8  0x0000000000598c95 in ExecutorRun (queryDesc=queryDesc@entry=0x1b26698, direction=direction@entry=ForwardScanDirection, count=count@entry=0) at execMain.c:256
#9  0x000000000068000d in ProcessQuery (plan=plan@entry=0x1ae5c00, sourceText=0x1ae45b8 "insert into t_even(secret) values ('fred') returning secret;", params=0x0, 
    dest=dest@entry=0x1b26600, completionTag=completionTag@entry=0x7fffbfe1c550 "") at pquery.c:185
#10 0x00000000006801e9 in PortalRunMulti (portal=portal@entry=0x1b20ae8, isTopLevel=isTopLevel@entry=1 '\001', dest=dest@entry=0x1b26600, altdest=0xb9ac40 <donothingDR>, 
    completionTag=completionTag@entry=0x7fffbfe1c550 "") at pquery.c:1279
#11 0x000000000068048b in FillPortalStore (portal=portal@entry=0x1b20ae8, isTopLevel=isTopLevel@entry=1 '\001') at pquery.c:1056
#12 0x0000000000680e24 in PortalRun (portal=portal@entry=0x1b20ae8, count=count@entry=9223372036854775807, isTopLevel=isTopLevel@entry=1 '\001', dest=dest@entry=0x1ae5db0, 
    altdest=altdest@entry=0x1ae5db0, completionTag=completionTag@entry=0x7fffbfe1c730 "") at pquery.c:785
#13 0x000000000067d7eb in exec_simple_query (query_string=query_string@entry=0x1ae45b8 "insert into t_even(secret) values ('fred') returning secret;") at postgres.c:1048
#14 0x000000000067f244 in PostgresMain (argc=<optimized out>, argv=argv@entry=0x1a82418, dbname=0x1a82278 "postgres", username=<optimized out>) at postgres.c:3992
#15 0x0000000000634bc7 in BackendRun (port=port@entry=0x1aa0130) at postmaster.c:4085
#16 0x00000000006364e2 in BackendStartup (port=port@entry=0x1aa0130) at postmaster.c:3774
#17 0x0000000000636746 in ServerLoop () at postmaster.c:1585
#18 0x000000000063789e in PostmasterMain (argc=argc@entry=3, argv=argv@entry=0x1a7f800) at postmaster.c:1240
#19 0x00000000005cb0b0 in main (argc=3, argv=0x1a7f800) at main.c:196
(gdb) frame 0
#0  slot_getsomeattrs (slot=0x0, attnum=2) at heaptuple.c:1280
1280            if (slot->tts_nvalid >= attnum)

Anomalies noted are that the returning list seems to contain two entries for the base rel and then another for whatever actually gets specified, eg for insert into t_even (id, secret) values (111,'sdf') returning id; then in ExecProject we see:

(gdb) print *slot->tts_tupleDescriptor
$63 = {natts = 3, attrs = 0x10de110, constr = 0x0, tdtypeid = 2249, tdtypmod = -1, tdhasoid = 0 '\000', tdrefcount = -1}

where we'd only expect natts=1. (Could be something like resjunk tricks?).

The attributes are:

(gdb) print *slot->tts_tupleDescriptor->attrs[0]
$66 = {attrelid = 0, attname = {
    data = "id\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000"}, atttypid = 23, attstattarget = -1, attlen = 4, attnum = 1, attndims = 0, 
  attcacheoff = -1, atttypmod = -1, attbyval = 1 '\001', attstorage = 112 'p', attalign = 105 'i', attnotnull = 0 '\000', atthasdef = 0 '\000', attisdropped = 0 '\000', 
  attislocal = 1 '\001', attinhcount = 0, attcollation = 0}
(gdb) print *slot->tts_tupleDescriptor->attrs[1]
$67 = {attrelid = 0, attname = {
    data = "secret\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000"}, atttypid = 25, attstattarget = -1, attlen = -1, attnum = 2, attndims = 0, attcacheoff = -1, 
  atttypmod = -1, attbyval = 0 '\000', attstorage = 120 'x', attalign = 105 'i', attnotnull = 0 '\000', atthasdef = 0 '\000', attisdropped = 0 '\000', attislocal = 1 '\001', 
  attinhcount = 0, attcollation = 100}
(gdb) print *slot->tts_tupleDescriptor->attrs[2]
$68 = {attrelid = 0, attname = {
    data = "\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000"}, atttypid = 23, attstattarget = -1, attlen = 4, attnum = 3, attndims = 0, 
  attcacheoff = -1, atttypmod = -1, attbyval = 1 '\001', attstorage = 112 'p', attalign = 105 'i', attnotnull = 0 '\000', atthasdef = 0 '\000', attisdropped = 0 '\000', 
  attislocal = 1 '\001', attinhcount = 0, attcollation = 0}

i.e. we see an int and a text attr, then another int attr - the id we asked for. The first two have names, the third doesn't. Each is of type Form_pg_attribute.


So, questions:

  • What exactly does ExecProject do? It's better explained in the comments on ProjectInfo. So: ExecProject creates mappings of tuple references between plan layers. I think. Tuples that can be mixes of materialized values (e.g. when there's an expression to evaluate) and virtual values where there's just a ref to a var in another rel.
  • What exactly does slot_getsomeattrs do? It's an optimisation that fills the isnull and Datum arrays of a tuple; mainly to deal with tlist entries that are Var references I think.
  • What's projInfo->pi_lastScanVar

Defined as:

  227  *      lastInnerVar    highest attnum from inner tuple slot (0 if none)
  228  *      lastOuterVar    highest attnum from outer tuple slot (0 if none)
  229  *      lastScanVar     highest attnum from scan tuple slot (0 if none)

(not real helpful if you don't know that code).

  • What's econtext->ecxt_scantuple? In ExprContext, which is the current expression context, i.e. the context expressions are evaluated in at this moment. It doesn't define the tuple types, though; you're supposed to know what they are.
  • What sets econtext->ecxt_scantuple? Lots of places, so we need to work out what sets it *in our case*.
  • What sets projInfo->pi_lastScanVar
  • What sets the attname of a Form_pg_attribute in RETURNING
  • What creates and populates the TupleDesc for the TupleTableSlot?

Path of the null slot:

  • slot at slot_getsomeattrs
  • econtext->ecxt_scantuple at ExecProject; econtext is an ExprContext assigned from projInfo->pi_exprContext earlier in the fn.
  • econtext is node->ps.ps_ProjInfo in ExecResult; node is a PlanState
  • node is PlanState * subplanstate in ExecModifyTable; subplanstate is set as node->mt_plans[node->mt_whichplan]; earlier, where node is a ModifyTableState *.
  • ModifyTableState * node is passed through from ExecProcNode; it's a dispatcher that picks what to run based on node type. It's passed in from ExecutePlan, which comes from queryDesc->planstate in ExecutorMain
  • queryDesc in ExecutorMain contains the planned query, the plan executor state, etc.

So ... are we seeing some kind of mismatch between plan and executor state?

The tlist from the plan is https://gist.github.com/ringerc/39fdb3e57b34357575fd . The full plan is https://gist.github.com/ringerc/d3ce281917e6a4de579e for query insert into t_even(secret) values ('fred') returning secret. The parse tree is https://gist.github.com/ringerc/211e9e0d7df25ea4aa8b.

For contast, the plan with no returning clause is https://gist.github.com/ringerc/cba47f4ff5b895cb029a, and the plan with a const returning clause is https://gist.github.com/ringerc/e32f98079ed5203c7fe7.

There's also this gist of the returning secret plan from 9.3 with the old updatable views code: https://gist.github.com/ringerc/09a0f09d24fc7b04d78a


Notes on the plans:

#define    INNER_VAR            65000           /* reference to inner subplan */
#define    OUTER_VAR            65001           /* reference to outer subplan */
#define    INDEX_VAR            65002           /* reference to index column */

so in the returning code with the new system we seem to be getting a reference to OUTER_VAR ... in the returning list on the outside.

However I think it is dying in the subplan. For one thing, that's the only place with three tlist entries. Also, we're crashing in ExecResult, so we must be processing the Result node.

It looks like we're trying to evaluate a reference to a Var from within a Result node, where a Var is not legal because it looks like a Result may contain only constant expressions.


After a lot of docs and code reading, tracing, etc this was identified as an issue where preprocess_targetlist sees Var tlist entries with varno pointing to the rte of the subquery. So it adds resjunk columns to ensure that the subquery fetches the referenced cols.

(Needs better explanation here - why aren't those vars just taken as entries in the subquery tlist?)

We need to adjust the view tlist so the varno is that of the new baserel, and the attnos are remapped appropriately.

See #In the planner (new code).

After discussion with Andres: this isn't so simple. The RETURNING tlist entry might be pretty much anything - expressions involving both returned cols and other join products, subqueries, subqueries with CTEs, etc. So we need to have a way to insert resjunk cols into the inner subquery to project join products, and need to *recursively* rewrite the expression tree to ensure that any refs to the subquery's baserel are adjusted to point to the new outer resultrel.

It's a lot like what expand_targetlist, preprocess_targetlist, etc and the subquery pullup code do. They insert resjunk cols when a tlist entry or var needs to be visible from an outer level. They recursively rewrite the expression tree, subqueries, etc, using a mutator function. I think we're going to need a new recursive mutator.

Turns out that fixing this requires rewriting the RETURNING list so that the varnos that used to point to the view rel now point to the base rel.

Works:

postgres=# CREATE VIEW v1same AS SELECT * FROM t1;
CREATE VIEW
postgres=# CREATE VIEW v2same AS SELECT * FROM v1same;
CREATE VIEW
postgres=# INSERT INTO v1same (d, a) VALUES (NUMERIC '42', 42) RETURNING *;
 a  | b | c | d  
----+---+---+----
 42 |   |   | 42
(1 row)

INSERT 0 1

Complex update (TODO)

CREATE VIEW vcomplex AS SELECT c, (SELECT a * 42 OFFSET 0) AS a42, (SELECT b||x FROM generate_series(1,1) x) AS bx FROM t1;

UPDATE vcomplex
SET c = 123.22
RETURNING
  *,
  vcomplex.c,
  vcomplex.*,
  vcomplex,
  vcomplex.a42,
  vcomplex.a42 + vcomplex.c,
  (SELECT vcomplex.a42 + vcomplex.c + t1.a FROM t1 WHERE t1.a = vcomplex.c);

Currently fails due to next problem:

Insert via re-ordered view with skipped cols produces ERROR: attribute number ... not found in view targetlist (FIXED)

Test case:

CREATE TABLE t1 (a integer, b text, c float8, d numeric);
CREATE VIEW v1 AS SELECT d, c, a FROM t1;
CREATE VIEW v2 AS SELECT a, d FROM v1;

INSERT INTO v2 (d, a) VALUES (NUMERIC '42', 42);
ERROR:  attribute number 3 not found in view targetlist

INSERT INTO v1 (d, a) VALUES (NUMERIC '42', 42);
ERROR:  attribute number 4 not found in view targetlist

Fired at:

Breakpoint 2, rewriteTargetView (parsetree=parsetree@entry=0x14c9a68, view=view@entry=0x7f283c31a3a8) at rewriteHandler.c:2709
2709                                    elog(ERROR, "attribute number %d not found in view targetlist",
(gdb) bt
#0  rewriteTargetView (parsetree=parsetree@entry=0x14c9a68, view=view@entry=0x7f283c31a3a8) at rewriteHandler.c:2709
#1  0x0000000000656c2a in RewriteQuery (parsetree=parsetree@entry=0x14c9a68, rewrite_events=rewrite_events@entry=0x0) at rewriteHandler.c:2981
#2  0x000000000065730d in QueryRewrite (parsetree=parsetree@entry=0x14c9a68) at rewriteHandler.c:3176
#3  0x0000000000681763 in pg_rewrite_query (query=query@entry=0x14c9a68) at postgres.c:709
#4  0x00000000006817f3 in pg_analyze_and_rewrite (parsetree=parsetree@entry=0x14c9728, 
    query_string=query_string@entry=0x14c86e8 "INSERT INTO v1 (d, a) VALUES (NUMERIC '42', 42) RETURNING *;", paramTypes=paramTypes@entry=0x0, numParams=numParams@entry=0)
    at postgres.c:627
#5  0x0000000000681be0 in exec_simple_query (query_string=query_string@entry=0x14c86e8 "INSERT INTO v1 (d, a) VALUES (NUMERIC '42', 42) RETURNING *;") at postgres.c:980
#6  0x000000000068373e in PostgresMain (argc=<optimized out>, argv=argv@entry=0x14849d8, dbname=0x1484838 "postgres", username=<optimized out>) at postgres.c:4011
#7  0x0000000000638d77 in BackendRun (port=port@entry=0x1487f00) at postmaster.c:4085
#8  0x000000000063a692 in BackendStartup (port=port@entry=0x1487f00) at postmaster.c:3774
#9  0x000000000063a8f6 in ServerLoop () at postmaster.c:1585
#10 0x000000000063ba4e in PostmasterMain (argc=argc@entry=4, argv=argv@entry=0x1463730) at postmaster.c:1240
#11 0x00000000005cec76 in main (argc=4, argv=0x1463730) at main.c:194

Suspicious my (ab)use of expand_targetlist is at fault here. Seemed too easy to be true.

Before expand_targetlist:

      {TARGETENTRY 
      :expr 
         {CONST 
         :consttype 1700 
         :consttypmod -1 
         :constcollid 0 
         :constlen -1 
         :constbyval false 
         :constisnull false 
         :location 38 
         :constvalue 8 [ 32 0 0 0 0 -128 42 0 ]
         }
      :resno 1 
      :resname d 
      :ressortgroupref 0 
      :resorigtbl 0 
      :resorigcol 0 
      :resjunk false
      }
      {TARGETENTRY 
      :expr 
         {CONST 
         :consttype 23 
         :consttypmod -1 
         :constcollid 0 
         :constlen 4 
         :constbyval true 
         :constisnull false 
         :location 44 
         :constvalue 4 [ 42 0 0 0 0 0 0 0 ]
         }
      :resno 3 
      :resname a 
      :ressortgroupref 0 
      :resorigtbl 0 
      :resorigcol 0 
      :resjunk false
      }

After:

      {TARGETENTRY 
      :expr 
         {CONST 
         :consttype 1700 
         :consttypmod -1 
         :constcollid 0 
         :constlen -1 
         :constbyval false 
         :constisnull false 
         :location 38 
         :constvalue 8 [ 32 0 0 0 0 -128 42 0 ]
         }
      :resno 1 
      :resname d 
      :ressortgroupref 0 
      :resorigtbl 0 
      :resorigcol 0 
      :resjunk false
      }
      {TARGETENTRY 
      :expr 
         {CONST 
         :consttype 25 
         :consttypmod -1 
         :constcollid 100 
         :constlen -1 
         :constbyval false 
         :constisnull true 
         :location -1 
         :constvalue <>
         }
      :resno 2 
      :resname b 
      :ressortgroupref 0 
      :resorigtbl 0 
      :resorigcol 0 
      :resjunk false
      }
      {TARGETENTRY 
      :expr 
         {CONST 
         :consttype 23 
         :consttypmod -1 
         :constcollid 0 
         :constlen 4 
         :constbyval true 
         :constisnull false 
         :location 44 
         :constvalue 4 [ 42 0 0 0 0 0 0 0 ]
         }
      :resno 3 
      :resname a 
      :ressortgroupref 0 
      :resorigtbl 0 
      :resorigcol 0 
      :resjunk false
      }
      {TARGETENTRY 
      :expr 
         {CONST 
         :consttype 1700 
         :consttypmod -1 
         :constcollid 0 
         :constlen -1 
         :constbyval false 
         :constisnull true 
         :location -1 
         :constvalue <>
         }
      :resno 4 
      :resname d 
      :ressortgroupref 0 
      :resorigtbl 0 
      :resorigcol 0 
      :resjunk false
      }

Sure enough, using expand_targetlist is clearly wrong here, it's creating references to cols the base view might not even have, and it expects sorted input that doesn't exist. Expansion will happen later anyway.

RETURNING, ERROR: attribute 2 has wrong type (FIXED)

=# INSERT INTO v2 (d, a) VALUES (NUMERIC '42', 44) RETURNING d;
ERROR:  attribute 2 has wrong type
DETAIL:  Table has type text, but query expects numeric.

Problem is that ChangeVarNodes isn't remapping the varattnos when it changes the varno. Need to remap the varattno too.

Looks like map_variable_attnos is what we want for this.

Fixed by by @8276e14ddbbb36feb02ab3aa5d6880241679f23f@. While @map_variable_attnos@ + @ChangeVarNodes@ can be used for this, as can @replace_rte_variables@, it looks to be much better to generate a @List@ of @TargetEntry@ for the remapping, then invoke @ReplaceVarsFromTargetList@ to apply it.

WORKS!

Crash on UPDATE, or no relation entry for relid 2

CREATE TABLE t1 (a integer, b text, c float8, d numeric);
CREATE VIEW v1same AS SELECT * FROM t1;
CREATE VIEW v2same AS SELECT * FROM t2;

UPDATE v2same SET c = 32;
ERROR:  no relation entry for relid 2

UPDATE v1same SET c = 32;
... crash

Trace for the crash is:

Program received signal SIGABRT, Aborted.
0x0000003f02c359e9 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
56        return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
(gdb) bt
#0  0x0000003f02c359e9 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x0000003f02c370f8 in __GI_abort () at abort.c:90
#2  0x0000000000749ebd in ExceptionalCondition (conditionName=conditionName@entry=0x88c271 "!(var->varno == varno)", errorType=errorType@entry=0x77f7bc "FailedAssertion", 
    fileName=fileName@entry=0x88c266 "execScan.c", lineNumber=lineNumber@entry=287) at assert.c:54
#3  0x00000000005a56fc in tlist_matches_tupdesc (ps=ps@entry=0x152cdc0, tlist=<optimized out>, varno=<optimized out>, tupdesc=0x7f283c318cd8) at execScan.c:287
#4  0x00000000005a5aec in ExecAssignScanProjectionInfo (node=node@entry=0x152cdc0) at execScan.c:257
#5  0x00000000005b71e2 in ExecInitSeqScan (node=0x14c9ce0, estate=0x152c9e8, eflags=0) at nodeSeqscan.c:209
#6  0x000000000059e515 in ExecInitNode (node=node@entry=0x14c9ce0, estate=estate@entry=0x152c9e8, eflags=eflags@entry=0) at execProcnode.c:188
#7  0x00000000005b53dd in ExecInitModifyTable (node=0x151b598, estate=0x152c9e8, eflags=0) at nodeModifyTable.c:1131
#8  0x000000000059e4b5 in ExecInitNode (node=node@entry=0x151b598, estate=estate@entry=0x152c9e8, eflags=eflags@entry=0) at execProcnode.c:155
#9  0x000000000059d2e3 in InitPlan (queryDesc=queryDesc@entry=0x152c5d8, eflags=eflags@entry=0) at execMain.c:887
#10 0x000000000059d4e9 in standard_ExecutorStart (queryDesc=0x152c5d8, eflags=0) at execMain.c:208
#11 0x000000000059d525 in ExecutorStart (queryDesc=queryDesc@entry=0x152c5d8, eflags=eflags@entry=0) at execMain.c:122
#12 0x00000000006844eb in ProcessQuery (plan=plan@entry=0x14ca958, sourceText=0x14c8a48 "UPDATE v1same SET c = 32;", params=0x0, dest=dest@entry=0x14c9e90, 
    completionTag=completionTag@entry=0x7fff0c098cf0 "") at pquery.c:180
#13 0x00000000006846d9 in PortalRunMulti (portal=portal@entry=0x150ad68, isTopLevel=isTopLevel@entry=1 '\001', dest=dest@entry=0x14c9e90, altdest=altdest@entry=0x14c9e90, 
    completionTag=completionTag@entry=0x7fff0c098cf0 "") at pquery.c:1279
#14 0x00000000006853bd in PortalRun (portal=portal@entry=0x150ad68, count=count@entry=9223372036854775807, isTopLevel=isTopLevel@entry=1 '\001', dest=dest@entry=0x14c9e90, 
    altdest=altdest@entry=0x14c9e90, completionTag=completionTag@entry=0x7fff0c098cf0 "") at pquery.c:816
#15 0x0000000000681ceb in exec_simple_query (query_string=query_string@entry=0x14c8a48 "UPDATE v1same SET c = 32;") at postgres.c:1054
#16 0x000000000068373e in PostgresMain (argc=<optimized out>, argv=argv@entry=0x1466bb8, dbname=0x1466a18 "postgres", username=<optimized out>) at postgres.c:4011
#17 0x0000000000638d77 in BackendRun (port=port@entry=0x14846d0) at postmaster.c:4085
#18 0x000000000063a692 in BackendStartup (port=port@entry=0x14846d0) at postmaster.c:3774
#19 0x000000000063a8f6 in ServerLoop () at postmaster.c:1585
#20 0x000000000063ba4e in PostmasterMain (argc=argc@entry=4, argv=argv@entry=0x1463730) at postmaster.c:1240
#21 0x00000000005cec76 in main (argc=4, argv=0x1463730) at main.c:194

(gdb) frame 3
#3  0x00000000005a56fc in tlist_matches_tupdesc (ps=ps@entry=0x152cdc0, tlist=<optimized out>, varno=<optimized out>, tupdesc=0x7f283c318cd8) at execScan.c:287
287                     Assert(var->varno == varno);

(gdb) list
282                             return false;           /* tlist too short */
283                     var = (Var *) ((TargetEntry *) lfirst(tlist_item))->expr;
284                     if (!var || !IsA(var, Var))
285                             return false;           /* tlist item not a Var */
286                     /* if these Asserts fail, planner messed up */
287                     Assert(var->varno == varno);
288                     Assert(var->varlevelsup == 0);
289                     if (var->varattno != attrno)
290                             return false;           /* out of order */
291                     if (att_tup->attisdropped)

so looks like the TupleDesc doesn't match. We're still producing a bogus plan tree, the tlist is wrong.

So why does it work for insert?

Trace for the relid error (2 levels) is:

Breakpoint 1, find_base_rel (root=root@entry=0x1853278, relid=2) at relnode.c:210
210             elog(ERROR, "no relation entry for relid %d", relid);
(gdb) bt
#0  find_base_rel (root=root@entry=0x1853278, relid=2) at relnode.c:210
#1  0x000000000060bf0f in add_vars_to_targetlist (root=root@entry=0x1853278, vars=vars@entry=0x1866040, where_needed=0x18661e0, create_new_ph=create_new_ph@entry=1 '\001')
    at initsplan.c:187
#2  0x000000000060c07c in build_base_rel_tlists (root=root@entry=0x1853278, final_tlist=final_tlist@entry=0x1865680) at initsplan.c:153
#3  0x000000000060e386 in query_planner (root=root@entry=0x1853278, tlist=tlist@entry=0x1865680, qp_callback=qp_callback@entry=0x60f2b6 <standard_qp_callback>, 
    qp_extra=qp_extra@entry=0x7fff4f050b60) at planmain.c:154
#4  0x00000000006105c7 in grouping_planner (root=root@entry=0x1853278, tuple_fraction=tuple_fraction@entry=0) at planner.c:1243
#5  0x0000000000611c54 in subquery_planner (glob=glob@entry=0x1812948, parse=parse@entry=0x18117d8, parent_root=parent_root@entry=0x0, 
    hasRecursion=hasRecursion@entry=0 '\000', tuple_fraction=0, subroot=subroot@entry=0x7fff4f050cb8) at planner.c:572
#6  0x0000000000611e5b in standard_planner (parse=0x18117d8, cursorOptions=0, boundParams=0x0) at planner.c:210
#7  0x0000000000612065 in planner (parse=parse@entry=0x18117d8, cursorOptions=cursorOptions@entry=0, boundParams=boundParams@entry=0x0) at planner.c:139
#8  0x000000000068190b in pg_plan_query (querytree=0x18117d8, cursorOptions=cursorOptions@entry=0, boundParams=boundParams@entry=0x0) at postgres.c:759
#9  0x0000000000681993 in pg_plan_queries (querytrees=<optimized out>, cursorOptions=cursorOptions@entry=0, boundParams=boundParams@entry=0x0) at postgres.c:818
#10 0x0000000000681bf2 in exec_simple_query (query_string=query_string@entry=0x1810a48 "UPDATE v2same SET c = 32;") at postgres.c:983
#11 0x000000000068373e in PostgresMain (argc=<optimized out>, argv=argv@entry=0x17ae368, dbname=0x17ae1c8 "postgres", username=<optimized out>) at postgres.c:4011
#12 0x0000000000638d77 in BackendRun (port=port@entry=0x17cc6d0) at postmaster.c:4085
#13 0x000000000063a692 in BackendStartup (port=port@entry=0x17cc6d0) at postmaster.c:3774
#14 0x000000000063a8f6 in ServerLoop () at postmaster.c:1585
#15 0x000000000063ba4e in PostmasterMain (argc=argc@entry=4, argv=argv@entry=0x17ab730) at postmaster.c:1240
#16 0x00000000005cec76 in main (argc=4, argv=0x17ab730) at main.c:194


Looks like the issue is that we're doing a tuple projection, but the tlist contains an expanded whole-col expression exploded into a ROW(...) expression. This points to another instance of t1; there are two in the tlist after flattening.

Looks like what we might have is the planner flattening the subquery out, plus (maybe) expand_targetlist injecting tlist entries for the update tlist that refer to the scan var not the target rel. Everything should refer to the baserel not the resultrel, consistently, except the returning list which must refer only to the resultrel.

At expand_targetlist things are already somewhat off the rails. For UPDATE v1same SET c = 32;:

(
   {TARGETENTRY 
   :expr 
      {CONST 
      :consttype 701 
      :consttypmod -1 
      :constcollid 0 
      :constlen 8 
      :constbyval true 
      :constisnull false 
      :location -1 
      :constvalue 8 [ 0 0 0 0 0 0 64 64 ]
      }
   :resno 3 
   :resname c 
   :ressortgroupref 0 
   :resorigtbl 0 
   :resorigcol 0 
   :resjunk false
   }
   {TARGETENTRY 
   :expr 
      {ROW 
      :args (
         {VAR 
         :varno 5 
         :varattno 1 
         :vartype 23 
         :vartypmod -1 
         :varcollid 0 
         :varlevelsup 0 
         :varnoold 5 
         :varoattno 1 
         :location -1
         }
         {VAR 
         :varno 5 
         :varattno 2 
         :vartype 25 
         :vartypmod -1 
         :varcollid 100 
         :varlevelsup 0 
         :varnoold 5 
         :varoattno 2 
         :location -1
         }
         {VAR 
         :varno 5 
         :varattno 3 
         :vartype 701 
         :vartypmod -1 
         :varcollid 0 
         :varlevelsup 0 
         :varnoold 5 
         :varoattno 3 
         :location -1
         }
         {VAR 
         :varno 5 
         :varattno 4 
         :vartype 1700 
         :vartypmod -1 
         :varcollid 0 
         :varlevelsup 0 
         :varnoold 5 
         :varoattno 4 
         :location -1
         }
      )
      :row_typeid 24614 
      :row_format 2 
      :colnames ("a" "b" "c" "d")
      :location -1
      }
   :resno 5 
   :resname wholerow 
   :ressortgroupref 0 
   :resorigtbl 0 
   :resorigcol 0 
   :resjunk true
   }
   {TARGETENTRY 
   :expr 
      {VAR 
      :varno 2 
      :varattno -1 
      :vartype 27 
      :vartypmod -1 
      :varcollid 0 
      :varlevelsup 0 
      :varnoold 2 
      :varoattno -1 
      :location -1
      }
   :resno 3 
   :resname ctid 
   :ressortgroupref 0 
   :resorigtbl 0 
   :resorigcol 0 
   :resjunk true
   }
)

Note that the ctid resjunk col should refer to varno 5, not 2, since that's the Var we will actually be scanning. So we need to determine where that ctid came from, it wasn't in our original tlist.

It'd also be good to get rid of that unnecessary expanded whole-row var. What circumstances do we actually need it for? (See view expansion code). It forces an extra step we'd like to avoid during updates, tuple projection.

Looks like the whole-row Var is added by rewriteTargetListUD. Before then the tlist is:

(
   {TARGETENTRY 
   :expr 
      {FUNCEXPR 
      :funcid 316 
      :funcresulttype 701 
      :funcretset false 
      :funcvariadic false 
      :funcformat 2 
      :funccollid 0 
      :inputcollid 0 
      :args (
         {CONST 
         :consttype 23 
         :consttypmod -1 
         :constcollid 0 
         :constlen 4 
         :constbyval true 
         :constisnull false 
         :location 22 
         :constvalue 4 [ 32 0 0 0 0 0 0 0 ]
         }
      )
      :location -1
      }
   :resno 3 
   :resname c 
   :ressortgroupref 0 
   :resorigtbl 0 
   :resorigcol 0 
   :resjunk false
   }
)

after, the extre tle added is:

   {TARGETENTRY 
   :expr 
      {VAR 
      :varno 1 
      :varattno 0 
      :vartype 24614 
      :vartypmod -1 
      :varcollid 0 
      :varlevelsup 0 
      :varnoold 1 
      :varoattno 0 
      :location -1
      }
   :resno 2 
   :resname wholerow 
   :ressortgroupref 0 
   :resorigtbl 0 
   :resorigcol 0 
   :resjunk true
   }

This doesn't seem to be the origin of the ctid var in this pass. However, when execution continues we break in rewriteTargetListUD again. This time we add the ctid, since we're processing the base relation. Before/after are:

(
   {TARGETENTRY 
   :expr 
      {FUNCEXPR 
      :funcid 316 
      :funcresulttype 701 
      :funcretset false 
      :funcvariadic false 
      :funcformat 2 
      :funccollid 0 
      :inputcollid 0 
      :args (
         {CONST 
         :consttype 23 
         :consttypmod -1 
         :constcollid 0 
         :constlen 4 
         :constbyval true 
         :constisnull false 
         :location 22 
         :constvalue 4 [ 32 0 0 0 0 0 0 0 ]
         }
      )
      :location -1
      }
   :resno 3 
   :resname c 
   :ressortgroupref 0 
   :resorigtbl 0 
   :resorigcol 0 
   :resjunk false
   }
   {TARGETENTRY 
   :expr 
      {VAR 
      :varno 1 
      :varattno 0 
      :vartype 24614 
      :vartypmod -1 
      :varcollid 0 
      :varlevelsup 0 
      :varnoold 1 
      :varoattno 0 
      :location -1
      }
   :resno 5 
   :resname wholerow 
   :ressortgroupref 0 
   :resorigtbl 0 
   :resorigcol 0 
   :resjunk true
   }
)


after:

(
   {TARGETENTRY 
   :expr 
      {FUNCEXPR 
      :funcid 316 
      :funcresulttype 701 
      :funcretset false 
      :funcvariadic false 
      :funcformat 2 
      :funccollid 0 
      :inputcollid 0 
      :args (
         {CONST 
         :consttype 23 
         :consttypmod -1 
         :constcollid 0 
         :constlen 4 
         :constbyval true 
         :constisnull false 
         :location 22 
         :constvalue 4 [ 32 0 0 0 0 0 0 0 ]
         }
      )
      :location -1
      }
   :resno 3 
   :resname c 
   :ressortgroupref 0 
   :resorigtbl 0 
   :resorigcol 0 
   :resjunk false
   }
   {TARGETENTRY 
   :expr 
      {VAR 
      :varno 1 
      :varattno 0 
      :vartype 24614 
      :vartypmod -1 
      :varcollid 0 
      :varlevelsup 0 
      :varnoold 1 
      :varoattno 0 
      :location -1
      }
   :resno 5 
   :resname wholerow 
   :ressortgroupref 0 
   :resorigtbl 0 
   :resorigcol 0 
   :resjunk true
   }
   {TARGETENTRY 
   :expr 
      {VAR 
      :varno 2 
      :varattno -1 
      :vartype 27 
      :vartypmod -1 
      :varcollid 0 
      :varlevelsup 0 
      :varnoold 2 
      :varoattno -1 
      :location -1
      }
   :resno 3 
   :resname ctid 
   :ressortgroupref 0 
   :resorigtbl 0 
   :resorigcol 0 
   :resjunk true
   }
)

That whole-row var then gets expanded later. At this point we have one problem already: The ctid we need is not that of the resultRelation at all, it's the ctid of the relation we're scanning. These have been the same relation until updatable subqueries, but are no longer; now target_relation->rd_rel isn't necessarily the same as parsetree->resultRelation.

How do we teach rewriteTargetListUD that? How do we get it the RTI of the target_rte?

It's more complicated than that. The problem is the recursive rewrite phase. Each TLE gets added to point to the current resultRelation in that rewrite phase. So the first TLE is a whole-row Var pointing to the view RTE. The second TLE (2nd pass) is a ctid Var pointing to the view's base table RTE that was created in the first pass.

Problems:

  • All TLEs for an UPDATE must point to the same Var otherwise tlist_matches_tupdesc gets upset
  • TLEs must point to the Var we are going to scan; when we're updating a subquery or non-flattened view that isn't the resultRelation.

The 2nd problem looks like another case of possibly needing to separate source and target relation.

Solving issues one at a time, all the tlist must contain is a ref to the ctid of the base relation we'll be modifying. This is only required for simply updatable views, for which the whole-row Var is useless because it doesn't carry the ctid anyway. For trigger-updatable views we need the whole-row Var and *not* the ctid.

I think we need to step back and figure out how it works for DELETE at all, since it must obtain the correct ctid somehow.


(TODO)

Views with expressions over baserel in tlist (FIXED)

If we have a view like:

SELECT a + 32 AS x,
       b
FROM t;

and we:

UPDATE t
SET b = 33
RETURNING x, b

... and if there's a BEFORE trigger that causes a to change, does the current upd. views code return the old or new value for a + 32 for x?

How do we get that behaviour in the new code?

Answer: Current code doesn't permit this, so we don't care.

Crash with subsets of columns

INSERT crashes when we operate on a subset of cols, eg for table t (a, b, c) if we have a view v (a, c) it'll crash when we insert into the view.

Cause not yet known.

How does DELETE work?

Right now we can DELETE from a view and it works. I have no idea why.

In the delete case at rewrite we have the view subquery, with no injected TLEs. In the outer delete query we have a jointree fromlist that refers to one element, the expanded s.b. view. The outer tlist has two entries, one wholerow with varno for the view, one ctid with the varno for the result relation.

At the plan phase we have a subquery scan with an odd tlist - a wholerow ref to varno 1 (the view), a ctid col with varno 2, and another wholerow with varno 1. The subquery's subplan has a seqscan over id, secret with varno 5 (no other cols) with the % 2 = 0 qualifier. The rangetable is:

  • 1: the outer view rel
  • 2: The view base rel (our result relation)
  • 3: old (unused)
  • 4: new (unused)
  • 5: The view base rel (not result relation)

How the xxx does this query even work? It has a tlist in the outer subplan that refers to stuff we never attempt to access, and wouldn't be able to if we did. The inner tlist of the seqscan doesn't expose the ctid for us to use. So how are we getting the right tuples to delete?

With flattened query

Query: delete from t_even

The seqscan node hits :scanrelid 5, i.e. it uses scanrelid to identify what's being scanned (at the plan level; this doesn't exist in the parse tree, since we haven't decided on a seqscan). How do we decide what the scanrelid is? How do we get a ctid out of the scan when all the tlist specifies is the real cols?

ExecDelete gets called with the tupleid by ExecModifyTable, which is responsible for extracting the ctid or wholerow junk attribute. It does this with ExecGetJunkAttribute for the relation case, which in turn uses slot_getattr. The passed slot->tts_tuple is null, so the HeapTuple looks it up in the fast-path slot->tts_values if populated, which it always is for virtual tuples. In our example, we're fetching attribute 2, which according to print *slot->tts_tupleDescriptor->attrs[1]->data has the name ctid.

Where's that come from? What creates our slot? The ExecProcNode that evaluates the subplan. In the simple case that's a ExecScan call, which does a SeqNext. The tuple is returned by heap_getnext, and since it's a table scan it includes a t_self ctid:

#0  heap_getnext (scan=scan@entry=0x2c3acd0, direction=<optimized out>) at /home/craig/projects/postgresql/src/backend/access/heap/heapam.c:1498

(gdb) print scan->rs_ctup
$102 = {t_len = 39, t_self = {ip_blkid = {bi_hi = 0, bi_lo = 0}, ip_posid = 122}, t_tableOid = 16384, t_data = 0x7f47a82d7d28}

The tuple returned by heap_getnext gets stored into the TupleTableSlot slot by ExecStoreTuple.

At this point the slot tuple is non-null. A qual check is done, then the tuple is projected by ExecProject if required, which it is in this case. This is what's giving us our new TupleTableSlot - the projection of the heap tuple. The projInfo is using ExecTargetList to do the dirty work. In our case it's projecting

  • whole-row with varno 5, resno 1
  • Var "ctid" with varno 2, resno 2
  • Var "ctid1" with varno 5, resno 3

to produce a slot (TupleTableSlot) with null tts_tuple (since it's virtual) and a Datum tts_values.

So what we're doing is:

  • Scanning the table to get a row
  • Projecting that row into a virtual tuple
  • Getting one field, the "ctid", out of that virtual tuple and ignoring the rest
  • Unwrapping it into an ItemPointer using DatumGetPointer and copying it
  • Passing that to ExecDelete

The tuple we fetch is the attribute identified by junkfilter->jf_junkAttNo, i.e. attribute 2 ctid, which is something we set earlier in ExecInitModifyTable using ExecFindJunkAttribute.

The rest of the projection seems to be unused for deletes.

Anomaly 1: We're fetching ctid for varno 2, but we're scanning varno 5, and that seems to be acceptable. (Because they're the same relid?).

Anomaly 2: We're generating and totally ignoring the whole-row var.

The Var fetch is done by ExecEvalScalarVarFast via ExecEvalRow via ExecTargetList. This doesn't care what relation is being scanned, it ignores the varno and fetches from the current scan relation.

Anyway, those whole-row fields injected by ...UD appear to be wholly useless if we're doing a simply updatable view. They don't help us with the ctid at all, and the old simply updatable view coe removed them. we can do te same.

With unflattened query

With deeper nested query

vim notes

vim: ctl-T for back to last tag, ctl-] for follow-tag-identifier-to-definition, :tag tagname for jump by name,:ts tagname for tag select list, :stj for split tag jump, :tn and :tp for prev/next tag match. None of these find the caller.

Use cscope, eg :cs find c funcname for find callers.


Open items

  • INSERT with a DEFAULT isn't properly expanded where the col with the DEFAULT is included in the view, eg CREATE VIEW t_even AS SELECT id, secret FROM t WHERE id % 2 = 0, INSERT INTO t_even(secret) VALUES ('sekrit');. True whether explicit DEFAULT or omitted column.
  • 'ERROR: attribute number 3 not found in view targetlist'
  • Crash on subset of columns on insert
  • Need to prove concurrency safety, proper rowmarking, insert FOR UPDATE into nested queries if required
  • WITH CHECK OPTION untested
  • Column- and relation- write permission checking untested
  • Leak resilience untsted
  • UPDATE / DELETE on inherited table untested
  • Set operations untested
  • Need to test with deeply nested views
  • Need to test RETURNING whole-row references, eg RETURNING tablename rather than RETURNING tablename.*.
  • Needs to work with RETURNING expressions containing multiple baserel cols, mix of baserel cols and refs to subquery.

Fixed

  • INSERT or UPDATE into view with different column order now works
  • INSERT with a DEFAULT works where the view has a subset of columns and the DEFAULT col isn't included in the view, eg CREATE VIEW t_even_subset AS SELECT secret FROM t WHERE id % 2 = 0, INSERT INTO t_even_subset (secret) VALUES ('secretive'); will insert the DEFAULT for id.
  • INSERT ... RETURNING is broken (crash)
  • UPDATE ... RETURNING is broken (crash)
  • Should we be using RELOPT_RESULTREL instead of RELOPT_DEADREL?


Docs refs

Personal tools