Prev: [COMMITTERS] pgsql: Stamp HEAD as 9.1devel.
Next: [HACKERS] FYI: Ubuntu 10.04 lucid strange segfault
From: Stephen Frost on 9 Jul 2010 13:28
* Tom Lane (tgl(a)sss.pgh.pa.us) wrote:
> Simon Riggs <simon(a)2ndQuadrant.com> writes:
> > Agreed that permission checks should logically be applied at execution
> > time. I am proposing a performance optimisation, not a change in
> > behaviour.
>
> Except that it *is* a change in behavior: the first check will occur too
> soon.

Yeah, I have to say that I don't see any way you could avoid the
behaviour change from doing this. Specifically, you can prepare plans
today that you don't have access to run and then run them later after
you've been granted the permission.

I'm not saying that's a huge use-case or anything, but moving the checks
to planner time would definitely change the behavior. No clue if any of
this is covered in the SQL spec.

> The fact that we're interested in adding plugin permissions checking
> pretty much destroys the idea anyway. You cannot assume that a plan
> cache invalidation will happen for any change in external state that
> a plugin might be consulting.

Yeah, this would certainly be a problem too, unless we kept the plugin
hook in the executor and only used the "optimization" for the stock PG
checks.

> > The proposed performance enhancement would be very useful since we have
> > to check permissions of functions, views, tables and every other aspect.
> > We could spend a while quantifying that overhead, though "non-zero" is
> > all we need to know.
>
> No, it's not all we need to know. If you can't prove the overhead
> involved here is significant, we should not be expending effort and
> creating subtle behavioral changes in pursuit of a minor optimization.

Agreed.

Thanks,

Stephen
From: Robert Haas on 9 Jul 2010 13:33
On Fri, Jul 9, 2010 at 1:21 PM, Tom Lane <tgl(a)sss.pgh.pa.us> wrote:
> Simon Riggs <simon(a)2ndQuadrant.com> writes:
>> On Fri, 2010-07-09 at 11:09 -0400, Tom Lane wrote:
>>> Simon Riggs <simon(a)2ndQuadrant.com> writes:
>>>> Strangely, I was looking into removing the ExecCheckRTPerms check
>>>> altogether by forcing plan invalidation when permissions are updated.
>>>> That would be a performance tweak that would render this change useless.
>>>
>>> That seems both pointless and wrong. �Permissions checks should happen
>>> at execution time not plan time.
>
>> Agreed that permission checks should logically be applied at execution
>> time. I am proposing a performance optimisation, not a change in
>> behaviour.
>
> Except that it *is* a change in behavior: the first check will occur too
> soon.

You might be able to get around this by doing the first check on first
use of the plan and then going and marking all the plans as needing a
recheck whenever a permissions change happens. Whether the
performance savings are sufficient to justify such a thing is another
matter.

> The fact that we're interested in adding plugin permissions checking
> pretty much destroys the idea anyway. �You cannot assume that a plan
> cache invalidation will happen for any change in external state that
> a plugin might be consulting.

This is certainly true, but I also wonder what SE-PostgreSQL plans to
do about this. Taking this to its logical exteme, the system security
policy could change in mid-query - and while you'd like to think that
the system would stop emitting tuples on a dime, that's probably not
too feasible in practice. I am assuming that SE-PostgreSQL will want
to do some kind of caching, but I wonder how one decides what to cache
and for how long, and whether there's any mechanism for propagating
cache invalidations.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Simon Riggs on 9 Jul 2010 13:38
On Fri, 2010-07-09 at 13:21 -0400, Tom Lane wrote:
> > Agreed that permission checks should logically be applied at
> execution
> > time. I am proposing a performance optimisation, not a change in
> > behaviour.
>
> Except that it *is* a change in behavior: the first check will occur
> too
> soon.

Sooner matters why? We already have a lock on the table at plan time so
there cannot be a concurrent GRANT against a plan-then-execute
transaction. Later transactions would invalidate and replan.

> The fact that we're interested in adding plugin permissions checking
> pretty much destroys the idea anyway. You cannot assume that a plan
> cache invalidation will happen for any change in external state that
> a plugin might be consulting.

Plugin can still be executed at appropriate time, its mostly absent and
so cheap. I guess we can keep plugin whatever else I attempt.

> > The proposed performance enhancement would be very useful since we
> have
> > to check permissions of functions, views, tables and every other
> aspect.
> > We could spend a while quantifying that overhead, though "non-zero"
> is
> > all we need to know.
>
> No, it's not all we need to know. If you can't prove the overhead
> involved here is significant, we should not be expending effort and
> creating subtle behavioral changes in pursuit of a minor optimization.

OK, will gather evidence.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Training and Services


--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Stephen Frost on 9 Jul 2010 13:44
Robert,

* Robert Haas (robertmhaas(a)gmail.com) wrote:
> This is certainly true, but I also wonder what SE-PostgreSQL plans to
> do about this. Taking this to its logical exteme, the system security
> policy could change in mid-query - and while you'd like to think that
> the system would stop emitting tuples on a dime, that's probably not
> too feasible in practice. I am assuming that SE-PostgreSQL will want
> to do some kind of caching, but I wonder how one decides what to cache
> and for how long, and whether there's any mechanism for propagating
> cache invalidations.

Yes, SE-PG will be doing cacheing and this exact problem has already
been addressed (KaiGai's original SE-PG patches included cacheing,
actually). It's also not something that's unique to PG in any way.

Thanks,

Stephen
From: Tom Lane on 9 Jul 2010 14:01
Simon Riggs <simon(a)2ndQuadrant.com> writes:
> On Fri, 2010-07-09 at 13:21 -0400, Tom Lane wrote:
>> Except that it *is* a change in behavior: the first check will occur
>> too soon.

> Sooner matters why?

Consider PREPARE followed only later by EXECUTE. Your proposal would
make the PREPARE fail outright, when it currently does not.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4 5
Prev: [COMMITTERS] pgsql: Stamp HEAD as 9.1devel.
Next: [HACKERS] FYI: Ubuntu 10.04 lucid strange segfault