Re: 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 Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.