Re: sepgsql contrib module

2011/1/5 KaiGai Kohei <kaigai@ak.jp.nec.com>:
> How about feasibility to merge this 4KL chunks during the rest of
> 45 days? I think we should decide this general direction at first.

I read through this code tonight and it looks pretty straightforward.
I don't see much reason not to accept this more or less as-is.  I'm a
bit suspicious of this line:

                        { "translation",        SEPG_PROCESS__TRANSITION },

I can't help wondering based on the rest of the table whether you
intend to have the same word on that line twice, but you don't.  On a
related note, would it make sense to pare down this table to the
entries that are actually used at the moment?  And how about adding a
ProcessUtility_hook to trap evil non-DML statements that some
nefarious user might issues?

One other problem is that you need to work on your whitespace a bit.
I believe in a few places you have a mixture of tabs and spaces.  More
seriously, pgindent is going to completely mangle things like this:

/*
 * sepgsql_mode
 *
 * SEPGSQL_MODE_DISABLED        : Disabled on runtime
 * SEPGSQL_MODE_DEFAULT         : Same as system settings
 * SEPGSQL_MODE_PERMISSIVE      : Always permissive mode
 * SEPGSQL_MODE_INTERNAL        : Same as SEPGSQL_MODE_PERMISSIVE,
 *                                                        except for
no audit prints
 */

You have to write it with a line of dashes on the first and last
lines, if you don't want it reformatted as a paragraph.  It might be
worth actually running pgindent over contrib/selinux and then check
over the results.

Finally, we need to work on the documentation.

But overall, it looks pretty good, IMHO.

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