Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1403)
Martijn van Oosterhout wrote: On Tue, Jan 13, 2009 at 10:05:45AM -0300, Alvaro Herrera wrote: pgace.h: you have a bunch of static inline functions in here. As far as I know this doesn't work in compilers other than GCC :-( See pg_list.h (list_head) for an example. I think we can tolerate this for the three functions in pg_list.h because they are so few and so tiny, but I'm not sure about PGACE because they are a large lot. On the other hand, turning them to real functions would be a performance hit. Really? C99 requires it and MSVC does support it. At least the other compilers whose name I remembered (HP, Sun) support it also. I'd be surprised if a compiler didn't since it's the form of inline that most matches what people expect to happen. Do you have an example? I have no preference either of them, because it is not an essence of my patches whether its security hooks are implemented as inline, or not. IIRC, indeed, some of compiler also supported static inline. However, it also seems to me that PostgreSQL implementation tend to avoid to use inline functions actively. For example, heap_getattr() and fastgetattr() are implemented as macros, even if they have a bit complex conditional branches, which can be rewritten more simple with inline functions. If we have any policy to use inline functions, I'll follow this. Thanks, http://www.greenend.org.uk/rjk/2003/03/inline.html http://hi.baidu.com/junru/blog/item/4d8db11339050c856438db7a.html Have a nice day, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1403)
Martijn van Oosterhout wrote: On Tue, Jan 13, 2009 at 10:05:45AM -0300, Alvaro Herrera wrote: pgace.h: you have a bunch of static inline functions in here. As far as I know this doesn't work in compilers other than GCC :-( See pg_list.h (list_head) for an example. I think we can tolerate this for the three functions in pg_list.h because they are so few and so tiny, but I'm not sure about PGACE because they are a large lot. On the other hand, turning them to real functions would be a performance hit. Really? C99 requires it and MSVC does support it. Apparently we're stuck with C89 for some time yet; maybe an upgrade can be argued, if a large gain by the jump to C99 can be demonstrated: http://archives.postgresql.org/pgsql-hackers/2008-11/msg01494.php -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1403)
KaiGai Kohei wrote: Martijn van Oosterhout wrote: On Tue, Jan 13, 2009 at 10:05:45AM -0300, Alvaro Herrera wrote: pgace.h: you have a bunch of static inline functions in here. As far as I know this doesn't work in compilers other than GCC :-( See pg_list.h (list_head) for an example. I think we can tolerate this for the three functions in pg_list.h because they are so few and so tiny, but I'm not sure about PGACE because they are a large lot. On the other hand, turning them to real functions would be a performance hit. Really? C99 requires it and MSVC does support it. At least the other compilers whose name I remembered (HP, Sun) support it also. I'd be surprised if a compiler didn't since it's the form of inline that most matches what people expect to happen. Do you have an example? I have no preference either of them, because it is not an essence of my patches whether its security hooks are implemented as inline, or not. IIRC, indeed, some of compiler also supported static inline. However, it also seems to me that PostgreSQL implementation tend to avoid to use inline functions actively. For example, heap_getattr() and fastgetattr() are implemented as macros, even if they have a bit complex conditional branches, which can be rewritten more simple with inline functions. I thought one advantage of using macros is that we force the inlining, while I think inline compiler directives are more of a hint, but maybe the compiler knows better than we do in some cases. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1403)
Martijn van Oosterhout klep...@svana.org writes: On Tue, Jan 13, 2009 at 10:05:45AM -0300, Alvaro Herrera wrote: pgace.h: you have a bunch of static inline functions in here. As far as I know this doesn't work in compilers other than GCC :-( Really? C99 requires it and MSVC does support it. Wrong. What C99 requires is a uselessly cumbersome form of inline that is not compatible with the GCC feature. We did actually implement C99-compatible inlines in one or two places (in the sorting code IIRC), but it's not something that I want to put up with on a large scale. If you ran the current sepostgres patch through an actual C99 compiler, it would fail. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1403)
Bruce Momjian br...@momjian.us writes: KaiGai Kohei wrote: However, it also seems to me that PostgreSQL implementation tend to avoid to use inline functions actively. I thought one advantage of using macros is that we force the inlining, The (only) good thing about macros is they're portable: they work, and work the same, on every C compiler. This cannot be said of inline. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1403)
Tom Lane wrote: If you ran the current sepostgres patch through an actual C99 compiler, it would fail. The current one (r1408) is reworked to use normal functions, and inlines are eliminated. :-) http://code.google.com/p/sepgsql/source/browse/trunk/sepgsql/src/include/security/sepgsql.h http://code.google.com/p/sepgsql/source/browse/trunk/sepgsql/src/backend/security/pgaceHooks.c Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1403)
On Wed, Jan 14, 2009 at 10:00 AM, Tom Lane t...@sss.pgh.pa.us wrote:
Bruce Momjian br...@momjian.us writes:
KaiGai Kohei wrote:
However, it also seems to me that PostgreSQL implementation tend to
avoid to use inline functions actively.
I thought one advantage of using macros is that we force the inlining,
The (only) good thing about macros is they're portable: they work,
and work the same, on every C compiler. This cannot be said of inline.
Just out of curiosity, does C89, or whatever standard we follow, allow this?
int
somefunc(int x)
{
int foo[x];
/* use foo[] for scratch space */
}
Obviously this is a bad plan if x can be a big number because you
might crash your stack, but suppose we know that's not an issue? It
seems a shame to have to do palloc/pfree in a situation like this.
...Robert
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1403)
Robert Haas robertmh...@gmail.com writes:
Just out of curiosity, does C89, or whatever standard we follow, allow this?
int
somefunc(int x)
{
int foo[x];
/* use foo[] for scratch space */
}
It's not in C89 but look up alloca.
We don't use it anywhere in postgres currently so it's kind of unlikely we
would start now.
I think C99 does allow what you typed, and I think gcc has an extension to
allow it too.
Obviously this is a bad plan if x can be a big number because you
might crash your stack, but suppose we know that's not an issue? It
seems a shame to have to do palloc/pfree in a situation like this.
palloc really isn't that expensive, unless you're allocating tons of tiny
objects or you're in a tight loop it's not worth worrying about.
--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's Slony Replication support!
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1403)
It's not in C89 but look up alloca. I know about alloca... We don't use it anywhere in postgres currently so it's kind of unlikely we would start now. :-( Obviously this is a bad plan if x can be a big number because you might crash your stack, but suppose we know that's not an issue? It seems a shame to have to do palloc/pfree in a situation like this. palloc really isn't that expensive, unless you're allocating tons of tiny objects or you're in a tight loop it's not worth worrying about. Yeah... but... It really depends on what you compare it to. It's cheap compared to 99% of the functions in the code base - perhaps so. But it's darn expensive compared to moving the stack pointer. I have seen profiles for PostgreSQL and other systems where memory management is a sizable percentage of the CPU time, so it is not silly to worry about economizing. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1403)
On Wed, Jan 14, 2009 at 09:52:20AM -0500, Tom Lane wrote: Martijn van Oosterhout klep...@svana.org writes: On Tue, Jan 13, 2009 at 10:05:45AM -0300, Alvaro Herrera wrote: pgace.h: you have a bunch of static inline functions in here. As far as I know this doesn't work in compilers other than GCC :-( Really? C99 requires it and MSVC does support it. Wrong. What C99 requires is a uselessly cumbersome form of inline that is not compatible with the GCC feature. We did actually implement C99-compatible inlines in one or two places (in the sorting code IIRC), but it's not something that I want to put up with on a large scale. I was talking about static inline, where C99 agrees completely with GCC and is significantly more portable. Have a nice day, -- Martijn van Oosterhout klep...@svana.org http://svana.org/kleptog/ Please line up in a tree and maintain the heap invariant while boarding. Thank you for flying nlogn airlines. signature.asc Description: Digital signature
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1403)
Martijn van Oosterhout wrote: On Wed, Jan 14, 2009 at 09:52:20AM -0500, Tom Lane wrote: Martijn van Oosterhout klep...@svana.org writes: On Tue, Jan 13, 2009 at 10:05:45AM -0300, Alvaro Herrera wrote: pgace.h: you have a bunch of static inline functions in here. As far as I know this doesn't work in compilers other than GCC :-( Really? C99 requires it and MSVC does support it. Wrong. What C99 requires is a uselessly cumbersome form of inline that is not compatible with the GCC feature. We did actually implement C99-compatible inlines in one or two places (in the sorting code IIRC), but it's not something that I want to put up with on a large scale. I was talking about static inline, where C99 agrees completely with GCC and is significantly more portable. As I noted yesterday, I have no preference between inline and real one. However, I don't think it is a good idea to apply such an arguable manner because of current v8.4 development schedule. All patches are available here for a long time: [1/5] http://sepgsql.googlecode.com/files/sepostgresql-sepgsql-8.4devel-3-r1408.patch [2/5] http://sepgsql.googlecode.com/files/sepostgresql-utils-8.4devel-3-r1408.patch [3/5] http://sepgsql.googlecode.com/files/sepostgresql-policy-8.4devel-3-r1408.patch [4/5] http://sepgsql.googlecode.com/files/sepostgresql-docs-8.4devel-3-r1408.patch [5/5] http://sepgsql.googlecode.com/files/sepostgresql-tests-8.4devel-3-r1408.patch I would like committer to begin their reviews. If necessary, I can rework/update them with my highest priority. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1403)
KaiGai Kohei wrote: I updated patch set of SE-PostgreSQL and related stuff (r1403). [1/5] http://sepgsql.googlecode.com/files/sepostgresql-sepgsql-8.4devel-3-r1403.patch Random observations: heapam.c: you've got a bunch of elog(ERROR) calls in there that should be ereport(ERROR), and should probably have a errcode() on them too. Also the message should be worded like this: could not insert tuple on \%s\ due to security reasons or something like that. I mean: do not use C function names in error messages; quote the table name. tuptoaster.c: same problem heap.c: typo on line 1062, says el, should say rel pgace.h: you have a bunch of static inline functions in here. As far as I know this doesn't work in compilers other than GCC :-( See pg_list.h (list_head) for an example. I think we can tolerate this for the three functions in pg_list.h because they are so few and so tiny, but I'm not sure about PGACE because they are a large lot. On the other hand, turning them to real functions would be a performance hit. The pgace worker process ... do your postmaster.c changes work when pgace is disabled? I think it tries to start the worker on every iteration. Maybe it needs more smarts in postmaster.c so that when !sepgsqlIsEnabled() it just doesn't try to start it up. Also, I think there should be a separate function to tell whether a particular PGACE_FEATURE actually needs a worker process; right now the only feature (SELinux) does need it, but is this the case for them all? I didn't delve into many details in the avc, the worker, or anything much here actually -- I just skimmed randomly. This is a really huge patch; sorry I do not have time right now to review it. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1403)
Alvaro Herrera wrote: KaiGai Kohei wrote: I updated patch set of SE-PostgreSQL and related stuff (r1403). [1/5] http://sepgsql.googlecode.com/files/sepostgresql-sepgsql-8.4devel-3-r1403.patch Random observations: Thanks for your comment! heapam.c: you've got a bunch of elog(ERROR) calls in there that should be ereport(ERROR), and should probably have a errcode() on them too. Also the message should be worded like this: could not insert tuple on \%s\ due to security reasons or something like that. I mean: do not use C function names in error messages; quote the table name. tuptoaster.c: same problem heap.c: typo on line 1062, says el, should say rel Fixed, http://code.google.com/p/sepgsql/source/detail?r=1404 IIRC, Tom pointed out we should apply ereport() for user visible error messages, not elog(), on CommitFest:May, but I missed to fix them. pgace.h: you have a bunch of static inline functions in here. As far as I know this doesn't work in compilers other than GCC :-( See pg_list.h (list_head) for an example. I think we can tolerate this for the three functions in pg_list.h because they are so few and so tiny, but I'm not sure about PGACE because they are a large lot. On the other hand, turning them to real functions would be a performance hit. It was the original implementation of PGACE hooks about 700 revisions ago. :-) http://code.google.com/p/sepgsql/source/browse/trunk/sepgsql/src/backend/security/pgaceHooks.c?r=739 On CommitFest:May, I got a comment to replace them as inline functions, so they are moved to src/inclide/security/pgace.h. At that time, it turned on/off a security feature using compile-time option, so I guess reviewer concerned about cost to invoke empty function when no enhanced security feature is available. However, I also think what you pointed out is fair enough. Now pgace hooks allows 'pgace_feature' option to choose an enhanced security feature on startup time, not a compile time, so the assumption was changed. I think they should be reverted to real functions again. Is there any other opinion? The pgace worker process ... do your postmaster.c changes work when pgace is disabled? I think it tries to start the worker on every iteration. It is incorrect. When enhanced security is disabled, pgaceStartupWorkerProcess() does nothing and always returns (pid_t) 0. It means there is no worker process. Maybe it needs more smarts in postmaster.c so that when !sepgsqlIsEnabled() it just doesn't try to start it up. When user chooses selinux as 'pgace_feature', and sepgsqlIsEnabled() returns true, it tries to start it up. Yes, it is already done. Also, I think there should be a separate function to tell whether a particular PGACE_FEATURE actually needs a worker process; right now the only feature (SELinux) does need it, but is this the case for them all? Basically, I think the core code should not invoke functions of enhanced security features directly, as far as possible. (GUC options and reloptions are exceptions.) It is worthwhile that pgace() wraps and turns on/off all the security features, even if SELinux is currently the only guest, because it enables to separate security related code so well. I didn't delve into many details in the avc, the worker, or anything much here actually -- I just skimmed randomly. This is a really huge patch; sorry I do not have time right now to review it. Never mind it. I think your comments help us to improve SE-PostgreSQL more. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1403)
On Tue, Jan 13, 2009 at 10:05:45AM -0300, Alvaro Herrera wrote: pgace.h: you have a bunch of static inline functions in here. As far as I know this doesn't work in compilers other than GCC :-( See pg_list.h (list_head) for an example. I think we can tolerate this for the three functions in pg_list.h because they are so few and so tiny, but I'm not sure about PGACE because they are a large lot. On the other hand, turning them to real functions would be a performance hit. Really? C99 requires it and MSVC does support it. At least the other compilers whose name I remembered (HP, Sun) support it also. I'd be surprised if a compiler didn't since it's the form of inline that most matches what people expect to happen. Do you have an example? http://www.greenend.org.uk/rjk/2003/03/inline.html http://hi.baidu.com/junru/blog/item/4d8db11339050c856438db7a.html Have a nice day, -- Martijn van Oosterhout klep...@svana.org http://svana.org/kleptog/ Please line up in a tree and maintain the heap invariant while boarding. Thank you for flying nlogn airlines. signature.asc Description: Digital signature
