Thread

  1. [v9.2] Fix Leaky View Problem

    Kohei Kaigai <kohei.kaigai@emea.nec.com> — 2011-08-24T12:38:12Z

    Hi,
    
    The attached patches try to tackle our long-standing leaky view problem, and were revised according to the discussion we had in the commit-fest 1st.
    
    We already knew two scenarios to leak contents of invisible tuples using functions with side-effects; such as error messages containing its arguments.
    
    The first one was the order of execution of qualifiers within a scan plan. Query optimizer shall pull-up simple sub-queries into inner-join due to the performance gain, however, it possibly cause a problem that functions supplied at outside of the sub-query is launched earlier than functions come from inside of the sub-query; depending on the cost estimation. In the result, it allows users to reference contents of invisible tuples (to be filtered by view), if they provide a function with side-effects as a part of WHERE clause.
    
    The second one was the distribution policy of qualifiers. In the case when a view (that intends row-level security) contains JOIN clause, we hope the qualifiers supplied from outside of the view to be launched after the table join, because the view may filter out some of tuples during checks of its join condition. However, the current query optimizer will distribute a qualifier that reference only one-side of the join into inside of the join-loop to minimize number of tuples to be joined. In the result, it also allows users to reference contents of invisible tuples.
    
    In the commit-fest 1st, we made a consensus that a part of views should perform as "security barrier" that enables to prevent unexpected push-down and execution order of qualifiers; being marked by creator of the view.
    And, we also made a consensus obviously secure functions should be allowed to push-down across security barrier; to minimize unnecessary performance damages.
    
    The part-1 patch implements SQL enhancement stuffs; (1) add reloption support on RELKIND_VIEW with "security_barrier" bool variable (2) add pg_proc.proleaky flag to show whether the function is possibly leaky, or not.
    The (2) is new stuff from the revision in commit-fest 1st. It enables to supply "NOLEAKY" option on CREATE FUNCTION statement, then the function is allowed to distribute across security barrier. Only superuser can set this option.
    
      Example)
        CREATE FUNCTION safe_func(text) RETURNS bool
            LANGUAGE 'C' NOLEAKY AS '$libdir/safe_lib', 'safe_func';
                         ^^^^^^^
    A patch to add a new field into pg_proc always takes a large chunk, so the attached proctrans.php is the script I used to append a new field to the existing functions. Right now, I marked it true (= possibly leaky), because we need to have a discussion what shall be none-leaky functions in the default.
    
    The part-2 patch is same as we had discussed in the commit fest. Here is not updates except for rebasing to the latest tree. It enables to remember the nest level of the qualifier being originally used, and utilize it to sort order of the qualifiers.
    
    The part-3 patch was a bit revised, although its basic idea has not been changed.
    It enables to prevent qualifiers come from outside of security barrier being pushed down into inside of the security barrier, even if it references only a part of relations within the sub-query expanded from a view with "security_barrier" flag.
    
    Thanks,
    --
    NEC Europe Ltd, SAP Global Competence Center
    KaiGai Kohei <kohei.kaigai@emea.nec.com>
    
  2. Re: [v9.2] Fix Leaky View Problem

    Thom Brown <thom@linux.com> — 2011-09-07T13:09:15Z

    On 24 August 2011 13:38, Kohei Kaigai <Kohei.Kaigai@emea.nec.com> wrote:
    
    > The (2) is new stuff from the revision in commit-fest 1st. It enables to
    > supply "NOLEAKY" option on CREATE FUNCTION statement, then the function is
    > allowed to distribute across security barrier. Only superuser can set this
    > option.
    >
    
    "NOLEAKY" doesn't really sound appropriate as it sounds like pidgin English.
     Also, it could be read as "Don't allow leaks in this function".  Could we
    instead use something like TRUSTED or something akin to it being allowed to
    do more than safer functions?  It then describes its level of behaviour
    rather than what it promises not to do.
    
    -- 
    Thom Brown
    Twitter: @darkixion
    IRC (freenode): dark_ixion
    Registered Linux user: #516935
    
    EnterpriseDB UK: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
  3. Re: [v9.2] Fix Leaky View Problem

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2011-09-07T13:34:24Z

    2011/9/7 Thom Brown <thom@linux.com>:
    > On 24 August 2011 13:38, Kohei Kaigai <Kohei.Kaigai@emea.nec.com> wrote:
    >>
    >> The (2) is new stuff from the revision in commit-fest 1st. It enables to
    >> supply "NOLEAKY" option on CREATE FUNCTION statement, then the function is
    >> allowed to distribute across security barrier. Only superuser can set this
    >> option.
    >
    > "NOLEAKY" doesn't really sound appropriate as it sounds like pidgin English.
    >  Also, it could be read as "Don't allow leaks in this function".  Could we
    > instead use something like TRUSTED or something akin to it being allowed to
    > do more than safer functions?  It then describes its level of behaviour
    > rather than what it promises not to do.
    >
    Thanks for your comment. I'm not a native English specker, so it is helpful.
    
    "TRUSTED" sounds meaningful for me, however, it is confusable with a concept
    of "trusted procedure" in label-based MAC. It is not only SELinux,
    Oracle's label
    based security also uses this term to mean a procedure that switches user's
    credential during its execution.
      http://download.oracle.com/docs/cd/B28359_01/network.111/b28529/storproc.htm
    
    So, how about "CREDIBLE", instead of "TRUSTED"?
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    
  4. Re: [v9.2] Fix Leaky View Problem

    Thom Brown <thom@linux.com> — 2011-09-07T13:39:11Z

    On 7 September 2011 14:34, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    
    > 2011/9/7 Thom Brown <thom@linux.com>:
    > > On 24 August 2011 13:38, Kohei Kaigai <Kohei.Kaigai@emea.nec.com> wrote:
    > >>
    > >> The (2) is new stuff from the revision in commit-fest 1st. It enables to
    > >> supply "NOLEAKY" option on CREATE FUNCTION statement, then the function
    > is
    > >> allowed to distribute across security barrier. Only superuser can set
    > this
    > >> option.
    > >
    > > "NOLEAKY" doesn't really sound appropriate as it sounds like pidgin
    > English.
    > >  Also, it could be read as "Don't allow leaks in this function".  Could
    > we
    > > instead use something like TRUSTED or something akin to it being allowed
    > to
    > > do more than safer functions?  It then describes its level of behaviour
    > > rather than what it promises not to do.
    > >
    > Thanks for your comment. I'm not a native English specker, so it is
    > helpful.
    >
    > "TRUSTED" sounds meaningful for me, however, it is confusable with a
    > concept
    > of "trusted procedure" in label-based MAC. It is not only SELinux,
    > Oracle's label
    > based security also uses this term to mean a procedure that switches user's
    > credential during its execution.
    >
    > http://download.oracle.com/docs/cd/B28359_01/network.111/b28529/storproc.htm
    >
    > So, how about "CREDIBLE", instead of "TRUSTED"?
    >
    
    I can't say I'm keen on that alternative, but I'm probably not the one to
    participate in bike-shedding here, so I'll leave comment to you hackers. :)
    
    -- 
    Thom Brown
    Twitter: @darkixion
    IRC (freenode): dark_ixion
    Registered Linux user: #516935
    
    EnterpriseDB UK: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
  5. Re: [v9.2] Fix Leaky View Problem

    Robert Haas <robertmhaas@gmail.com> — 2011-09-07T13:58:46Z

    On Wed, Sep 7, 2011 at 9:39 AM, Thom Brown <thom@linux.com> wrote:
    > On 7 September 2011 14:34, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    >> 2011/9/7 Thom Brown <thom@linux.com>:
    >> > On 24 August 2011 13:38, Kohei Kaigai <Kohei.Kaigai@emea.nec.com> wrote:
    >> >>
    >> >> The (2) is new stuff from the revision in commit-fest 1st. It enables
    >> >> to
    >> >> supply "NOLEAKY" option on CREATE FUNCTION statement, then the function
    >> >> is
    >> >> allowed to distribute across security barrier. Only superuser can set
    >> >> this
    >> >> option.
    >> >
    >> > "NOLEAKY" doesn't really sound appropriate as it sounds like pidgin
    >> > English.
    >> >  Also, it could be read as "Don't allow leaks in this function".  Could
    >> > we
    >> > instead use something like TRUSTED or something akin to it being allowed
    >> > to
    >> > do more than safer functions?  It then describes its level of behaviour
    >> > rather than what it promises not to do.
    >> >
    >> Thanks for your comment. I'm not a native English specker, so it is
    >> helpful.
    >>
    >> "TRUSTED" sounds meaningful for me, however, it is confusable with a
    >> concept
    >> of "trusted procedure" in label-based MAC. It is not only SELinux,
    >> Oracle's label
    >> based security also uses this term to mean a procedure that switches
    >> user's
    >> credential during its execution.
    >>
    >>  http://download.oracle.com/docs/cd/B28359_01/network.111/b28529/storproc.htm
    >>
    >> So, how about "CREDIBLE", instead of "TRUSTED"?
    >
    > I can't say I'm keen on that alternative, but I'm probably not the one to
    > participate in bike-shedding here, so I'll leave comment to you hackers. :)
    
    I think TRUSTED actually does a reasonably good job capturing what
    we're after here, although I do share a bit of KaiGai's nervousness
    about terminological confusion.  Still, I'd be inclined to go that way
    if we can't come up with anything better.  CREDIBLE is definitely the
    wrong idea: that means "believable", which sounds more like a
    statement about the function's results than about its side-effects.  I
    thought about TACITURN, since we need the error messages to not be
    excessively informative, but that doesn't do a good job characterizing
    the hazard created by side-effects, or the potential for abuse due to
    - for example - deliberate division by zero.  I also thought about
    PURE, which is a term that's sometimes used to describe code that
    throws no errors and has no side effects, and comes pretty close to
    our actual requirement here, but doesn't necessarily convey that a
    security concern is involved.  Yet another idea would be to use a
    variant of TRUSTED, such as TRUSTWORTHY, just to avoid confusion with
    the idea of a trusted procedure, but I'm not that excited about that
    idea despite have no real specific gripe with it other than length.
    So at the moment I am leaning toward TRUSTED.
    
    Anyone else want to bikeshed?
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
    
  6. Re: [v9.2] Fix Leaky View Problem

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-09-07T14:02:25Z

    Robert Haas <robertmhaas@gmail.com> wrote:
     
    > Anyone else want to bikeshed?
     
    I'm not sure they beat TRUSTED, but:
     
    SECURE
    OPAQUE
     
    -Kevin
    
    
  7. Re: [v9.2] Fix Leaky View Problem

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2011-09-07T14:21:28Z

    2011/9/7 Robert Haas <robertmhaas@gmail.com>:
    > On Wed, Sep 7, 2011 at 9:39 AM, Thom Brown <thom@linux.com> wrote:
    >> On 7 September 2011 14:34, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    >>> 2011/9/7 Thom Brown <thom@linux.com>:
    >>> > On 24 August 2011 13:38, Kohei Kaigai <Kohei.Kaigai@emea.nec.com> wrote:
    >>> >>
    >>> >> The (2) is new stuff from the revision in commit-fest 1st. It enables
    >>> >> to
    >>> >> supply "NOLEAKY" option on CREATE FUNCTION statement, then the function
    >>> >> is
    >>> >> allowed to distribute across security barrier. Only superuser can set
    >>> >> this
    >>> >> option.
    >>> >
    >>> > "NOLEAKY" doesn't really sound appropriate as it sounds like pidgin
    >>> > English.
    >>> >  Also, it could be read as "Don't allow leaks in this function".  Could
    >>> > we
    >>> > instead use something like TRUSTED or something akin to it being allowed
    >>> > to
    >>> > do more than safer functions?  It then describes its level of behaviour
    >>> > rather than what it promises not to do.
    >>> >
    >>> Thanks for your comment. I'm not a native English specker, so it is
    >>> helpful.
    >>>
    >>> "TRUSTED" sounds meaningful for me, however, it is confusable with a
    >>> concept
    >>> of "trusted procedure" in label-based MAC. It is not only SELinux,
    >>> Oracle's label
    >>> based security also uses this term to mean a procedure that switches
    >>> user's
    >>> credential during its execution.
    >>>
    >>>  http://download.oracle.com/docs/cd/B28359_01/network.111/b28529/storproc.htm
    >>>
    >>> So, how about "CREDIBLE", instead of "TRUSTED"?
    >>
    >> I can't say I'm keen on that alternative, but I'm probably not the one to
    >> participate in bike-shedding here, so I'll leave comment to you hackers. :)
    >
    > I think TRUSTED actually does a reasonably good job capturing what
    > we're after here, although I do share a bit of KaiGai's nervousness
    > about terminological confusion.  Still, I'd be inclined to go that way
    > if we can't come up with anything better.  CREDIBLE is definitely the
    > wrong idea: that means "believable", which sounds more like a
    > statement about the function's results than about its side-effects.  I
    > thought about TACITURN, since we need the error messages to not be
    > excessively informative, but that doesn't do a good job characterizing
    > the hazard created by side-effects, or the potential for abuse due to
    > - for example - deliberate division by zero.  I also thought about
    > PURE, which is a term that's sometimes used to describe code that
    > throws no errors and has no side effects, and comes pretty close to
    > our actual requirement here, but doesn't necessarily convey that a
    > security concern is involved.  Yet another idea would be to use a
    > variant of TRUSTED, such as TRUSTWORTHY, just to avoid confusion with
    > the idea of a trusted procedure, but I'm not that excited about that
    > idea despite have no real specific gripe with it other than length.
    > So at the moment I am leaning toward TRUSTED.
    >
    > Anyone else want to bikeshed?
    >
    I also become leaning toward TRUSTED, although we still have a bit risk of
    terminology confusion, because I assume it is quite rare case to set this
    option by DBA and we will able to expect DBAs who try to this option have
    correct knowledge about background of the leaky-view problem.
    
    I'll submit the patch again.
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    
  8. Re: [v9.2] Fix Leaky View Problem

    Yeb Havinga <yebhavinga@gmail.com> — 2011-09-07T14:37:50Z

    On 2011-09-07 16:02, Kevin Grittner wrote:
    > Robert Haas<robertmhaas@gmail.com>  wrote:
    >
    >> Anyone else want to bikeshed?
    >
    > I'm not sure they beat TRUSTED, but:
    >
    > SECURE
    > OPAQUE
    
    SAVE
    
    -- Yeb
    
    
    
  9. Re: [v9.2] Fix Leaky View Problem

    Noah Misch <noah@leadboat.com> — 2011-09-07T15:30:40Z

    On Wed, Sep 07, 2011 at 02:09:15PM +0100, Thom Brown wrote:
    > On 24 August 2011 13:38, Kohei Kaigai <Kohei.Kaigai@emea.nec.com> wrote:
    > 
    > > The (2) is new stuff from the revision in commit-fest 1st. It enables to
    > > supply "NOLEAKY" option on CREATE FUNCTION statement, then the function is
    > > allowed to distribute across security barrier. Only superuser can set this
    > > option.
    > >
    > 
    > "NOLEAKY" doesn't really sound appropriate as it sounds like pidgin English.
    >  Also, it could be read as "Don't allow leaks in this function".  Could we
    > instead use something like TRUSTED or something akin to it being allowed to
    > do more than safer functions?  It then describes its level of behaviour
    > rather than what it promises not to do.
    
    I liked NOLEAKY for its semantics, though I probably would have spelled it
    "LEAKPROOF".  PostgreSQL will trust the function to implement a specific,
    relatively-unintuitive security policy.  We want the function implementers to
    read that policy closely and not rely on any intuition they have about the
    "trusted" term of art.  Our use of TRUSTED in CREATE LANGUAGE is more
    conventional, I think, as is the trusted nature of SECURITY DEFINER.  In that
    vein, folks who actually need SECURITY DEFINER might first look at TRUSTED;
    NOLEAKY would not attract the same unwarranted attention.
    
    
  10. Re: [v9.2] Fix Leaky View Problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-09-07T16:05:40Z

    Noah Misch <noah@leadboat.com> writes:
    > I liked NOLEAKY for its semantics, though I probably would have spelled it
    > "LEAKPROOF".  PostgreSQL will trust the function to implement a specific,
    > relatively-unintuitive security policy.  We want the function implementers to
    > read that policy closely and not rely on any intuition they have about the
    > "trusted" term of art.  Our use of TRUSTED in CREATE LANGUAGE is more
    > conventional, I think, as is the trusted nature of SECURITY DEFINER.  In that
    > vein, folks who actually need SECURITY DEFINER might first look at TRUSTED;
    > NOLEAKY would not attract the same unwarranted attention.
    
    I agree that TRUSTED is a pretty bad choice here because of the high
    probability that people will think it means something else than what
    it really means.  LEAKPROOF isn't too bad.
    
    			regards, tom lane
    
    
  11. Re: [v9.2] Fix Leaky View Problem

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2011-09-07T16:19:16Z

    2011/9/7 Tom Lane <tgl@sss.pgh.pa.us>:
    > Noah Misch <noah@leadboat.com> writes:
    >> I liked NOLEAKY for its semantics, though I probably would have spelled it
    >> "LEAKPROOF".  PostgreSQL will trust the function to implement a specific,
    >> relatively-unintuitive security policy.  We want the function implementers to
    >> read that policy closely and not rely on any intuition they have about the
    >> "trusted" term of art.  Our use of TRUSTED in CREATE LANGUAGE is more
    >> conventional, I think, as is the trusted nature of SECURITY DEFINER.  In that
    >> vein, folks who actually need SECURITY DEFINER might first look at TRUSTED;
    >> NOLEAKY would not attract the same unwarranted attention.
    >
    > I agree that TRUSTED is a pretty bad choice here because of the high
    > probability that people will think it means something else than what
    > it really means.  LEAKPROOF isn't too bad.
    >
    It seems to me LEAKPROOF is never confusable for everyone, and
    no conflicts with other concept, although it was not in my vocaburary.
    
    If no better idea anymore, I'll submit the patch again; with LEAKPROOF keyword.
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    
  12. Re: [v9.2] Fix Leaky View Problem

    Andrew Dunstan <andrew@dunslane.net> — 2011-09-07T16:21:03Z

    
    On 09/07/2011 12:05 PM, Tom Lane wrote:
    >
    > I agree that TRUSTED is a pretty bad choice here because of the high
    > probability that people will think it means something else than what
    > it really means.
    
    Agreed.
    
    > LEAKPROOF isn't too bad.
    >
    > 			
    
    It's fairly opaque unless you know the context, although that might be 
    said of some of our other terms too. Someone coming across it is going 
    to think "What would it leak?"
    
    cheers
    
    andrew
    
    
  13. Re: [v9.2] Fix Leaky View Problem

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2011-09-07T16:24:07Z

    2011/9/7 Andrew Dunstan <andrew@dunslane.net>:
    >> LEAKPROOF isn't too bad.
    >>
    >>
    >
    > It's fairly opaque unless you know the context, although that might be said
    > of some of our other terms too. Someone coming across it is going to think
    > "What would it leak?"
    >
    It is introduced in the documentation. I'll add a point to this
    chapter in the introduction of this keyword.
    
    http://developer.postgresql.org/pgdocs/postgres/rules-privileges.html
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    
  14. Re: [v9.2] Fix Leaky View Problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-09-07T17:05:09Z

    Andrew Dunstan <andrew@dunslane.net> writes:
    > On 09/07/2011 12:05 PM, Tom Lane wrote:
    >> LEAKPROOF isn't too bad.
    
    > It's fairly opaque unless you know the context, although that might be 
    > said of some of our other terms too. Someone coming across it is going 
    > to think "What would it leak?"
    
    Well, the whole point is that we want people to RTFM instead of assuming
    they know what it means ...
    
    			regards, tom lane
    
    
  15. Re: [v9.2] Fix Leaky View Problem

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2011-09-12T19:31:21Z

    I updated the patches of fix-leaky-view problem, according to the
    previous discussion.
    The "NOLEAKY" option was replaced by "LEAKPROOF" option, and several regression
    test cases were added. Rest of stuffs are unchanged.
    
    For convenience of reviewer, below is summary of these patches:
    
    The Part-1 implements corresponding SQL syntax stuffs which are
    "security_barrier"
    reloption of views, and "LEAKPROOF" option on creation of functions to be stored
    new pg_proc.proleakproof field.
    
    The Part-2 tries to tackles a leaky-view scenarios by functions with
    very tiny cost
    estimation value. It was same one we had discussed in the commitfest-1st.
    It prevents to launch functions earlier than ones come from inside of views with
    "security_barrier" option.
    
    The Part-3 tries to tackles a leaky-view scenarios by functions that references
    one side of join loop. It prevents to distribute qualifiers including
    functions without
    "leakproof" attribute into relations across security-barrier.
    
    Thanks,
    
    2011/9/7 Kohei KaiGai <kaigai@kaigai.gr.jp>:
    > 2011/9/7 Tom Lane <tgl@sss.pgh.pa.us>:
    >> Noah Misch <noah@leadboat.com> writes:
    >>> I liked NOLEAKY for its semantics, though I probably would have spelled it
    >>> "LEAKPROOF".  PostgreSQL will trust the function to implement a specific,
    >>> relatively-unintuitive security policy.  We want the function implementers to
    >>> read that policy closely and not rely on any intuition they have about the
    >>> "trusted" term of art.  Our use of TRUSTED in CREATE LANGUAGE is more
    >>> conventional, I think, as is the trusted nature of SECURITY DEFINER.  In that
    >>> vein, folks who actually need SECURITY DEFINER might first look at TRUSTED;
    >>> NOLEAKY would not attract the same unwarranted attention.
    >>
    >> I agree that TRUSTED is a pretty bad choice here because of the high
    >> probability that people will think it means something else than what
    >> it really means.  LEAKPROOF isn't too bad.
    >>
    > It seems to me LEAKPROOF is never confusable for everyone, and
    > no conflicts with other concept, although it was not in my vocaburary.
    >
    > If no better idea anymore, I'll submit the patch again; with LEAKPROOF keyword.
    >
    > Thanks,
    > --
    > KaiGai Kohei <kaigai@kaigai.gr.jp>
    >
    
    
    
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
  16. Re: [v9.2] Fix Leaky View Problem

    Robert Haas <robertmhaas@gmail.com> — 2011-09-23T22:25:01Z

    On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    > I updated the patches of fix-leaky-view problem, according to the
    > previous discussion.
    > The "NOLEAKY" option was replaced by "LEAKPROOF" option, and several regression
    > test cases were added. Rest of stuffs are unchanged.
    
    You have a leftover reference to NOLEAKY.
    
    > For convenience of reviewer, below is summary of these patches:
    >
    > The Part-1 implements corresponding SQL syntax stuffs which are
    > "security_barrier"
    > reloption of views, and "LEAKPROOF" option on creation of functions to be stored
    > new pg_proc.proleakproof field.
    
    The way you have this implemented, we just blow away all view options
    whenever we do CREATE OR REPLACE VIEW.  Is that the behavior we want?
    If a security_barrier view gets accidentally turned into a
    non-security_barrier view, doesn't that create a security_hole?
    
    I'm also wondering if the way you're using ResetViewOptions() is the
    right way to handle this anyhow.  Isn't that going to update pg_class
    twice?  I guess that's probably harmless from a performance
    standpoint, but wouldn't it be better not to?  I guess we could define
    something like AT_ReplaceRelOptions to handle this case.
    
    The documentation in general is not nearly adequate, at least IMHO.
    
    I'm a bit nervous about storing security_barrier in the RTE.  What
    happens to stored rules if the security_barrier option gets change
    later?
    
    More when I've had more time to look at this...
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
    
  17. Re: [v9.2] Fix Leaky View Problem

    Noah Misch <noah@leadboat.com> — 2011-09-24T21:37:52Z

    On Fri, Sep 23, 2011 at 06:25:01PM -0400, Robert Haas wrote:
    > On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    > > The Part-1 implements corresponding SQL syntax stuffs which are
    > > "security_barrier"
    > > reloption of views, and "LEAKPROOF" option on creation of functions to be stored
    > > new pg_proc.proleakproof field.
    > 
    > The way you have this implemented, we just blow away all view options
    > whenever we do CREATE OR REPLACE VIEW.  Is that the behavior we want?
    > If a security_barrier view gets accidentally turned into a
    > non-security_barrier view, doesn't that create a security_hole?
    
    I think CREATE OR REPLACE needs to keep meaning just that, never becoming
    "replace some characteristics, merge others".  The consequence is less than
    delightful here, but I don't have an idea that avoids this problem without
    running afoul of some previously-raised design constraint.
    
  18. Re: [v9.2] Fix Leaky View Problem

    Robert Haas <robertmhaas@gmail.com> — 2011-09-25T15:52:47Z

    On Sat, Sep 24, 2011 at 5:37 PM, Noah Misch <noah@leadboat.com> wrote:
    > On Fri, Sep 23, 2011 at 06:25:01PM -0400, Robert Haas wrote:
    >> On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    >> > The Part-1 implements corresponding SQL syntax stuffs which are
    >> > "security_barrier"
    >> > reloption of views, and "LEAKPROOF" option on creation of functions to be stored
    >> > new pg_proc.proleakproof field.
    >>
    >> The way you have this implemented, we just blow away all view options
    >> whenever we do CREATE OR REPLACE VIEW.  Is that the behavior we want?
    >> If a security_barrier view gets accidentally turned into a
    >> non-security_barrier view, doesn't that create a security_hole?
    >
    > I think CREATE OR REPLACE needs to keep meaning just that, never becoming
    > "replace some characteristics, merge others".  The consequence is less than
    > delightful here, but I don't have an idea that avoids this problem without
    > running afoul of some previously-raised design constraint.
    
    Hmm, you might be right, although I'm not sure we've been 100%
    consistent about that, since we previously made CREATE OR REPLACE
    LANGUAGE not replace the owner with the current user.
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
    
  19. Re: [v9.2] Fix Leaky View Problem

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2011-09-25T19:25:34Z

    2011/9/24 Robert Haas <robertmhaas@gmail.com>:
    > On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    >> I updated the patches of fix-leaky-view problem, according to the
    >> previous discussion.
    >> The "NOLEAKY" option was replaced by "LEAKPROOF" option, and several regression
    >> test cases were added. Rest of stuffs are unchanged.
    >
    > You have a leftover reference to NOLEAKY.
    >
    Oops, I'll fix it.
    
    >> For convenience of reviewer, below is summary of these patches:
    >>
    >> The Part-1 implements corresponding SQL syntax stuffs which are
    >> "security_barrier"
    >> reloption of views, and "LEAKPROOF" option on creation of functions to be stored
    >> new pg_proc.proleakproof field.
    >
    > The way you have this implemented, we just blow away all view options
    > whenever we do CREATE OR REPLACE VIEW.  Is that the behavior we want?
    > If a security_barrier view gets accidentally turned into a
    > non-security_barrier view, doesn't that create a security_hole?
    >
    > I'm also wondering if the way you're using ResetViewOptions() is the
    > right way to handle this anyhow.  Isn't that going to update pg_class
    > twice?  I guess that's probably harmless from a performance
    > standpoint, but wouldn't it be better not to?  I guess we could define
    > something like AT_ReplaceRelOptions to handle this case.
    >
    IIRC, we had a discussion that the behavior should follow the case
    when a view is newly defined, even if it would be replaced actually.
    However, it seems to me consistent way to keep existing setting
    as long as user does not provide new option explicitly.
    If so, I think AT_ReplaceRelOptions enables to simplify the code
    to implement such a behavior.
    
    > The documentation in general is not nearly adequate, at least IMHO.
    >
    Do you think the description is poor to introduce the behavior changes
    corresponding to security_barrier option?
    OK, I'll try to update the documentation.
    
    > I'm a bit nervous about storing security_barrier in the RTE.  What
    > happens to stored rules if the security_barrier option gets change
    > later?
    >
    The rte->security_barrier is evaluated when a query referencing security
    views get expanded. So, rte->security_barrier is not stored to catalog.
    Even if security_barrier option was changed after PREPARE statement
    with references to security view, our existing mechanism re-evaluate
    the query on EXECUTE, thus, it shall be executed as we expected.
    (As an aside, I didn't know this mechanism and surprised at EXECUTE
    works correctly, even if VIEW definition was changed after PREPARE.)
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    
  20. Re: [v9.2] Fix Leaky View Problem

    Robert Haas <robertmhaas@gmail.com> — 2011-09-26T00:50:42Z

    On Sun, Sep 25, 2011 at 3:25 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    >> I'm a bit nervous about storing security_barrier in the RTE.  What
    >> happens to stored rules if the security_barrier option gets change
    >> later?
    >>
    > The rte->security_barrier is evaluated when a query referencing security
    > views get expanded. So, rte->security_barrier is not stored to catalog.
    
    I think it is.  If you create a view that involves an RTE, the node
    tree is going to get stored in pg_rewrite.ev_action.  And it's going
    to include the security_barrier attribute, because you added outfuncs
    support for it...
    
    No?
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
    
  21. Re: [v9.2] Fix Leaky View Problem

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2011-09-26T09:58:56Z

    2011/9/26 Robert Haas <robertmhaas@gmail.com>:
    > On Sun, Sep 25, 2011 at 3:25 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    >>> I'm a bit nervous about storing security_barrier in the RTE.  What
    >>> happens to stored rules if the security_barrier option gets change
    >>> later?
    >>>
    >> The rte->security_barrier is evaluated when a query referencing security
    >> views get expanded. So, rte->security_barrier is not stored to catalog.
    >
    > I think it is.  If you create a view that involves an RTE, the node
    > tree is going to get stored in pg_rewrite.ev_action.  And it's going
    > to include the security_barrier attribute, because you added outfuncs
    > support for it...
    >
    > No?
    >
    IIUC, nested views are also expanded when user's query gets rewritten.
    Thus, rte->security_barrier shall be set based on the latest configuration
    of the view.
    I injected an elog(NOTICE, ...) to confirm the behavior, when security_barrier
    flag was set on rte->security_barrier at ApplyRetrieveRule().
    
    postgres=# CREATE VIEW v1 WITH (security_barrier=true) AS SELECT *
    FROM t1 WHERE a % 2 = 0;
    CREATE VIEW
    postgres=# CREATE VIEW v2 WITH (security_barrier=true) AS SELECT a + a
    AS aa, b FROM v1;
    CREATE VIEW
    
    postgres=# SELECT * FROM v2;
    NOTICE:  security barrier set on v1
    NOTICE:  security barrier set on v2
     aa |  b
    ----+-----
      4 | bbb
    (1 row)
    
    postgres=# ALTER TABLE v1 SET (security_barrier=false);
    ALTER TABLE
    postgres=# SELECT * FROM v2;
    NOTICE:  security barrier set on v2
     aa |  b
    ----+-----
      4 | bbb
    (1 row)
    
    postgres=# ALTER TABLE v1 SET (security_barrier=true);
    ALTER TABLE
    postgres=# SELECT * FROM v2;
    NOTICE:  security barrier set on v1
    NOTICE:  security barrier set on v2
     aa |  b
    ----+-----
      4 | bbb
    (1 row)
    
    It seems to me the rte->security_barrier flag is correctly set, even
    if underlying view's option was changed later.
    
    Thanks,
    --
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    
  22. Re: [v9.2] Fix Leaky View Problem

    Robert Haas <robertmhaas@gmail.com> — 2011-09-26T18:37:26Z

    On Mon, Sep 26, 2011 at 5:58 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    >> I think it is.  If you create a view that involves an RTE, the node
    >> tree is going to get stored in pg_rewrite.ev_action.  And it's going
    >> to include the security_barrier attribute, because you added outfuncs
    >> support for it...
    >>
    >> No?
    >>
    > IIUC, nested views are also expanded when user's query gets rewritten.
    > Thus, rte->security_barrier shall be set based on the latest configuration
    > of the view.
    > I injected an elog(NOTICE, ...) to confirm the behavior, when security_barrier
    > flag was set on rte->security_barrier at ApplyRetrieveRule().
    
    Hmm, OK.  I am still not convinced that this is the right approach.
    Normally, we don't cache anything in the RangeTblEntry that might
    change between plan time and execution time.  Those things are
    normally stored in the RelOptInfo - why not do the same here?
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
    
  23. Re: [v9.2] Fix Leaky View Problem

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2011-09-26T19:23:55Z

    2011/9/26 Robert Haas <robertmhaas@gmail.com>:
    > On Mon, Sep 26, 2011 at 5:58 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    >>> I think it is.  If you create a view that involves an RTE, the node
    >>> tree is going to get stored in pg_rewrite.ev_action.  And it's going
    >>> to include the security_barrier attribute, because you added outfuncs
    >>> support for it...
    >>>
    >>> No?
    >>>
    >> IIUC, nested views are also expanded when user's query gets rewritten.
    >> Thus, rte->security_barrier shall be set based on the latest configuration
    >> of the view.
    >> I injected an elog(NOTICE, ...) to confirm the behavior, when security_barrier
    >> flag was set on rte->security_barrier at ApplyRetrieveRule().
    >
    > Hmm, OK.  I am still not convinced that this is the right approach.
    > Normally, we don't cache anything in the RangeTblEntry that might
    > change between plan time and execution time.  Those things are
    > normally stored in the RelOptInfo - why not do the same here?
    >
    The point is that a sub-query come from a particular view does not
    keep the information what view originally stored the sub-query when
    it was passed to the executor stage.
    PostgreSQL handles a view as just a sub-query after the rewriter stage.
    
    One possible idea not to store the flag in RangeTblEntry is to utilize
    rte->relid to show the relation-id of the source view, when rtekind is
    RTE_SUBQUERY; that enables to pull the security_barrier flag in
    executor stage.
    However, the interface to reference reloptions are designed to pull
    this information with Relation pointer, rather than lsyscache, so
    I implemented this revision with a new rte->security_barrier member.
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    
  24. Re: [v9.2] Fix Leaky View Problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-09-26T19:51:00Z

    Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
    > However, the interface to reference reloptions are designed to pull
    > this information with Relation pointer, rather than lsyscache, so
    > I implemented this revision with a new rte->security_barrier member.
    
    This approach will guarantee that we can never implement an ALTER VIEW
    (or CREATE OR REPLACE VIEW) option that changes the state of the flag.
    I don't think that's a good idea.
    
    			regards, tom lane
    
    
  25. Re: [v9.2] Fix Leaky View Problem

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2011-09-26T19:57:15Z

    2011/9/26 Tom Lane <tgl@sss.pgh.pa.us>:
    > Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
    >> However, the interface to reference reloptions are designed to pull
    >> this information with Relation pointer, rather than lsyscache, so
    >> I implemented this revision with a new rte->security_barrier member.
    >
    > This approach will guarantee that we can never implement an ALTER VIEW
    > (or CREATE OR REPLACE VIEW) option that changes the state of the flag.
    > I don't think that's a good idea.
    >
    Sorry, are you saying the current (in other words, rte->security_barrier
    stores the state of reloption) approach is not a good idea?
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    
  26. Re: [v9.2] Fix Leaky View Problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-09-26T19:59:39Z

    Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
    > Sorry, are you saying the current (in other words, rte->security_barrier
    > stores the state of reloption) approach is not a good idea?
    
    Yes.  I think the same as Robert: the way to handle this is to store it
    in RelOptInfo for the duration of planning, and pull it from the
    catalogs during planner startup (cf plancat.c).
    
    			regards, tom lane
    
    
  27. Re: [v9.2] Fix Leaky View Problem

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2011-09-26T20:06:44Z

    2011/9/26 Tom Lane <tgl@sss.pgh.pa.us>:
    > Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
    >> Sorry, are you saying the current (in other words, rte->security_barrier
    >> stores the state of reloption) approach is not a good idea?
    >
    > Yes.  I think the same as Robert: the way to handle this is to store it
    > in RelOptInfo for the duration of planning, and pull it from the
    > catalogs during planner startup (cf plancat.c).
    >
    Hmm. If so, it seems to me worthwhile to investigate an alternative
    approach that stores relation-id of the view on rte->relid if rtekind is
    RTE_SUBQUERY and pull the "security_barrier" flag from the catalog
    during planner stage.
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    
  28. Re: [v9.2] Fix Leaky View Problem

    Robert Haas <robertmhaas@gmail.com> — 2011-09-26T20:26:02Z

    On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    > The Part-2 tries to tackles a leaky-view scenarios by functions with
    > very tiny cost
    > estimation value. It was same one we had discussed in the commitfest-1st.
    > It prevents to launch functions earlier than ones come from inside of views with
    > "security_barrier" option.
    >
    > The Part-3 tries to tackles a leaky-view scenarios by functions that references
    > one side of join loop. It prevents to distribute qualifiers including
    > functions without
    > "leakproof" attribute into relations across security-barrier.
    
    I took a little more of a look at this today.  It has major problems.
    
    First, I get compiler warnings (which you might want to trap in the
    future by creating src/Makefile.custom with COPT=-Werror when
    compiling).
    
    Second, the regression tests fail on the select_views test.
    
    Third, it appears that the part2 patch works by adding an additional
    traversal of the entire query tree to standard_planner().  I don't
    think we want to add overhead to the common case where no security
    views are in use, or at least it had better be very small - so this
    doesn't seem acceptable to me.
    
    Here are some simple benchmarking with pgbench -S (scale factor 10,
    shared_buffers=400MB, MacBook Pro laptop) with and without this stack
    of patches.  These aren't clear-cut enough to make me absolutely sure
    that this patch causes a noticeable performance regression, but I
    think it does, and I'm not at all sure that this is the worst case:
    
    results.kaigai.1:tps = 9359.908769 (including connections establishing)
    results.kaigai.1:tps = 9366.317857 (including connections establishing)
    results.kaigai.1:tps = 9413.593349 (including connections establishing)
    results.master.1:tps = 9444.494510 (including connections establishing)
    results.master.1:tps = 9400.486860 (including connections establishing)
    results.master.1:tps = 9472.220529 (including connections establishing)
    
    In the light of these problems, it doesn't seem worthwhile for me to
    spend any more time on this right now: it looks to me like this needs
    a lot more work before it can be considered for commit.  I will mark
    it Waiting on Author for now, but I think Returned with Feedback might
    be more appropriate.  This needs more than light cleanup; it needs
    much more rigorous testing, both as to correctness and performance,
    and at least a partial redesign.
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
    
  29. Re: [v9.2] Fix Leaky View Problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-09-26T20:40:25Z

    Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
    > One possible idea not to store the flag in RangeTblEntry is to utilize
    > rte->relid to show the relation-id of the source view, when rtekind is
    > RTE_SUBQUERY; that enables to pull the security_barrier flag in
    > executor stage.
    
    Maybe I'm confused here, but what does the executor need the information
    for?  I thought this was a planner problem.
    
    			regards, tom lane
    
    
  30. Re: [v9.2] Fix Leaky View Problem

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2011-09-26T20:41:30Z

    2011/9/26 Tom Lane <tgl@sss.pgh.pa.us>:
    > Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
    >> One possible idea not to store the flag in RangeTblEntry is to utilize
    >> rte->relid to show the relation-id of the source view, when rtekind is
    >> RTE_SUBQUERY; that enables to pull the security_barrier flag in
    >> executor stage.
    >
    > Maybe I'm confused here, but what does the executor need the information
    > for?  I thought this was a planner problem.
    >
    Sorry, "planner" was what I wanted to say.
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    
  31. Re: [v9.2] Fix Leaky View Problem

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2011-09-27T08:05:38Z

    2011/9/26 Robert Haas <robertmhaas@gmail.com>:
    > On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    >> The Part-2 tries to tackles a leaky-view scenarios by functions with
    >> very tiny cost
    >> estimation value. It was same one we had discussed in the commitfest-1st.
    >> It prevents to launch functions earlier than ones come from inside of views with
    >> "security_barrier" option.
    >>
    >> The Part-3 tries to tackles a leaky-view scenarios by functions that references
    >> one side of join loop. It prevents to distribute qualifiers including
    >> functions without
    >> "leakproof" attribute into relations across security-barrier.
    >
    > I took a little more of a look at this today.  It has major problems.
    >
    > First, I get compiler warnings (which you might want to trap in the
    > future by creating src/Makefile.custom with COPT=-Werror when
    > compiling).
    >
    > Second, the regression tests fail on the select_views test.
    >
    > Third, it appears that the part2 patch works by adding an additional
    > traversal of the entire query tree to standard_planner().  I don't
    > think we want to add overhead to the common case where no security
    > views are in use, or at least it had better be very small - so this
    > doesn't seem acceptable to me.
    >
    The reason why I put a walker routine on the head of standard_planner()
    was that previous revision of this patch tracked strict depth of sub-queries,
    not a number of times to go through security barrier.
    The policy to count-up depth of qualifier was changed according to Noad's
    suggestion is commit-fest 1st, however, the suitable position to mark the
    depth value was kept.
    I'll try to revise the suitable position to track the depth value. It seems to
    me one candidate is pull_up_subqueries during its recursive call, because
    this patch originally set FromExpr->security_barrier here.
    
    In addition to the two points you mentioned above, I'll update this patch
    as follows:
    * Use CREATE [SECURITY] VIEW statement, instead of reloptions.
      the flag shall be stored within a new attribute of pg_class, and it shall
      be kept when an existing view getting replaced.
    
    * Utilize RangeTblEntry->relid, instead of rte->security_barrier, and the
      flag shall be pulled from the catalog on planner stage.
    
    * Documentation and Regression test.
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    
  32. Re: [v9.2] Fix Leaky View Problem

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2011-09-28T15:43:37Z

    I updated the patches according to the suggestion.
    And most of documentation and regression test changes are consolidated
    to the part-4 patch.
    
    * CREATE SECURITY VIEW statement, instead of reloptions. (Part-1)
    
    I added this new statement, instead of reloptions support on views.
    The "security_barrier" information gets being stored on
    pg_class.relissecbarrier field newly added.
    However, in my opinion, the previous implementation was simpler from
    the viewpoint of code,
    because it allows to utilize existing facilities to support reloptions.
    As long as we definitely inform users the reloptions are preserved
    even if a view is replaced,
    my preference is utilization of reloption...
    
    * RangeTblEntry->security_barrier has gone.
    
    The security_barrier field of RangeTblEntry was removed, and
    rte->relid also gets utilized to track
    what view was originally referenced instead of the sub-query.
    The rte->relid enables to pull "security_barrier" flag from the
    catalog, if and when a sub-query is
    originally referenced as a view.
    
    * Performance improvement
    
    I removed mark_qualifiers_depth that walks on whole of the Query tree
    from the head of
    standard_planner. Instead of this, I put this routine at pull_up_subqueries().
    This design change enabled to avoid unneeded tree-walking when
    security view was not
    appeared in the query, because Expr nodes are initialized by 0 on
    makeNode(), so we
    don't need to mark the "depth" field by zero.
    The subquery_depth is incremented when we pull-up a sub-query that
    come from security
    view, and its initial value is 0. So, we don't need to mark depth
    unless we pull up a sub-query
    come from the security view.
    
    +       /*
    +        * Mark the sub-query depth of qualifiers to determine the original
    +        * level of them, if necessary. Expr->depth is initialized to zero,
    +        * so we don't need to walk on the expression tree, if security view
    +        * was not used.
    +        */
    +       if (subquery_depth > 0)
    +           mark_qualifiers_depth(f->quals, subquery_depth);
    
    I have no idea why the patched version records better results,
    although it might be within
    the mergin of statistical errors. Could you reproduce it in your environment?
    - Xeon E5504 (2.00GHz), shared_buffers=512, scaling factor=10
    - Using pgbench -S -c 4 -T 15 postgres
    
    * Git master + all the patches
    tps = 6414.525599 (excluding connections establishing)
    tps = 6422.960691 (excluding connections establishing)
    tps = 6239.301706 (excluding connections establishing)
    tps = 6406.008424 (excluding connections establishing)
    tps = 6361.722286 (excluding connections establishing)
    
    * Git master
    tps = 6141.622043 (excluding connections establishing)
    tps = 6243.385064 (excluding connections establishing)
    tps = 6266.548213 (excluding connections establishing)
    tps = 6020.004101 (excluding connections establishing)
    tps = 6210.104070 (excluding connections establishing)
    
    * Compiler warnning
    I fixed them using Makefile.custom.
    
    * Regression test
    I noticed there are two select_view*.out files in regtest/expected directory,
    and regress.diff compared results/select_view.out and
    expected/select_view_1.out.
    So, I copied my results/select_view.out to expected/select_view_1.out,
    then we have no unexpected regression test errors.
    However, it seems to me quite confusable.
    
    Thanks,
    
    2011/9/27 Kohei KaiGai <kaigai@kaigai.gr.jp>:
    > 2011/9/26 Robert Haas <robertmhaas@gmail.com>:
    >> On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    >>> The Part-2 tries to tackles a leaky-view scenarios by functions with
    >>> very tiny cost
    >>> estimation value. It was same one we had discussed in the commitfest-1st.
    >>> It prevents to launch functions earlier than ones come from inside of views with
    >>> "security_barrier" option.
    >>>
    >>> The Part-3 tries to tackles a leaky-view scenarios by functions that references
    >>> one side of join loop. It prevents to distribute qualifiers including
    >>> functions without
    >>> "leakproof" attribute into relations across security-barrier.
    >>
    >> I took a little more of a look at this today.  It has major problems.
    >>
    >> First, I get compiler warnings (which you might want to trap in the
    >> future by creating src/Makefile.custom with COPT=-Werror when
    >> compiling).
    >>
    >> Second, the regression tests fail on the select_views test.
    >>
    >> Third, it appears that the part2 patch works by adding an additional
    >> traversal of the entire query tree to standard_planner().  I don't
    >> think we want to add overhead to the common case where no security
    >> views are in use, or at least it had better be very small - so this
    >> doesn't seem acceptable to me.
    >>
    > The reason why I put a walker routine on the head of standard_planner()
    > was that previous revision of this patch tracked strict depth of sub-queries,
    > not a number of times to go through security barrier.
    > The policy to count-up depth of qualifier was changed according to Noad's
    > suggestion is commit-fest 1st, however, the suitable position to mark the
    > depth value was kept.
    > I'll try to revise the suitable position to track the depth value. It seems to
    > me one candidate is pull_up_subqueries during its recursive call, because
    > this patch originally set FromExpr->security_barrier here.
    >
    > In addition to the two points you mentioned above, I'll update this patch
    > as follows:
    > * Use CREATE [SECURITY] VIEW statement, instead of reloptions.
    >  the flag shall be stored within a new attribute of pg_class, and it shall
    >  be kept when an existing view getting replaced.
    >
    > * Utilize RangeTblEntry->relid, instead of rte->security_barrier, and the
    >  flag shall be pulled from the catalog on planner stage.
    >
    > * Documentation and Regression test.
    >
    > Thanks,
    > --
    > KaiGai Kohei <kaigai@kaigai.gr.jp>
    >
    
    
    
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
  33. Re: [v9.2] Fix Leaky View Problem

    Robert Haas <robertmhaas@gmail.com> — 2011-12-07T17:52:23Z

    On Mon, Sep 26, 2011 at 3:59 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
    > Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
    >> Sorry, are you saying the current (in other words, rte->security_barrier
    >> stores the state of reloption) approach is not a good idea?
    >
    > Yes.  I think the same as Robert: the way to handle this is to store it
    > in RelOptInfo for the duration of planning, and pull it from the
    > catalogs during planner startup (cf plancat.c).
    
    Having looked at this more, I'm starting to believe KaiGai has this
    part right after all.  The trouble is that the rewriter does this:
    
        /*
         * Now, plug the view query in as a subselect, replacing the relation's
         * original RTE.
         */
        rte = rt_fetch(rt_index, parsetree->rtable);
        rte->rtekind = RTE_SUBQUERY;
        rte->relid = InvalidOid;
        rte->subquery = rule_action;
        rte->inh = false;           /* must not be set for a subquery */
    
    In other words, by the time the planner comes along and tries to
    decide whether or not it should flatten this subquery, the view has
    already been rewritten into a subquery - and that subquery is in most
    respects indistinguishable from a subquery that the user wrote
    directly.  There is one difference: the permission check that would
    have been done against the view gets attached to the OLD entry in the
    subquery's range table.  It would probably be possible to make this
    work by having the code paths that need to know whether or not a given
    subquery originated from a security-barrier-enabled view do that same
    trick: peek down into the OLD entry in the subquery rangetable,
    extract the view OID from there, and go check its reloptions.  But
    that seems awfully complicated and error-prone, hence my feeling that
    just flagging the subquery explicitly is probably a better approach.
    
    One other possibility that comes to mind is that, instead of adding
    "bool security_view" to the RTE, we could instead add a new RTEKind,
    something like RTE_SECURITY_VIEW.  That would mean going through and
    finding all the places that refer to RTE_SUBQUERY and adjusting them
    to handle RTE_SECURITY_VIEW in either the same way or differently as
    may be appropriate.  The possible advantage of this approach is that
    it doesn't bloat the RTE structure (and stored rules that use it) with
    an additional attribute that (I think) will always be false - because
    security_barrier can only be set on a subquery RTE after rewriting has
    happened, and stored rules are haven't been rewritten yet.  It might
    also force people to think a bit more carefully about how security
    views should be handled during future code changes, which could also
    be viewed as a plus.
    
    I'm attaching my current version of KaiGai's patch (with substantial
    cleanup of the comments and documentation, and some other changes) for
    reference.
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
  34. Re: [v9.2] Fix Leaky View Problem

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-12-07T18:45:19Z

    Robert Haas <robertmhaas@gmail.com> writes:
    > Having looked at this more, I'm starting to believe KaiGai has this
    > part right after all.
    
    Yeah, you have a point.  The rewriter is intentionally trying to make an
    expanded view look just the same as an in-line SELECT-in-FROM, and we
    need it to be easier to distinguish them.
    
    > One other possibility that comes to mind is that, instead of adding
    > "bool security_view" to the RTE, we could instead add a new RTEKind,
    > something like RTE_SECURITY_VIEW.  That would mean going through and
    > finding all the places that refer to RTE_SUBQUERY and adjusting them
    > to handle RTE_SECURITY_VIEW in either the same way or differently as
    > may be appropriate.  The possible advantage of this approach is that
    > it doesn't bloat the RTE structure (and stored rules that use it) with
    > an additional attribute that (I think) will always be false - because
    > security_barrier can only be set on a subquery RTE after rewriting has
    > happened, and stored rules are haven't been rewritten yet.  It might
    > also force people to think a bit more carefully about how security
    > views should be handled during future code changes, which could also
    > be viewed as a plus.
    
    Hmm.  The question is whether the places where we need to care about
    this would naturally be looking at RTEKind anyway.  If they are, or many
    are, then I think this might be a good idea.  However if a lot of the
    action is elsewhere then I don't know if we get much leverage from the
    new RTEKind.  I haven't read the patch lately so can't opine on that.
    
    			regards, tom lane
    
    
  35. Re: [v9.2] Fix Leaky View Problem

    Robert Haas <robertmhaas@gmail.com> — 2011-12-07T19:09:08Z

    On Wed, Dec 7, 2011 at 1:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
    >> One other possibility that comes to mind is that, instead of adding
    >> "bool security_view" to the RTE, we could instead add a new RTEKind,
    >> something like RTE_SECURITY_VIEW.  That would mean going through and
    >> finding all the places that refer to RTE_SUBQUERY and adjusting them
    >> to handle RTE_SECURITY_VIEW in either the same way or differently as
    >> may be appropriate.  The possible advantage of this approach is that
    >> it doesn't bloat the RTE structure (and stored rules that use it) with
    >> an additional attribute that (I think) will always be false - because
    >> security_barrier can only be set on a subquery RTE after rewriting has
    >> happened, and stored rules are haven't been rewritten yet.  It might
    >> also force people to think a bit more carefully about how security
    >> views should be handled during future code changes, which could also
    >> be viewed as a plus.
    >
    > Hmm.  The question is whether the places where we need to care about
    > this would naturally be looking at RTEKind anyway.  If they are, or many
    > are, then I think this might be a good idea.  However if a lot of the
    > action is elsewhere then I don't know if we get much leverage from the
    > new RTEKind.  I haven't read the patch lately so can't opine on that.
    
    *reads through the code*
    
    It looks to me like most places that look at RTE_SUBQUERY really have
    no reason to care about this. So probably it's just as well to have a
    separate flag for it.
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company