Automatically updatable security barrier views

From PostgreSQL wiki

(Difference between revisions)
Jump to: navigation, search
(Where does 'ctid1' come from)
Line 110: Line 110:
 
To support updatable security barrier views we have to support <tt>UPDATE</tt> directly on a subquery without flattening the query. That's because we must still enforce the order of qual execution imposed by the <tt>security_barrier</tt> flag on the subquery. Anything else would require implementing a different approach to security barriers and introduce its own problems.
 
To support updatable security barrier views we have to support <tt>UPDATE</tt> directly on a subquery without flattening the query. That's because we must still enforce the order of qual execution imposed by the <tt>security_barrier</tt> 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? ===
+
== Updatable security barrier views via updatable subqueries ==
  
<tt>UPDATE ... FROM</tt> already supports acting on a join, as does <tt>DELETE ... USING</tt>.
+
One approach is to allow the rewriter to expand views that're subquery targets. The rather extensive history on this wiki page covers an attempt to implement that.
  
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:
+
The two fundamental issues that've arisen out of that effort are:
  
<pre>
+
=== resultRelation vs source relation ===
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)
+
</pre>
+
  
<pre>
+
Parts of the parser and executor are used to the resultRelation being the same RTE as the relation we get rows to feed into ExecModifyTable from. If updating a subquery this is no longer true. Instead of, after target view flattening:
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)
+
</pre>
+
  
 
<pre>
 
<pre>
regress=> SET enable_hashjoin = off;
+
      SeqScan<baseRel> WHERE quals1 AND quals2 AND quals3
SET
+
      |
regress=> SET enable_mergejoin = off;
+
      v
SET
+
    ModifyTable
regress=> explain delete from t using t t2 where t.id = t2.id;
+
      -> heap_modify_tuple(baseRel)
                              QUERY PLAN                               
+
      -> RETURNING (mix of refs to baserel and other expressions)
-------------------------------------------------------------------------
+
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)
+
 
</pre>
 
</pre>
  
so it's clearly at least somewhat generic.
+
we get:
 
+
How does this work at the code level, though?
+
  
 
<pre>
 
