[v9.4] row level security

Hackers,

Please, oh please, won't someone review this? This patch has been 3
years in the making, so I suspect that it will NOT be a fast review.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

I was pointed out that the previous patch was not applied cleanly because of
enhancement on pg_class system catalog, and related pg_dump portion was
getting broken.

The attached patch fixes this matters. Please reference this patch instead.
Thanks,

2013/6/13 Kohei KaiGai <kaigai@kaigai.gr.jp>:

The attached patch implements row-level security feature; that allows to
enforce a pre-configured security policy on reference of tables with the
row-level security policy.
It enables to isolate records to be visible from others according to access
control decision, usually done based on current user's credential.
It will make sense to ensure correctness of security for SaaS style
applications that typically share a common table for multiple users but
correctness of access control was ensured with correctness of application
itself.

Here is not functional update since the last commit fest for v9.3 except
for adjustment towards the latest master branch.

So, the explanation below might be bored for someone.

This feature enhances ALTER TABLE statement as follows:
ALTER TABLE <tablename> SET ROW SECURITY
FOR <command> TO (<expression>);
ALTER TABLE <tablename> RESET ROW SECURITY
FOR <command>;
<command> := { ALL | SELECT | INSERT | UPDATE | DELETE }

Right now, only "ALL" is supported command, even though syntax reserves
future enhancement allows to set individual security policy for each command.
The <expression> should be an expression that returns a bool value. It can
reference any column in the target table and contain sub-query that reference
another tables.
Then, the pre-configured expression shall be added when the table is referenced.

See below, it gives "(X % 2 = 1)" as security policy, user can see the record
that has odd-number at X. The EXPLAIN output below shows this expression
was automatically attached.

postgres=> ALTER TABLE tbl SET ROW SECURITY FOR ALL TO (x % 2 = 1);
ALTER TABLE
postgres=> EXPLAIN SELECT * FROM tbl WHERE y like '%abc%';
QUERY PLAN
-----------------------------------------------------------------
Subquery Scan on tbl (cost=0.00..28.52 rows=1 width=36)
Filter: (tbl.y ~~ '%abc%'::text)
-> Seq Scan on tbl tbl_1 (cost=0.00..28.45 rows=6 width=36)
Filter: ((x % 2) = 1)
(4 rows)

An important point is, reference to a particular relation is replaced
with a sub-
query that has security policy expression and security barrier attribute.
That prevent any (non leakproof) user given condition earlier than
security poliy
itself, thus, it ensures all records user can see is satisfies the
security policy.

On writer-queries, things to do are similar. It adds security policy expression
on the scan phase of UPDATE or DELETE statement. Thus, only visible records
are updatable or deletable.

postgres=> EXPLAIN UPDATE tbl SET y = y || '_update' WHERE y like '%xyz%';
QUERY PLAN
-----------------------------------------------------------------------
Update on tbl (cost=0.00..28.53 rows=1 width=42)
-> Subquery Scan on tbl_1 (cost=0.00..28.53 rows=1 width=42)
Filter: (tbl_1.y ~~ '%xyz%'::text)
-> Seq Scan on tbl tbl_2 (cost=0.00..28.45 rows=6 width=42)
Filter: ((x % 2) = 1)
(5 rows)

I had a relevant presentation at PGcon last month. I think its slides
are good summary
to know brief background of the long-standing problem.
http://www.pgcon.org/2013/schedule/attachments/273_PGcon2013-kaigai-row-level-security.pdf

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

--
KaiGai Kohei <kaigai@kaigai.gr.jp>

Sorry, this included a garbage chunk when I made a patch.

The attached one is corrected revision. Please see this patch instead.
Thanks,

2013/7/8 Kohei KaiGai <kaigai@kaigai.gr.jp>:

I was pointed out that the previous patch was not applied cleanly because of
enhancement on pg_class system catalog, and related pg_dump portion was
getting broken.

The attached patch fixes this matters. Please reference this patch instead.
Thanks,

