(2014/01/21 18:18), Dean Rasheed wrote:
On 21 January 2014 01:09, KaiGai Kohei <kai...@ak.jp.nec.com> wrote:
(2014/01/13 22:53), Craig Ringer wrote:
On 01/09/2014 11:19 PM, Tom Lane wrote:
Dean Rasheed <dean.a.rash...@gmail.com> writes:
My first thought was that it should just preprocess any security
barrier quals in subquery_planner() in the same way as other quals are
preprocessed. But thinking about it further, those quals are destined
to become the quals of subqueries in the range table, so we don't
actually want to preprocess them at that stage --- that will happen
later when the new subquery is planned by recursion back into
subquery_planner(). So I think the right answer is to make
adjust_appendrel_attrs() handle recursion into sublink subqueries.
TBH, this sounds like doubling down on a wrong design choice. I see
no good reason that updatable security views should require any
fundamental rearrangements of the order of operations in the planner.
In that case, would you mind offerign a quick sanity check on the
following alternative idea:
- Add "sourceRelation" to Query. This refers to the RTE that supplies
tuple projections to feed into ExecModifyTable, with appropriate resjunk
"ctid" and (if requ'd) "oid" cols present.
- When expanding a target view into a subquery, set "sourceRelation" on
the outer view to the index of the RTE of the newly expanded subquery.
I have sane opinion. Existing assumption, "resultRelation" is RTE of the
table to be read not only modified, makes the implementation complicated.
If we would have a separate "sourceRelation", it allows to have flexible
form including sub-query with security_barrier on the reader side.
Could you tell me the direction you're inclined right now?
I wonder whether I should take the latest patch submitted on 09-Jan for
a deep code reviewing and testing.
Yes, please review the patch from 09-Jan
(http://www.postgresql.org/message-id/CAEZATCUiKxOg=voovja2s6g-sixzzxg18totggp8zobq6qn...@mail.gmail.com).
The idea behind that patch is to avoid a lot of the complication that
leads to and then arises from having a separate "sourceRelation" in
the query.
If you go down the route of expanding the subquery in the rewriter and
making it the "sourceRelation", then you have to make extensive
changes to preprocess_targetlist so that it recursively descends into
the subquery to pull out variables needed by an update.
Also you would probably need additional changes in the rewriter so
that later stages didn't trample on the "sourceRelation", and
correctly updated it in the case of views on top of other views.
Also, you would need to make changes to the inheritance planner code,
because you'd now have 2 RTEs referring to the target relation
("resultRelation" and "sourceRelation" wrapping it in a subquery).
Referring to the comment in inheritance_planner():
* Source inheritance is expanded at the bottom of the
* plan tree (see allpaths.c), but target inheritance has to be expanded at
* the top.
except that in the case of the "sourceRelation", it is actually
effectively the target, which means it shouldn't be expanded,
otherwise you'd get plans like the ones I complained about upthread
(see the final explain output in
http://www.postgresql.org/message-id/caezatcu3vcdkjpnhgfkrmrkz0axkcuh4ce_kqq3z2hzknhi...@mail.gmail.com).
Perhaps all of that could be made to work, but I think that it would
end up being a far more invasive patch than my 09-Jan patch. My patch
avoids both those issues by not doing the subquery expansion until
after inheritance expansion, and after targetlist preprocessing.
Probably, I could get your point.
Even though the sub-query being replaced from a relation with
securityQuals is eventually planned according to the usual
manner, usual order as a part of regular sub-query planning,
however, adjust_appendrel_attrs() is called by inheritance_planner()
earlier than sub-query's planning.
Maybe, we originally had this problem but not appeared on the
surface, because child relations don't have qualifiers on this
phase. (It shall be distributed later.)
But now, this assumption was broken because of a relation with
securityQuals being replaced by a sub-query with SubLink.
So, I'm inclined to revise the assumption side, rather than
existing assertion checks.
Shall we investigate what assumption should be revised if child-
relation, being expanded at expand_inherited_tables(), would be
a sub-query having Sub-Link?
A minor comment is below:
! /*
! * Make sure that the query is marked correctly if the added qual
! * has sublinks.
! */
! if (!parsetree->hasSubLinks)
! parsetree->hasSubLinks = checkExprHasSubLink(viewqual);
Is this logic really needed? This flag says the top-level query
contains SubLinks, however, the above condition checks whether
a sub-query to be constructed shall contain SubLinks.
Also, securityQuals is not attached to the parse tree right now.
Thanks,
--
OSS Promotion Center / The PG-Strom Project
KaiGai Kohei <kai...@ak.jp.nec.com>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