<pre>
CREATE TABLE t2 AS SELECT * FROM t;
+
    (SELECT ctid, ... FROM (
SET debug_print_plan = on;
+
      (SELECT ctid, ... FROM (
SET debug_print_parse = on;
+
        (SELECT ctid, ... FROM SeqScan<baseRel> WHERE quals1)
SET debug_print_rewritten = on;
+
      WHERE quals2)
SET client_min_messages = debug;
+
    WHERE quals3)
DELETE FROM t USING t2 WHERE t.id = t2.id;
+
    |
 +
    v
 +
  ModifyTable
 +
    -> resultRelation(baserel)
 +
    -> RETURNING (mix of refs to baserel and other expressions)
 
</pre>
 
</pre>
  
produces [https://gist.github.com/ringerc/7413509] - [https://gist.github.com/ringerc/7413509#file-gistfile1-txt-L3 parse], [https://gist.github.com/ringerc/7413509#file-gistfile1-txt-L118 rewrite] and [https://gist.github.com/ringerc/7413509#file-gistfile1-txt-L257 plan] tree set.
+
The <tt>RETURNING</tt> list may contain expressions referencing the result relation, which ''must'' reflect the final rel after the effect of any <tt>BEFORE</tt> triggers etc. Expressions may also contain subqueries, may refer to expressions in the view queries that aren't based on the result relation, etc. So we must rewrite the tlist to replace references to the result relation while leaving everything else untouched.
 
+
The parse tree shows two range-table entries, one for <tt>t</tt> and one for <tt>t2</tt>. They have different <tt>:requiredPerms</tt> but are otherwise the same. The parse-tree has <tt>:commandType 4</tt> (<tt>CMD_DELETE</tt>)
+
 
+
By contrast, in a [https://gist.github.com/ringerc/7413693 simple delete] doesn't differ except for <tt>:requiredPerms 8</tt>.
+
 
+
 
+
[https://gist.github.com/ringerc/7423819 The diff between the two parse, plan and rewrite trees] may be informative.
+
 
+
==== Parse tree ====
+
 
+
* <tt>:requiredPerms</tt> on the first RTE, for <tt>t</tt>, is 8 for the simple delete, 10 for the join delete. The extra bit, 0x02, is defined in <tt>include/nodes/parsenodes.h</tt> as <tt>#define ACL_SELECT (1&lt;&lt;1)</tt>.
+
 
+
* <tt>:selectedCols</tt> on the first RTE has an extra entry <tt>9</tt> in the join delete. This is a resjunk column used to hold the join key?
+
 
+
* <tt>:modifiedCols (b)</tt> is on the first RTE for both parse trees
+
 
+
* There's a second RTE added by the join plan, for <tt>t</tt> aliased to <tt>t2</tt>. 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 ====
+
 
+
[https://gist.github.com/ringerc/7423819#file-gistfile1-diff-L85 diff starts here].
+
 
+
The rewritten tree is the same as the parse tree in both cases; no change.
+
 
+
==== Plan tree ====
+
 
+
[https://gist.github.com/ringerc/7423819#file-gistfile1-diff-L161 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 <tt>QUERY</tt> node the key entry (thanks RhodiumToad) appears:
+
 
+
<pre>
+
  :resultRelation 1
+
</pre>
+
 
+
where <tt>1</tt> is a RTI.
+
 
+
* In both cases the top node is <tt>MODIFYTABLE</tt> with identical parameters, the interesting ones being:
+
 
+
<pre>
+
      :extParam (b)
+
      :allParam (b)
+
      :operation 4
+
      :canSetTag true
+
      :resultRelations (i 1)
+
      :resultRelIndex 0
+
</pre>
+
 
+
IOW it refers to the result relation(s). It's the same for both the simple and join deletes. To focus on are <tt>:resultRelations</tt> and <tt>resultRelIndex</tt>. '''TODO'''
+
 
+
There is a <tt>HASHJOIN</tt> node instead of a <tt>SEQSCAN</tt> node at the first sublevel of the plan.
+
 
+
The <tt>HASHJOIN</tt> node has two <tt>TARGETENTRY</tt> nodes, not just the one for a <tt>SEQSCAN</tt>. The only differences are <tt>:varnoold</tt> 2 instead of 1 for the expr of the 2nd node, <tt>resno</tt> 2 instead of 1, <tt>resname</tt> is <tt>ctid1</tt> instead of <tt>ctid</tt>. Both are <tt>resjunk</tt> columns. Under the <tt>HASHJOIN</tt> 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:
+
 
+
<pre>
+
  :rowMarks (
+
      {PLANROWMARK
+
      :rti 2
+
      :prti 2
+
      :rowmarkId 1
+
      :markType 4
+
      :noWait false
+
      :isParent false
+
      }
+
    :relationOids (o 16387 16402)
+
  )
+
</pre>
+
 
+
Here <tt>:relationOids</tt> are for <tt>t</tt> and <tt>t2</tt> respectively.
+
 
+
The <tt>:rowMarks</tt> clause appears to refer to a <tt>FOR UPDATE</tt> clause. <tt>markType</tt> is defined in <tt>include/nodes/plannodes.h</tt> as <tt>RowMarkType</tt>. <tt>markType</tt> 4 is <tt>ROW_MARK_REFERENCE /* just fetch the TID */</tt>. The comment there says that a ''rowMark'' is added for each ''non-target'' relation and that if it isn't <tt>FOR UPDATE</tt> it's flagged <tt>ROW_MARK_REFERENCE</tt>. So this is the ref to <tt>t2</tt>, which fits given <tt>rti</tt> being the index of range table entry 2, <tt>t2</tt>.
+
 
+
The <tt>rowmarkId</tt> is for <tt>resjunk</tt> cols, referring to the unique resjunk col id <tt>:resno</tt>? If so, it seems to refer to the left side of the join.
+
 
+
=== In the code ===
+
 
+
* <tt>src/backend/executor/README</tt>.
+
* <tt>backend/parser/README</tt>
+
* Target relation of update is <tt>Query->resultRelation</tt>
+
* <tt>UPDATE ... FROM</tt> on a simple table target is acted on in <tt>preprocess_targetlist</tt> and <tt>expand_targetlist</tt> in <tt>backend/optimizer/prep/preptlist.c</tt>.
+
* Updatable views work via:
+
** <tt>INSTEAD OF</tt> '''trigger''', in <tt>rewriteTargetListIU</tt> add whole-view resjunk col to be passed to trigger
+
** For <tt>INSTEAD OF</tt> '''rules''', application of the rule directly substitutes the query in <tt>fireRules</tt>
+
** For automatically updatable views, in <tt>rewriteTargetView</tt>
+
 
+
 
+
Regular views instead work via subquery expansion, then subquery flattening in the optimizer. See [https://gist.github.com/ringerc/7429182 this parse, rewrite, and plan tree].
+
 
+
IRC chat:
+
<pre>
+
<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
+
</pre>
+
 
+
Current limitations:
+
 
+
* in <tt>preprocess_targetlist</tt> an explicit check is made to reject a <tt>result_relation</tt> that is a subquery.
+
* <tt>heap_form_tuple</tt> 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 ===
+
 
+
<tt>RETURNING</tt> 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.
+
+
* <tt>RETURNING target_list</tt> becomes <tt>returning_clause</tt> in <tt>gram.y</tt>
+
 
+
* <tt>returning_clause</tt> isn't a struct and isn't defined directly anywhere; it's part of the structure of <tt>DeleteStmt</tt>, <tt>InsertStmt</tt> and <tt>UpdateStmt</tt> in <tt>gram.y</tt>, where its target list is added to the <tt>n->returningList</tt> of the relevant blahStmt parse output node. The target list is a <tt>List</tt> of <tt>target_el</tt>, which seems to generate a list of <tt>ResTarget</tt> nodes.
+
 
+
* <tt>InsertStmt</tt> etc are defined in <tt>src/include/nodes/parsenodes.h</tt> as <tt>List *returningClause</tt>, and created in <tt>gram.y</tt> by the parser rules.
+
 
+
* <tt>TransformReturningList</tt> in <tt>src/backend/parser/analyze.c</tt> then gets the list and invokes <tt>transformTargetList</tt> with it. The comment on <tt>transformTargetList</tt> notes that it "Turns a list of ResTarget's into a list of TargetEntry's.". This is where wildcard <tt>*</tt> and <tt>blah.*</tt> expansion happens, and conversion from <tt>ResTarget</tt> to <tt>TargetEntry</tt>. Replacement appears to be in-place.
+
 
+
==== In the rewriter ====
+
 
+
We're going to do view expansion here. The <tt>RETURNING</tt> list is in the query's <tt>returningList</tt> as a <tt>List</tt> of <tt>TargetEntry</tt>.
+
 
+
It looks like most of the work on returning lists happens in rules, but also some in <tt>RewriteQuery</tt>, 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 <tt>rewriteRuleAction</tt>.
+
 
+
===== 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 <tt>rewriteRuleAction</tt>). 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 <tt>RETURNING</tt> clause, it's a <tt>SELECT</tt>. The <tt>RETURNING</tt> 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 <tt>expand_targetlist</tt> doing that? Doesn't look like it, because it's invoked before view expansion happens. (Separately, '''<tt>expand_targetlist</tt> 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 <tt>CONST</tt>s. 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 <tt>DEFAULT</tt>, 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 <tt>rewriteHandler.c:3198</tt>.
+
 
+
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 <tt>expand_targetlist</tt> 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 <tt>planner</tt> called from <tt>postgres.c:753</tt>, 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 <tt>resjunk</tt>, referencing varattno 2 (<tt>secret</tt>) of vartype 25 (text). This is referenced by both the modifytable and returning nodes in the outer tlist.
+
 
+
That in turn is produced by <tt>subquery_planner</tt>.
+
 
+
The faulty plan seems to be introduced by <tt>grouping_planner</tt>.
+
 
+
At <tt>planner.c:1512</tt> the <tt>sub_tlist</tt> is set as the plan's targetlist. This <tt>sub_tlist</tt> contains the <tt>Var</tt> that we're concerned about. The <tt>sub_tlist</tt> comes from earlier in <tt>grouping_planner</tt>, at <tt>planner.c:1185</tt>:
+
 
+
<pre>
+
        /*
+
        * 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);
+
 
+
</pre>
+
 
+
Wondering if the real culprit is earlier, though, at:
+
 
+
<pre>
+
        /* Preprocess targetlist */
+
        tlist = preprocess_targetlist(root, tlist);
+
</pre>
+
 
+
Before the call the tlist is:
+
 
+
<pre>
+
(
+
  {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
+
  }
+
)
+
</pre>
+
 
+
and after it is:
+
 
+
<pre>
+
(
+
  {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
+
  }
+
)
+
</pre>
+
 
+
Ha! GOTCHA Y BAST. What's happening is that <tt>preprocess_targetlist</tt> is adding <tt>Var</tt> nodes to the target list of the subquery so that the outer query's <tt>returning</tt> list can reference them. The returning list is:
+
 
+
<pre>
+
  :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
+
      }
+
  )
+
</pre>
+
 
+
It points to <tt>varno 1</tt> but <tt>resultRelation</tt> is <tt>2</tt>. 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.
+
 
+
<tt>pull_var_clause</tt> returns:
+
 
+
<pre>
+
(
+
  {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
+
  }
+
)
+
</pre>
+
 
+
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:
+
 
+
<pre>
+
ERROR:  variable not found in subplan target lists
+
</pre>
+
 
+
... because the returning list gets transformed to refer to these new Vars, not the perfectly good consts already present. So we need to teach <tt>expand_targetlist</tt> 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 <tt>set_plan_refs</tt> in <tt>setrefs.c</tt>, <tt>case T_ModifyTable:</tt>. It takes as input the regular tlist:
+
 
+
<pre>
+
((
+
  {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
+
  }
+
))
+
</pre>
+
 
+
and hands that off to <tt>set_returning_clause_references</tt>. 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 <tt>expand_targetlist</tt> and <tt>set_returning_clause_references</tt> 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 <tt>set_returning_clause_references</tt>.
+
 
+
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 <tt>subquery_planner</tt> is:
+
 
+
<pre>
+
 
+
  {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 <>
+
  }
+
</pre>
+
 
+
and the overall plan including outer from <tt>planner</tt> 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: <tt>info locals</tt>, <tt>list</tt>.
+
 
+
== Glossary ==
+
 
+
What's a var? RangeTable? RTE? Attr? See [[Source Glossary]] and Tom's Intro to hacking the query planner.
+
 
+
== Adding <tt>security_barrier</tt> support ==
+
 
+
=== Doing it cleanly ===
+
 
+
To add <tt>security_barrier</tt> support for updatable views we need to teach <tt>UPDATE</tt> and <tt>DELETE</tt> to operate on a subquery. This [http://www.postgresql.org/message-id/CA+TgmobvM9t8y6+BoG0DXMVA5jcNc9V6fzNt2tNSaVqsRmGpHA@mail.gmail.com would be very useful for row-security].
+
 
+
This isn't unprecedented; we support <tt>UPDATE ... FROM</tt> already:
+
 
+
<pre>
+
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)
+
</pre>
+
 
+
where the root node of the <tt>UPDATE</tt> is a <tt>MergeJoin</tt> not a direct table or index scan.
+
 
+
Can that be extended to a <tt>SubqueryScan</tt>? [http://www.postgresql.org/message-id/CA+TgmoYr1PHw3X9vnVuWDcfXkzK2p_jhtWc0fV2Q58NEgcxyTA@mail.gmail.com Robert thinks it probably can].
+
 
+
=== Prior approaches ===
+
 
+
The 9.4 RLS patch implements an equivalent feature by internally replacing the <tt>RTE_RELATION</tt> range-table entry for the RLS-affected table with an <tt>RTE_SUBQUERY</tt>. It then has to do fixup for <tt>resjunk columns</tt> (temporary sort keys not output in the final result set, <tt>ctid</tt>, 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 <tt>UPDATE</tt> that the relation it is scanning isn't necessarily the same as the relation it is updating.
+
 
+
Dean Rasheed [http://www.postgresql.org/message-id/CAEZATCV+12de7XCFv09oC7Lbb6x1Nwjd_RLAHiH4CKThU72LQA@mail.gmail.com 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 ==
+
 
+
<pre>
+
QUERY
+
* :targetRelation (rti 1)
+
* ModifyTable
+
** SubqueryScan (rti 2)
+
*** SeqScan (ref rti 1)
+
**** Filter
+
* RangeTable
+
** RTE_RELATION underlying target-relation
+
** RTE_SUBQUERY subquery source
+
</pre>
+
 
+
here the target relation is still a RTE_RELATION.
+
 
+
See [http://www.postgresql.org/message-id/5281C4B1.4040209@2ndquadrant.com mailing list post with proposed approach]
+
 
+
Need to:
+
 
+
* Remove security barrier check in <tt>view_query_is_auto_updatable</tt>
+
* Omit/replace/rewrite <tt>rewriteTargetView</tt>
+
** After <tt>new_rt_index = list_length(parsetree->rtable);</tt>, assign new index to <tt>parsetree->resultRelation</tt>
+
** 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 <tt>ModifyRelation</tt> 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 <tt>:resultRelation</tt>. 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 <tt>ApplyRetrieveRule</tt> does the work that should gets done for auto-updatable views in <tt>rewriteTargetView</tt>, 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 <tt>ApplyRetrieveRule</tt> it looks more like we want to enhance the view expansion rewrite code instead. What causes <tt>RewriteQuery</tt> to call <tt>rewriteTargetView</tt> instead of normal view expansion?
+
 
+
Answer appears to be <tt>backend/rewrite/rewriteHandler.c</tt> line 2839. We should use the standard view expansion code on this instead, cons it to the product, then set the "instead" flag, replacing <tt>rewriteTargetView</tt> with the regular view rewriting code. The "view expansion code" is the rules code, since views are internally just <tt>_RETURN</tt> rules. So how do we handle that - we'd be returning a new <tt>SELECT</tt> query? Can we wrap that up into a subquery? Isn't that just what <tt>rewriteTargetView</tt> 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 <tt>rewriteTargetListUD</tt> 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 <tt>rewriteTargetListUD</tt> to inject a reference to <tt>ctid</tt> 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 <tt>SELECT ... FROM ONLY</tt> where appropriate. (Is this correct?)
+
 
+
* Must inject <tt>ctid</tt> 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) ===
+
 
+
* <tt>PostgresMain</tt>
+
** <tt>exec_simple_query</tt>
+
*** <tt>querytree_list = pg_analyze_and_rewrite(parsetree, ...)</tt>
+
*** <tt>plantree_list = pg_plan_queries(querytree_list, ...)</tt>
+
**** <tt>pg_plan_query</tt>
+
***** <tt>planner</tt>
+
****** <tt>standard_planner</tt>
+
******** <tt>subquery_planner</tt>
+
********* <tt>grouping_planner</tt> (from planner.c:572)
+
********** <tt>query_planner</tt> (from planner.c:1236) - generates paths
+
*********** <tt>setup_simple_rel_arrays</tt> (from planmain.c:113)
+
*********** <tt>add_base_rels_to_query</tt> (recursive, then invokes:)
+
************ <tt>build_simple_rel</tt> (at relnode.c:94)
+
*********** <tt>build_base_rel_tlists</tt>
+
************ <tt>add_vars_to_targetlist</tt>
+
************* <tt>find_base_rel</tt>
+
 
+
<tt>find_base_rel</tt> fails because the <tt>root->simple_rel_array</tt> entry for relid 2 doesn't exist.
+
 
+
<tt>root->simple_rte_array</tt> is populated by <tt>setup_simple_rel_arrays</tt>. <tt>setup_simple_rel_arrays</tt> seems to fully populate <tt>root->simple_rte_array</tt>. It initializes <tt>root->simple_rel_array</tt> too, but leaves all entries null.
+
 
+
<tt>root->simple_rel_array</tt> is populated by <tt>build_simple_rel</tt> via <tt>add_base_rels_to_query</tt>.
+
 
+
At the moment <tt>build_simple_rel</tt> only gets called for relid 5.
+
 
+
The lookup is failing in <tt>build_base_rel_tlists</tt> because the <tt>simple_rel_array</tt> entry is null for that <tt>RangeTblEntry</tt>.
+
 
+
It looks like <tt>add_base_rels_to_query</tt> 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:
+
 
+
<pre>
+
    /* 
+
    * 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);
+
 
+
</pre>
+
 
+
after <tt>add_base_rels_to_query</tt> in <tt>query_planner(...)</tt>
+
 
+
=== Problem 2, make_one_rel (SOLVED) ===
+
 
+
Now crashes at <tt>make_one_rel</tt>:
+
 
+
<pre>
+
    /*
+
    * Ready to do the primary planning.
+
    */
+
    final_rel = make_one_rel(root, joinlist);
+
</pre>
+
 
+
Fails at:
+
 
+
<pre>
+
#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));
+
</pre>
+
 
+
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 <tt>RELOPT_DEADREL</tt> 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 <tt>RELOPT_RESULTREL</tt>?
+
 
+
=== Missing DEFAULT ===
+
 
+
<tt>DEFAULT</tt> isn't respected. whatever we're doing with <tt>expand_targetlist</tt> in the view subquery expansion needs to handle default too; perhaps we should be using <tt>preprocess_targetlist</tt> after all?
+
 
+
=== RETURNING crash in <tt>add_rtes_to_flat_rtable</tt> (SOLVED) ===
+
 
+
 
+
* <tt>INSERT ... RETURNING</tt> is broken (crash)
+
* <tt>UPDATE ... RETURNING</tt> is broken (crash)
+
 
+
It crashes at
+
 
+
<pre>
+
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];
+
</pre>
+
 
+
with stack
+
 
+
<pre>
+
(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
+
</pre>
+
 
+
because <tt>root->simple_rel_array</tt> is null.
+
 
+
It gets set at <tt>setup_simple_rel_arrays</tt> in <tt>src/backend/optimizer/util/relnode.c:67</tt>. 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 <tt>query_planner</tt> falls through early if <tt>parse->jointree->fromlist == NIL</tt>. 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 <tt>parse->jointree->fromlist</tt> is truly <tt>NIL</tt>. 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:
+
 
+
<pre>
+
                                * 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.
+
</pre>
+
 
+
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:
+
 
+
<pre>
+
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)
+
</pre>
+
 
+
... yet it seems to work anyway. Um. Seems to be a perms check earlier on.
+
 
+
<pre>
+
#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
+
</pre>
+
 
+
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:
+
 
+
<pre>
+
                * 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)
+
</pre>
+
 
+
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:
+
 
+
<pre>
+
[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 ;-)
+
</pre>
+
 
+
=== RETURNING crash in <tt>slot_getsomeattrs</tt> with null slot (SOLVED) ===
+
 
+
Immediate cause is <tt>econtext->ecxt_scantuple</tt> is NULL, but <tt>projInfo->pi_lastScanVar</tt> is non-zero, causing <tt>slot_getsomeattrs</tt> to be invoked with a null <tt>ecxt_scantuple</tt>.
+
 
+
<pre>
+
(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)
+
</pre>
+
 
+
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 <tt>insert into t_even (id, secret) values (111,'sdf') returning id;</tt> then in <tt>ExecProject</tt> we see:
+
 
+
<pre>
+
(gdb) print *slot->tts_tupleDescriptor
+
$63 = {natts = 3, attrs = 0x10de110, constr = 0x0, tdtypeid = 2249, tdtypmod = -1, tdhasoid = 0 '\000', tdrefcount = -1}
+
</pre>
+
 
+
where we'd only expect natts=1. (Could be something like resjunk tricks?).
+
 
+
The attributes are:
+
 
+
<pre>
+
(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}
+
</pre>
+
 
+
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 <tt>Form_pg_attribute</tt>.
+
 
+
 
+
So, questions:
+
 
+
* ''What exactly does [http://doxygen.postgresql.org/execQual_8c.html#a47a1a54262188f106b6da411a9235323 ExecProject] do''? It's better explained in the comments on [http://doxygen.postgresql.org/structProjectionInfo.html 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 [http://doxygen.postgresql.org/heaptuple_8c.html#a45866935a0fe827e3c677b1528991fa8 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:
+
 
+
<pre>
+
  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)
+
</pre>
+
 
+
(not real helpful if you don't know that code).
+
 
+
* ''What's econtext->ecxt_scantuple''? In [http://doxygen.postgresql.org/structExprContext.html 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 <tt>Form_pg_attribute</tt> in <tt>RETURNING</tt>
+
 
+
* ''What creates and populates the <tt>TupleDesc</tt> for the <tt>TupleTableSlot</tt>?
+
 
+
Path of the null slot:
+
 
+
* <tt>slot</tt> at slot_getsomeattrs
+
* <tt>econtext->ecxt_scantuple</tt> at <tt>ExecProject</tt>; <tt>econtext</tt> is an <tt>ExprContext</tt> assigned from <tt>projInfo->pi_exprContext</tt> earlier in the fn.
+
* <tt>econtext</tt> is <tt>node->ps.ps_ProjInfo</tt> in <tt>ExecResult</tt>; <tt>node</tt> is a <tt>PlanState</tt>
+
* <tt>node</tt> is <tt>PlanState * subplanstate</tt> in <tt>ExecModifyTable</tt>; <tt>subplanstate</tt> is set as <tt>node->mt_plans[node->mt_whichplan];</tt> earlier, where <tt>node</tt> is a <tt>ModifyTableState *</tt>.
+
* <tt>ModifyTableState * node</tt> is passed through from <tt>ExecProcNode</tt>; it's a dispatcher that picks what to run based on node type. It's passed in from <tt>ExecutePlan</tt>, which comes from <tt>queryDesc->planstate</tt> in <tt>ExecutorMain</tt>
+
* <tt>queryDesc</tt> in <tt>ExecutorMain</tt> 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 <tt>insert into t_even(secret) values ('fred') returning secret</tt>. 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:
+
 
+
<pre>
+
#define    INNER_VAR            65000          /* reference to inner subplan */
+
#define    OUTER_VAR            65001          /* reference to outer subplan */
+
#define    INDEX_VAR            65002          /* reference to index column */
+
</pre>
+
 
+
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 <tt>ExecResult</tt>, so we must be processing the <tt>Result</tt> 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 <tt>preprocess_targetlist</tt> sees <tt>Var</tt> tlist entries with <tt>varno</tt> 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:
+
 
+
<pre>
+
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
+
</pre>
+
 
+
=== Complex update (TODO) ===
+
 
+
<pre>
+
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);
+
</pre>
+
 
+
Currently fails due to next problem:
+
 
+
=== Insert via re-ordered view with skipped cols produces <tt>ERROR:  attribute number ... not found in view targetlist</tt> (FIXED) ===
+
 
+
Test case:
+
 
+
<pre>
+
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
+
</pre>
+
 
+
Fired at:
+
 
+
<pre>
+
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
+
</pre>
+
 
+
Suspicious my (ab)use of <tt>expand_targetlist</tt> is at fault here. Seemed too easy to be true.
+
 
+
Before <tt>expand_targetlist</tt>:
+
 
+
<pre>
+
      {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
+
      }
+
</pre>
+
 
+
After:
+
 
+
<pre>
+
      {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
+
      }
+
</pre>
+
 
+
Sure enough, using <tt>expand_targetlist</tt> 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, <tt>ERROR:  attribute 2 has wrong type</tt> (FIXED) ===
+
 
+
<pre>
+
=# 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.
+
</pre>
+
 
+
Problem is that ChangeVarNodes isn't remapping the <tt>varattno</tt>s when it changes the <tt>varno</tt>. Need to remap the varattno too.
+
 
+
Looks like <tt>map_variable_attnos</tt> 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 <tt>no relation entry for relid 2</tt> ===
+
 
+
<pre>
+
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
+
</pre>
+
 
+
Trace for the crash is:
+
 
+
<pre>
+
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)
+
</pre>
+
 
+
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:
+
 
+
<pre>
+
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
+
</pre>
+
 
+
 
+
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) <tt>expand_targetlist</tt> 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 <tt>expand_targetlist</tt> things are already somewhat off the rails. For <tt>UPDATE v1same SET c = 32;</tt>:
+
 
+
<pre>
+
(
+
  {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
+
  }
+
)
+
</pre>
+
 
+
Note that the <tt>ctid</tt> resjunk col should refer to <tt>varno</tt> 5, not 2, since that's the Var we will actually be scanning. So we need to determine where that <tt>ctid</tt> 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 <tt>rewriteTargetListUD</tt>. Before then the tlist is:
+
 
+
<pre>
+
(
+
  {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
+
  }
+
)
+
</pre>
+
 
+
after, the extre tle added is:
+
 
+
<pre>
+
  {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
+
  }
+
</pre>
+
 
+
This doesn't seem to be the origin of the ctid var in this pass. However, when execution continues we break in <tt>rewriteTargetListUD</tt> again. This time we add the ctid, since we're processing the base relation. Before/after are:
+
 
+
<pre>
+
(
+
  {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
+
  }
+
)
+
 
+
</pre>
+
 
+
 
+
after:
+
 
+
<pre>
+
(
+
  {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
+
  }
+
)
+
</pre>
+
 
+
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 <tt>target_relation->rd_rel</tt> isn't necessarily the same as <tt>parsetree->resultRelation</tt>.
+
 
+
How do we teach <tt>rewriteTargetListUD</tt> that? How do we get it the RTI of the <tt>target_rte</tt>?
+
 
+
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:
+
 
+
<pre>
+
SELECT a + 32 AS x,
+
      b
+
FROM t;
+
</pre>
+
 
+
and we:
+
 
+
<pre>
+
UPDATE t
+
SET b = 33
+
RETURNING x, b
+
</pre>
+
 
+
... and if there's a <tt>BEFORE</tt> trigger that causes <tt>a</tt> to change, does the current upd. views code return the old or new value for <tt>a + 32</tt> for <tt>x</tt>?
+
 
+
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 ===
+
 
+
<tt>INSERT</tt> crashes when we operate on a subset of cols, eg for table <tt>t (a, b, c)</tt> if we have a view <tt>v (a, c)</tt> 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: <tt>delete from t_even</tt>
+
 
+
The seqscan node hits <tt>:scanrelid 5</tt>, i.e. it uses <tt>scanrelid</tt> 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?
+
 
+
<tt>ExecDelete</tt> gets called with the <tt>tupleid</tt> by <tt>ExecModifyTable</tt>, which is responsible for extracting the ctid or wholerow junk attribute. It does this with <tt>ExecGetJunkAttribute</tt> for the relation case, which in turn uses <tt>slot_getattr</tt>. The passed <tt>slot->tts_tuple</tt> is null, so the <tt>HeapTuple</tt> looks it up in the fast-path <tt>slot->tts_values</tt> if populated, which it always is for virtual tuples. In our example, we're fetching attribute 2, which according to <tt>print *slot->tts_tupleDescriptor->attrs[1]->data</tt> has the name <tt>ctid</tt>.
+
 
+
Where's that come from? What creates our <tt>slot</tt>? The <tt>ExecProcNode</tt> that evaluates the subplan. In the simple case that's a <tt>ExecScan</tt> call, which does a <tt>SeqNext</tt>. The tuple is returned by <tt>heap_getnext</tt>, and since it's a table scan it includes a <tt>t_self</tt> ctid:
+
 
+
<pre>
+
#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}
+
</pre>
+
 
+
The tuple returned by <tt>heap_getnext</tt> gets stored into the <tt>TupleTableSlot slot</tt> by <tt>ExecStoreTuple</tt>.
+
 
+
At this point the slot tuple is non-null. A qual check is done, then the tuple is projected by <tt>ExecProject</tt> if required, which it is in this case. This is what's giving us our new <tt>TupleTableSlot</tt> - the projection of the heap tuple.  The projInfo is using <tt>ExecTargetList</tt> 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 <tt>tts_values</tt>.
+
 
+
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 <tt>DatumGetPointer</tt> and copying it
+
* Passing that to ExecDelete
+
 
+
The tuple we fetch is the attribute identified by <tt>junkfilter->jf_junkAttNo</tt>, i.e. attribute 2 <tt>ctid</tt>, which is something we set earlier in <tt>ExecInitModifyTable</tt> using <tt>ExecFindJunkAttribute</tt>.
+
 
+
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 <tt>ExecEvalScalarVarFast</tt> via <tt>ExecEvalRow</tt> via <tt>ExecTargetList</tt>. 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 the same.
+
 
+
Except that when we do remove the whole-row var, we get a <tt>tlist_matches_tupdesc</tt> failure, where we complain about mismatching varnos. Why didn't we before? That mismatch was still there.
+
 
+
The following hunk disables a sanity check that actually makes things mostly work (make check still fails, possibly issues with rules?).
+
 
+
<pre>
+
*** a/src/backend/executor/execScan.c
+
--- b/src/backend/executor/execScan.c
+
*************** tlist_matches_tupdesc(PlanState *ps, Lis
+
*** 284,290 ****
+
                if (!var || !IsA(var, Var))
+
                        return false;          /* tlist item not a Var */
+
                /* if these Asserts fail, planner messed up */
+
!              Assert(var->varno == varno);
+
                Assert(var->varlevelsup == 0);
+
                if (var->varattno != attrno)
+
                        return false;          /* out of order */
+
--- 284,291 ----
+
                if (!var || !IsA(var, Var))
+
                        return false;          /* tlist item not a Var */
+
                /* if these Asserts fail, planner messed up */
+
!              /* TODO: We're generating wrong plans at the moment, but it works without this check. */
+
!              /*Assert(var->varno == varno); */
+
                Assert(var->varlevelsup == 0);
+
                if (var->varattno != attrno)
+
                        return false;          /* out of order */
+
</pre>
+
 
+
... but it's dirty, and should not be needed. What we need to do is tell <tt>rewriteTargetListUD</tt> that the source and target rels are different, so it adds a ctid ref to the source rel.
+
 
+
Handy reference: RLS patch https://github.com/ringerc/postgres/commit/a134c701a058062d4ad37f34e87d4edb8993f6a6
+
 
+
==== With unflattened query ====
+
 
+
==== With deeper nested query ====
+
 
+
=== vim notes ===
+
 
+
vim: <tt>ctl-T</tt> for back to last tag, <tt>ctl-]</tt> for follow-tag-identifier-to-definition, <tt>:tag tagname</tt> for jump by name,<tt>:ts tagname</tt> for tag select list, <tt>:stj</tt> for split tag jump, <tt>:tn</tt> and <tt>:tp</tt> for prev/next tag match. None of these find the caller.
+
 
+
Use <tt>cscope</tt>, eg <tt>:cs find c funcname</tt> for find callers.
+
 
+
 
+
=== How to get the ctid ===
+
 
+
Simon had an interesting suggestion. Always have subqueries that're subjects of DML and are simply updatable return the ctid. So it doesn't have to be in the tlist, it's always stored in the <tt>TupleTableSlot</tt> the projection info says so. If it does a valid ctid must be available from the underlying heap tuple or if the underlying virtual tuple; absence is an Assert worthy error.
+
 
+
This avoids the need to inject tlist entries from outer inward during rewrite, or to try to walk backward from the inner baserel adding them post-rewrite.
+
 
+
Need to remember when we identify a view as simply updatable and transfer that into a subquery flag marking it as needing ctid in projections, then make the flag visible to <tt>ExecProject</tt> via <tt>ProjectionInfo</tt> or a submember. Do so without API changes if possible.
+
 
+
Investigate.
+
 
+
=== How to expand DEFAULT ===
+
 
+
....
+
 
+
This might be a related problem to the ctid walking. Study it before committing to ctid by subqyery.
+
 
+
 
+
=== Where does 'ctid1' come from ===
+
 
+
Comes from rowmarks, injected in optimizer/executor.
+
 
+
<pre>
+
src/backend/executor/execMain.c:1801:
+
snprintf(resname, sizeof(resname), "ctid%u", erm->rowmarkId);
+
 
+
src/backend/optimizer/prep/preptlist.c:101:
+
snprintf(resname, sizeof(resname), "ctid%u", rc->rowmarkId);
+
</pre>
+
 
+
=== Open items ===
+
 
+
* <tt>INSERT</tt> with a <tt>DEFAULT</tt> isn't properly expanded where the col with the <tt>DEFAULT</tt> ''is'' included in the view, eg <tt>CREATE VIEW t_even AS SELECT id, secret FROM t WHERE id % 2 = 0</tt>, <tt>INSERT INTO t_even(secret) VALUES ('sekrit');</tt>. True whether explicit <tt>DEFAULT</tt> or omitted column.
+
* <tt>'ERROR:  attribute number 3 not found in view targetlist'</tt>
+
* Crash on subset of columns on insert
+
* Need to prove concurrency safety, proper rowmarking, insert <tt>FOR UPDATE</tt> into nested queries if required
+
* <tt>WITH CHECK OPTION</tt> 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 <tt>RETURNING tablename</tt> rather than <tt>RETURNING tablename.*</tt>.
+
* Needs to work with RETURNING expressions containing multiple baserel cols, mix of baserel cols and refs to subquery.
+
 
+
=== Fixed ===
+
 
+
* <tt>INSERT</tt> or <tt>UPDATE</tt> into view with different column order now works
+
* <tt>INSERT</tt> with a <tt>DEFAULT</tt> works where the view has a subset of columns and the <tt>DEFAULT</tt> col isn't included in the view, eg <tt>CREATE VIEW t_even_subset AS SELECT secret FROM t WHERE id % 2 = 0</tt>, <tt>INSERT INTO t_even_subset (secret) VALUES ('secretive');</tt> will insert the <tt>DEFAULT</tt> for <tt>id</tt>.
+
* <tt>INSERT ... RETURNING</tt> is broken (crash)
+
* <tt>UPDATE ... RETURNING</tt> is broken (crash)
+
 
+
* Should we be using <tt>RELOPT_RESULTREL</tt> instead of <tt>RELOPT_DEADREL</tt>?
+
 
+
 
+
  
 
== Docs refs ==
 
== Docs refs ==

Revision as of 04:05, 13 January 2014

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.

Updatable security barrier views via updatable subqueries

One approach is to allow the rewriter to expand views that're subquery targets. The rather extensive history on this wiki page covers an attempt to implement that.

The two fundamental issues that've arisen out of that effort are:

resultRelation vs source relation

Parts of the parser and executor are used to the resultRelation being the same RTE as the relation we get rows to feed into ExecModifyTable from. If updating a subquery this is no longer true. Instead of, after target view flattening:

      SeqScan<baseRel> WHERE quals1 AND quals2 AND quals3
      |
      v
    ModifyTable
      -> heap_modify_tuple(baseRel)
      -> RETURNING (mix of refs to baserel and other expressions)

we get:

    (SELECT ctid, ... FROM (
      (SELECT ctid, ... FROM (
        (SELECT ctid, ... FROM SeqScan<baseRel> WHERE quals1)
      WHERE quals2) 
    WHERE quals3)
    |
    v
  ModifyTable
    -> resultRelation(baserel)
    -> RETURNING (mix of refs to baserel and other expressions)

The RETURNING list may contain expressions referencing the result relation, which must reflect the final rel after the effect of any BEFORE triggers etc. Expressions may also contain subqueries, may refer to expressions in the view queries that aren't based on the result relation, etc. So we must rewrite the tlist to replace references to the result relation while leaving everything else untouched.

Docs refs

Personal tools