Re: [PATCH] SE-PostgreSQL Updates rev.2096

Robert Haas wrote:
> 2009/6/30 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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 stuff including 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(at)kaigai(dot)gr(dot)jp>