Re: [PATCH] SE-PostgreSQL Updates rev.2096 - Mailing list pgsql-hackers

Robert Haas wrote:
> 2009/6/30 KaiGai Kohei <kaigai@ak.jp.nec.com>:
>> The SE-PostgreSQL patches are updated as follows:
>>
>> 01) http://sepgsql.googlecode.com/files/sepgsql-01-sysatt-8.4-r2096.patch
>> 02) http://sepgsql.googlecode.com/files/sepgsql-02-core-8.4-r2096.patch
>> 03) http://sepgsql.googlecode.com/files/sepgsql-03-gram-8.4-r2096.patch
>> 04) http://sepgsql.googlecode.com/files/sepgsql-04-writable-8.4-r2096.patch
>> 05) http://sepgsql.googlecode.com/files/sepgsql-05-rowlevel-8.4-r2096.patch
>> 06) http://sepgsql.googlecode.com/files/sepgsql-06-perms-8.4-r2096.patch
>> 07) http://sepgsql.googlecode.com/files/sepgsql-07-extra-8.4-r2096.patch
>> 08) http://sepgsql.googlecode.com/files/sepgsql-08-utils-8.4-r2096.patch
>> 09) http://sepgsql.googlecode.com/files/sepgsql-09-tests-8.4-r2096.patch
>> 10) http://sepgsql.googlecode.com/files/sepgsql-10-docs-8.4-r2096.patch
> 
> KaiGai,
> 
> It appears that some things that you were previously asked to remove
> for the first version, like row-level security, have crept back in
> here.  I think that there is a clear consensus that what you have here
> is too ambitious for a single commit.

Robert,

The row-level security is added at the fifth patch.
If you apply only the first three patches, it performs without row-level
security version.

I don't believe all the 10 patches get commited at the first commit fest.
If preferable, I can agree to forcus on the first three patches on the first
commit fest, according to the step-by-step approach, previously suggested.
 01 ... It provides a facility to manage security labels of database obejcts. 02 ... It provides a core access control
stuffincluding communication with kernel. 03 ... It provides SECURITY_LABEL option in some of CREATE/ALTER statement.
 

> http://archives.postgresql.org/message-id/20090311135413.GG4009@alvh.no-ip.org
> http://archives.postgresql.org/message-id/336.1236792143@sss.pgh.pa.us
> 
> To put some numbers around this:
> 
> $ cat *.patch | diffstat | tail -1
>  219 files changed, 11819 insertions(+), 29 deletions(-), 857 modifications(!)

Some of files are parched by multiple patches.

It is more correct estimation.  $ diffstat sepgsql-00-full-8.4.0-r2096.patch.gz  165 files changed, 11788
insertions(+),2 deletions(-), 657 modifications(!)
 

> For comparison:
> 
> Infrastructure Changes for Recovery
>  8 files changed, 912 insertions(+), 183 deletions(-)
> Window Functions
>  92 files changed, 6631 insertions(+), 232 deletions(-)
> WITH/WITH RECURSIVE
>  77 files changed, 5826 insertions(+), 246 deletions(-)
> 
> Based on that, it looks to me like you should really aim to have a
> patch set that changes AT MOST 5-6K lines, and less would be improve
> your odds of having something accepted.  You can always submit
> follow-on patches once the basic implementation is in, but I think I'm
> reflecting the shared view of those who have looked at this in the
> past when I say that it's never going to get committed in this form.

Scale of the first three patches is as follows:
$ diffstat sepgsql-01-sysatt-8.4.0-r2095.patch \           sepgsql-02-core-8.4.0-r2095.patch \
sepgsql-03-gram-8.4.0-r2095.patch
110 files changed, 5162 insertions(+), 2 deletions(-), 308 modifications(!)

The SE-PostgreSQL without row-level security version changes about 5-6K lines.
I can agree it may be a maximum scale in a single commit fest.

> Just to be clear, the fact that you have split this up into multiple,
> separate patch FILES is of no value at all, because they are
> interdependent.  For example, I just took a quick look at the "01"
> patch and it obviously includes code that is only necessary for
> row-level security.  Nothing is going to get committed here unless it
> is a self-contained change that does not presume that there will be
> further commits in the future.

I considered the separated patches reflects the step-by-step approach
previously suggested. At least, the minimum SE-PostgreSQL can works
with the first two patches.
The purpose of 01 patch is to provides a facility to manage security
labels of any database objects, including tables, columns and so on.
However, indeed, I could find a part only necessary for row-level stuff,
such as definitions of "security_labels" and "security_acl".
I can agree to move them to the later patch.

> Unless this is resubmitted in a form that complies with the changes
> that were requested the last time it was reviewed, there is really no
> point in reviewing it again.

Right now, I think it is preferable to focus on the first three patches
at the first commit fest.

What's your opinion?
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>