Re: sepgsql contrib module

2011/1/5 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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