2013/6/13 Kohei KaiGai <kaigai@kaigai.gr.jp>:

The attached patch implements row-level security feature; that allows to
enforce a pre-configured security policy on reference of tables with the
row-level security policy.
It enables to isolate records to be visible from others according to access
control decision, usually done based on current user's credential.
It will make sense to ensure correctness of security for SaaS style
applications that typically share a common table for multiple users but
correctness of access control was ensured with correctness of application
itself.

Here is not functional update since the last commit fest for v9.3 except
for adjustment towards the latest master branch.

So, the explanation below might be bored for someone.

This feature enhances ALTER TABLE statement as follows:
ALTER TABLE <tablename> SET ROW SECURITY
FOR <command> TO (<expression>);
ALTER TABLE <tablename> RESET ROW SECURITY
FOR <command>;
<command> := { ALL | SELECT | INSERT | UPDATE | DELETE }

Right now, only "ALL" is supported command, even though syntax reserves
future enhancement allows to set individual security policy for each command.
The <expression> should be an expression that returns a bool value. It can
reference any column in the target table and contain sub-query that reference
another tables.
Then, the pre-configured expression shall be added when the table is referenced.

See below, it gives "(X % 2 = 1)" as security policy, user can see the record
that has odd-number at X. The EXPLAIN output below shows this expression
was automatically attached.

postgres=> ALTER TABLE tbl SET ROW SECURITY FOR ALL TO (x % 2 = 1);
ALTER TABLE
postgres=> EXPLAIN SELECT * FROM tbl WHERE y like '%abc%';
QUERY PLAN
-----------------------------------------------------------------
Subquery Scan on tbl (cost=0.00..28.52 rows=1 width=36)
Filter: (tbl.y ~~ '%abc%'::text)
-> Seq Scan on tbl tbl_1 (cost=0.00..28.45 rows=6 width=36)
Filter: ((x % 2) = 1)
(4 rows)

An important point is, reference to a particular relation is replaced
with a sub-
query that has security policy expression and security barrier attribute.
That prevent any (non leakproof) user given condition earlier than
security poliy
itself, thus, it ensures all records user can see is satisfies the
security policy.

On writer-queries, things to do are similar. It adds security policy expression
on the scan phase of UPDATE or DELETE statement. Thus, only visible records
are updatable or deletable.

postgres=> EXPLAIN UPDATE tbl SET y = y || '_update' WHERE y like '%xyz%';
QUERY PLAN
-----------------------------------------------------------------------
Update on tbl (cost=0.00..28.53 rows=1 width=42)
-> Subquery Scan on tbl_1 (cost=0.00..28.53 rows=1 width=42)
Filter: (tbl_1.y ~~ '%xyz%'::text)
-> Seq Scan on tbl tbl_2 (cost=0.00..28.45 rows=6 width=42)
Filter: ((x % 2) = 1)
(5 rows)

I had a relevant presentation at PGcon last month. I think its slides
are good summary
to know brief background of the long-standing problem.
http://www.pgcon.org/2013/schedule/attachments/273_PGcon2013-kaigai-row-level-security.pdf

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

--
KaiGai Kohei <kaigai@kaigai.gr.jp>

--
KaiGai Kohei <kaigai@kaigai.gr.jp>

With the current HEAD and v3 patch I'm seeing the following error with
"make check".

------
============== creating temporary installation ==============
============== initializing database system ==============

