Skip site navigation (1) Skip section navigation (2)

Re: Updates of SE-PostgreSQL 8.4devel patches (r1403)

From: Alvaro Herrera <alvherre(at)commandprompt(dot)com>
To: KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org, KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>, bruce(at)momjian(dot)us, tgl(at)sss(dot)pgh(dot)pa(dot)us, simon(at)2ndQuadrant(dot)com
Subject: Re: Updates of SE-PostgreSQL 8.4devel patches (r1403)
Date: 2009-01-13 13:05:45
Message-ID: 20090113130545.GB4005@alvh.no-ip.org (view raw or flat)
Thread:
Lists: pgsql-hackers
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.

In response to

Responses

pgsql-hackers by date

Next:From: Emanuel Calvo FrancoDate: 2009-01-13 13:33:58
Subject: Re: solaris libpq threaded build fails
Previous:From: Magnus HaganderDate: 2009-01-13 12:58:56
Subject: Re: Open item: kerberos warning message

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group