Re: [v9.2] Fix Leaky View Problem

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(dot)kaigai(at)emea(dot)nec(dot)com>

On 24 August 2011 13:38, Kohei Kaigai <Kohei(dot)Kaigai(at)emea(dot)nec(dot)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

2011/9/7 Thom Brown <thom(at)linux(dot)com>:
> On 24 August 2011 13:38, Kohei Kaigai <Kohei(dot)Kaigai(at)emea(dot)nec(dot)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(at)kaigai(dot)gr(dot)jp>

On 7 September 2011 14:34, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:

> 2011/9/7 Thom Brown <thom(at)linux(dot)com>:
> > On 24 August 2011 13:38, Kohei Kaigai <Kohei(dot)Kaigai(at)emea(dot)nec(dot)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

On Wed, Sep 7, 2011 at 9:39 AM, Thom Brown <thom(at)linux(dot)com> wrote:
> On 7 September 2011 14:34, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> 2011/9/7 Thom Brown <thom(at)linux(dot)com>:
>> > On 24 August 2011 13:38, Kohei Kaigai <Kohei(dot)Kaigai(at)emea(dot)nec(dot)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

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> Anyone else want to bikeshed?

I'm not sure they beat TRUSTED, but:

SECURE
OPAQUE

-Kevin

2011/9/7 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Wed, Sep 7, 2011 at 9:39 AM, Thom Brown <thom(at)linux(dot)com> wrote:
>> On 7 September 2011 14:34, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> 2011/9/7 Thom Brown <thom(at)linux(dot)com>:
>>> > On 24 August 2011 13:38, Kohei Kaigai <Kohei(dot)Kaigai(at)emea(dot)nec(dot)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(at)kaigai(dot)gr(dot)jp>

On 2011-09-07 16:02, Kevin Grittner wrote:
> Robert Haas<robertmhaas(at)gmail(dot)com> wrote:
>
>> Anyone else want to bikeshed?
>
> I'm not sure they beat TRUSTED, but:
>
> SECURE
> OPAQUE

SAVE

-- Yeb

On Wed, Sep 07, 2011 at 02:09:15PM +0100, Thom Brown wrote:
> On 24 August 2011 13:38, Kohei Kaigai <Kohei(dot)Kaigai(at)emea(dot)nec(dot)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.

Noah Misch <noah(at)leadboat(dot)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

2011/9/7 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Noah Misch <noah(at)leadboat(dot)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(at)kaigai(dot)gr(dot)jp>

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

2011/9/7 Andrew Dunstan <andrew(at)dunslane(dot)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(at)kaigai(dot)gr(dot)jp>

Andrew Dunstan <andrew(at)dunslane(dot)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

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(at)kaigai(dot)gr(dot)jp>:
> 2011/9/7 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> Noah Misch <noah(at)leadboat(dot)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(at)kaigai(dot)gr(dot)jp>
>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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

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(at)kaigai(dot)gr(dot)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.

On Sat, Sep 24, 2011 at 5:37 PM, Noah Misch <noah(at)leadboat(dot)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(at)kaigai(dot)gr(dot)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

2011/9/24 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

On Sun, Sep 25, 2011 at 3:25 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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

2011/9/26 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Sun, Sep 25, 2011 at 3:25 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

On Mon, Sep 26, 2011 at 5:58 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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

2011/9/26 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Mon, Sep 26, 2011 at 5:58 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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

2011/9/26 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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

2011/9/26 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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

Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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

2011/9/26 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

2011/9/26 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

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(at)kaigai(dot)gr(dot)jp>:
> 2011/9/26 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>
>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Mon, Sep 26, 2011 at 3:59 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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

Robert Haas <robertmhaas(at)gmail(dot)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

On Wed, Dec 7, 2011 at 1:45 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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