pg_regress: initdb failed
Examine /usr/local/src/postgres.patched.v3/src/test/regress/log/initdb.log
for the reason.
Command was:
"/usr/local/src/postgres.patched.v3/src/test/regress/./tmp_check/install//usr/local/pgsql/bin/initdb"
-D "/usr/local/src/postgres.patched.v3/src/test/regress/./tmp_check/data"
-L
"/usr/local/src/postgres.patched.v3/src/test/regress/./tmp_check/install//usr/local/pgsql/share"
--noclean --nosync >
"/usr/local/src/postgres.patched.v3/src/test/regress/log/initdb.log" 2>&1
make[1]: *** [check] Error 2
make[1]: Leaving directory
`/usr/local/src/postgres.patched.v3/src/test/regress'
make: *** [check] Error 2

__________________________________________________________________________________
*Mike Blackwell | Technical Analyst, Distribution Services/Rollout
Management | RR Donnelley*
1750 Wallace Ave | St Charles, IL 60174-3401
Office: 630.313.7818
Mike.Blackwell@rrd.com
http://www.rrdonnelley.com

<http://www.rrdonnelley.com/&gt;
* <Mike.Blackwell@rrd.com>*

Current head 4cbe3ac3e86790d05c569de4585e5075a62a9b41 -> patch applies
correct (only change needed in parallel_schedule).
However it fails on own regression tests (other tests pass).
Regards,
Karol

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 7/18/13 7:57 PM, Karol Trzcionka wrote:

Current head 4cbe3ac3e86790d05c569de4585e5075a62a9b41 -> patch applies
correct (only change needed in parallel_schedule).
However it fails on own regression tests (other tests pass).

I got a rejected hunk in src/backend/nodes/nodeFuncs.c as well as that
parallel_schedule issue. Maybe you didn't get the nodeFuncs change but
didn't notice that? That might explain why the tests didn't work for
you either.

Attached is an updated patch where I tried to only fix the two small
hunks of bit rot. I get "All 140 tests passed" here, on a Mac no less.

I did a brief code scan through the patch just to get a feel for how the
feature is put together, and what you'd need to know for a deeper
review. (I'm trying to get customer time approved to work on this a lot
more) The code was easier to follow than I expected. The way it
completely avoids even getting into the security label integration yet
seems like a successful design partitioning. This isn't nearly as scary
as the SEPostgres patches. There are some useful looking utility
functions that dump information about what's going on too.

The bulk of the complexity is how the feature modifies query nodes to
restrict what rows come through them. Some familiarity with that part
of the code is what you'd need to take on reviewing this in detail.
That and a week of time to spend trudging through it. If anyone is
looking for an educational challenge on query execution, marching
through all of these changes to validate they work as expected would do
that.

--
Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

* Greg Smith (greg@2ndQuadrant.com) wrote:

On 7/18/13 7:57 PM, Karol Trzcionka wrote:

Current head 4cbe3ac3e86790d05c569de4585e5075a62a9b41 -> patch applies
correct (only change needed in parallel_schedule).
However it fails on own regression tests (other tests pass).

I got a rejected hunk in src/backend/nodes/nodeFuncs.c as well as
that parallel_schedule issue. Maybe you didn't get the nodeFuncs
change but didn't notice that? That might explain why the tests
didn't work for you either.

The nodeFuncs.c hunk seems likely to have been impacted by the patch I
committed today (WITH CHECK OPTION), so I doubt that was the issue.

Attached is an updated patch where I tried to only fix the two small
hunks of bit rot. I get "All 140 tests passed" here, on a Mac no
less.

Thanks for updating the patch, I ran into the failed hunks too and
expected to have to deal with them. :)

I did a brief code scan through the patch just to get a feel for how
the feature is put together, and what you'd need to know for a
deeper review.

That would be extremely helpful.. Wasn't there a wiki page about this
feature which might also help? Bigger question- is it correct for the
latest version of the patch?

(I'm trying to get customer time approved to work on
this a lot more) The code was easier to follow than I expected.
The way it completely avoids even getting into the security label
integration yet seems like a successful design partitioning. This
isn't nearly as scary as the SEPostgres patches. There are some
useful looking utility functions that dump information about what's
going on too.

The bulk of the complexity is how the feature modifies query nodes
to restrict what rows come through them. Some familiarity with that
part of the code is what you'd need to take on reviewing this in
detail. That and a week of time to spend trudging through it. If
anyone is looking for an educational challenge on query execution,
marching through all of these changes to validate they work as
expected would do that.

I'm hoping to find time this weekend to look into this patch myself, but
the weekend is also filling up with other activities, so we'll see.

Thanks!

Stephen

On 7/18/13 11:03 PM, Stephen Frost wrote:

Wasn't there a wiki page about this
feature which might also help? Bigger question- is it correct for the
latest version of the patch?

https://wiki.postgresql.org/wiki/RLS has accumulated a lot of older
debris that may or may not be useful here.

This useful piece was just presented at PGCon:
http://www.pgcon.org/2013/schedule/attachments/273_PGcon2013-kaigai-row-level-security.pdf
That is very up to date intro to the big picture issues.

--
Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 19 July 2013 04:09, Greg Smith <greg@2ndquadrant.com> wrote:

On 7/18/13 11:03 PM, Stephen Frost wrote:

Wasn't there a wiki page about this
feature which might also help? Bigger question- is it correct for the
latest version of the patch?

https://wiki.postgresql.org/wiki/RLS has accumulated a lot of older debris
that may or may not be useful here.

This useful piece was just presented at PGCon:
http://www.pgcon.org/2013/schedule/attachments/273_PGcon2013-kaigai-row-level-security.pdf
That is very up to date intro to the big picture issues.

Hi,

I took a look at this patch too. I didn't read all the code in detail,
but there was one area I wondered about --- is it still necessary to
add the new field rowsec_relid to RangeTblEntry?

As far as I can see, it is only used in the new function getrelid()
which replaces the old macro of the same name, so that it can work if
it is passed the index of the sourceRelation subquery RTE instead of
the resultRelation. This seems to be encoding a specific assumption
that a subquery RTE is always going to come from row-level security
expansion.

Is it the case that this can only happen from expand_targetlist(), in
which case why not pass both source_relation and result_relation to
expand_targetlist(), which I think will make that code neater. As it
stands, what expand_targetlist() sees as result_relation is actually
source_relation, which getrelid() then handles specially. Logically I
think expand_targetlist() should pass the actual result_relation to
getrelid(), opening the target table, and then pass source_relation to
makeVar() when building new TLEs.

With that change to expand_targetlist(), the change to getrelid() may
be unnecessary, and then also the new rowsec_relid field in
RangeTblEntry may not be needed.

Regards,
Dean

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Dean Rasheed <dean.a.rasheed@gmail.com> writes:

I took a look at this patch too. I didn't read all the code in detail,
but there was one area I wondered about --- is it still necessary to
add the new field rowsec_relid to RangeTblEntry?

As far as I can see, it is only used in the new function getrelid()
which replaces the old macro of the same name, so that it can work if
it is passed the index of the sourceRelation subquery RTE instead of
the resultRelation. This seems to be encoding a specific assumption
that a subquery RTE is always going to come from row-level security
expansion.

I haven't read the patch at all, but I would opine that anything that's
changing the behavior of getrelid() is broken by definition. Something
is being done at the wrong level of abstraction if you think that you
need to do that.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2013/7/19 Stephen Frost <sfrost@snowman.net>:

* Greg Smith (greg@2ndQuadrant.com) wrote:

On 7/18/13 7:57 PM, Karol Trzcionka wrote:

Current head 4cbe3ac3e86790d05c569de4585e5075a62a9b41 -> patch applies
correct (only change needed in parallel_schedule).
However it fails on own regression tests (other tests pass).

I got a rejected hunk in src/backend/nodes/nodeFuncs.c as well as
that parallel_schedule issue. Maybe you didn't get the nodeFuncs
change but didn't notice that? That might explain why the tests
didn't work for you either.

The nodeFuncs.c hunk seems likely to have been impacted by the patch I
committed today (WITH CHECK OPTION), so I doubt that was the issue.

Attached is an updated patch where I tried to only fix the two small
hunks of bit rot. I get "All 140 tests passed" here, on a Mac no
less.

Thanks for updating the patch, I ran into the failed hunks too and
expected to have to deal with them. :)

Thanks for pointing out this problem. I synchronized my local master
with the upstream one, then adjusted the row-security branch.

I'll submit the patch soon, with fixing-up the portion that Tom pointed
out.

Thanks,

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2013/7/19 Dean Rasheed <dean.a.rasheed@gmail.com>:

On 19 July 2013 04:09, Greg Smith <greg@2ndquadrant.com> wrote:

On 7/18/13 11:03 PM, Stephen Frost wrote:

Wasn't there a wiki page about this
feature which might also help? Bigger question- is it correct for the
latest version of the patch?

https://wiki.postgresql.org/wiki/RLS has accumulated a lot of older debris
that may or may not be useful here.

This useful piece was just presented at PGCon:
http://www.pgcon.org/2013/schedule/attachments/273_PGcon2013-kaigai-row-level-security.pdf
That is very up to date intro to the big picture issues.

Hi,

I took a look at this patch too. I didn't read all the code in detail,
but there was one area I wondered about --- is it still necessary to
add the new field rowsec_relid to RangeTblEntry?

As far as I can see, it is only used in the new function getrelid()
which replaces the old macro of the same name, so that it can work if
it is passed the index of the sourceRelation subquery RTE instead of
the resultRelation. This seems to be encoding a specific assumption
that a subquery RTE is always going to come from row-level security
expansion.

Is it the case that this can only happen from expand_targetlist(), in
which case why not pass both source_relation and result_relation to
expand_targetlist(), which I think will make that code neater. As it
stands, what expand_targetlist() sees as result_relation is actually
source_relation, which getrelid() then handles specially. Logically I
think expand_targetlist() should pass the actual result_relation to
getrelid(), opening the target table, and then pass source_relation to
makeVar() when building new TLEs.

With that change to expand_targetlist(), the change to getrelid() may
be unnecessary, and then also the new rowsec_relid field in
RangeTblEntry may not be needed.

Hmm. I didn't have this idea. It seems to me fair enough and kills
necessity to enhance RangeTblEntry and getrelid() indeed.
I try to fix up this implementation according to your suggestion.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 7/20/13 10:08 AM, Kohei KaiGai wrote:

With that change to expand_targetlist(), the change to getrelid() may
be unnecessary, and then also the new rowsec_relid field in
RangeTblEntry may not be needed.

Hmm. I didn't have this idea. It seems to me fair enough and kills
necessity to enhance RangeTblEntry and getrelid() indeed.
I try to fix up this implementation according to your suggestion.

Great, there's one useful bit of feedback for you then, and that seems
to address Tom's getrelid concern too.

For the active CommitFest, I don't see any place we can go with this
right now except for "Returned with Feedback". We really need more
reviewers willing to put a significant amount of time into going through
this code.

Anyone who would like to see RLS committed should consider how you can
get resources to support a skilled PostgreSQL reviewer spending time on
it. (This is a bit much to expect new reviewers to chew on usefully)
I've been working on that here, but I don't have anything I can publicly
commit to yet.

--
Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

(This is a bit much to expect new reviewers to chew on usefully) I've been
working on that here, but I don't have anything I can publicly commit to
yet.

True that.

I spent some time on it, but couldn't come up with anything useful.
Mike's extensive testing seems good enough for me, though.

One thing I was a bit concerned about (I don't know if it has been
resolved already, or if it isn't a concern) is that there was an issue
about a security part being visible in the syntax (or something like
that, my memory isn't too good today morning).

Regards,

Atri

Regards,

Atri
l'apprenant

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 07/22/2013 01:27 PM, Greg Smith wrote:

Anyone who would like to see RLS committed should consider how you can
get resources to support a skilled PostgreSQL reviewer spending time on
it. (This is a bit much to expect new reviewers to chew on usefully)
I've been working on that here, but I don't have anything I can publicly
commit to yet.

Apparently it's a little much for experienced reviewers to chew on, too,
since I've been trying to get someone to review it since the beginning
of the Commitfest.

While I understand the call for "resources", this is a bit unfair to
KaiGai, who has put in his time reviewing other people's patches.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 7/23/13 12:10 PM, Josh Berkus wrote:

Apparently it's a little much for experienced reviewers to chew on, too,
since I've been trying to get someone to review it since the beginning
of the Commitfest.

It's more than the available experienced reviewers are willing to chew
on fully as volunteers. The reward for spending review time is pretty
low right now.

While I understand the call for "resources", this is a bit unfair to
KaiGai, who has put in his time reviewing other people's patches.

If you read Dean Rasheed's comments, it's obvious he put a useful amount
of work into his review suggestions. It is not the case here that
KaiGai worked on a review and got nothing in return. Unfortunately that
has happened to this particular patch before, but the community did a
little better this time.

The goal of the CF is usually to reach one of these outcomes for every
submission:

-The submission is ready for commit
-The submission needs improvement in X

Review here went far enough to identify an X to be improved. It was a
big enough issue that a rewrite is needed, commit at this time isn't
possible, and now KaiGai has something we hope is productive he can
continue working on. That's all we can really promise here--that if we
say something isn't ready for commit yet, that there's some feedback as
to why.

I would have preferred to see multiple X issues identified here, given
that we know there has to be more than just the one in a submission of
this size. The rough fairness promises of the CommitFest seem satisfied
to me though.

--
Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Greg,

It's more than the available experienced reviewers are willing to chew
on fully as volunteers. The reward for spending review time is pretty
low right now.

Short of paying for review time, I don't think we have another solution
for getting the "big patches" reviewed, except to rely on the major
contributors who are paid full-time to hack Postgres. You know as well
as me that, as consultants, we can get clients to pay for 10% extra time
for review in the course of developing a feature, but the kind of time
which patches like Row Security, Changesets, or other "big patches" need
nobody is going to pay for on a contract basis. And nobody who is doing
this in their "spare time" has that kind of block.

So I don't think there's any good solution for the "big patches".

I do think our project could do much more to recruit reviewers for the
small-medium patches, to take workload off the core contributors in
general. Historically, however, this project (and the contributors on
this list) has made material decisions not to encourage or recruit new
people as reviewers, and has repeatedly stated that reviewers are not
important. Until that changes, we are not going to get more reviewers
(and I'm not going to have much sympathy for existing contributors who
say they have no time for review).

If we want more reviewers and people spending more time on review, then
we need to give reviewers the *same* respect and the *same* rewards that
feature contributors get. Not something else, the exact same.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 7/23/13 2:30 PM, Josh Berkus wrote:

You know as well as me that, as consultants, we can get clients to pay for 10% extra time
for review in the course of developing a feature

Before this number gets quoted anywhere, I think it's on the low side.
I've spent a good bit of time measuring how much time it takes to do a
fair offsetting review--one where you put as much time in as it takes to
review your submission--and I keep getting numbers more in the 20 to 25%
range. The work involved to do a high quality review takes a while.

I happen to think the review structure is one of the most important
components to PostgreSQL release quality. It used to be a single level
review with just the committers, now it's a two level structure. The
reason the Postgres code is so good isn't that the submitted development
is inherently any better than other projects. There's plenty of bogus
material submitted here. It's the high standards for review and commit
that are the key filter. The importance of the process to the result
isn't weighed as heavily as I think it should be.

--
Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 07/23/2013 03:34 PM, Greg Smith wrote:

I happen to think the review structure is one of the most important
components to PostgreSQL release quality. It used to be a single level
review with just the committers, now it's a two level structure. The
reason the Postgres code is so good isn't that the submitted development
is inherently any better than other projects. There's plenty of bogus
material submitted here. It's the high standards for review and commit
that are the key filter. The importance of the process to the result
isn't weighed as heavily as I think it should be.

I think we're in violent agreement here. Now, we just need to convince
everyone else on this list.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers