RLS feature has been committed

On Mon, Sep 22, 2014 at 4:02 PM, Andres Freund <andres@anarazel.de> wrote:

This patch has been pushed in a clear violation of established policy.

Fundamental pieces of the patch have changed *after* the commitfest
started. And there wasn't a recent patch in the commitfest either - the
entry was moved over from the last round without a new patch. It didn't
receive independent review (Robert explicitly said his wasn't a full
review). It wasn't marked ready for committer. The intention to commit
wasn't announced publically. There were *clear* and unaddressed
objections to committing the patch as is, by a committer (Robert)
nonetheless.

I have no reason to doubt your version of events here (although
Stephen may wish to address what you've said - I'm basing that on his
tone elsewhere). I must ask, though: what do you propose to do about
it in this instance? He has been chastised. Would you like to make a
point of formalizing what are (if I'm not mistaken) currently defacto
rules? Should RLS be reverted, and revisited in a future CF?

--
Peter Geoghegan

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

On 09/22/2014 04:22 PM, Peter Geoghegan wrote:

I have no reason to doubt your version of events here (although
Stephen may wish to address what you've said - I'm basing that on his
tone elsewhere). I must ask, though: what do you propose to do about
it in this instance? He has been chastised. Would you like to make a
point of formalizing what are (if I'm not mistaken) currently defacto
rules? Should RLS be reverted, and revisited in a future CF?

The CommitFests were never meant to restrict when a committer could
commit a patch. The point of the CFs was to give committers time *off*
from committing patches. If a committer wants to commit something
completely outside of the CF process, they are welcome to, as long as it
receives adequate review.

So if there's an argument here, it's whether or not the committed RLS
patch was adequately reviewed (and if not, if it should be reverted),
not whether it should have been in the CF or not.

--
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 Tue, Sep 23, 2014 at 6:55 AM, Josh Berkus <josh@agliodbs.com> wrote:

On 09/22/2014 04:22 PM, Peter Geoghegan wrote:

I have no reason to doubt your version of events here (although
Stephen may wish to address what you've said - I'm basing that on his
tone elsewhere). I must ask, though: what do you propose to do about
it in this instance? He has been chastised. Would you like to make a
point of formalizing what are (if I'm not mistaken) currently defacto
rules? Should RLS be reverted, and revisited in a future CF?

The CommitFests were never meant to restrict when a committer could
commit a patch. The point of the CFs was to give committers time *off*
from committing patches. If a committer wants to commit something
completely outside of the CF process, they are welcome to, as long as it
receives adequate review.

So if there's an argument here, it's whether or not the committed RLS
patch was adequately reviewed (and if not, if it should be reverted),

Who decides if the patch is adequately reviewed?

Author, Committer or Reviewer? In CF, that is comparatively clear
that once Reviewer is satisfied, he marks the patch as
Ready For Committer and then Committer picks up and if he is satisfied
with the review and quality of patch, then he commits the patch or if the
Committer himself is reviewer than in many cases once he is satisfied,
he commits it.

Now in this case, if I understand correctly the story is not
so straightforward. It seems to me Robert as the Reviewer was not
completely satisfied where as Stephen as the Committer was pretty
much okay with the patch so he went ahead and commits it.

In short, if it is solely at the discretion of Committer that he can
decide if the patch has got adequate Review and he is satisfied
with the quality of patch, then I think what Stephen did in this case
is not wrong (though I am not the one who can decide whether it is
right or wrong, just sharing my thoughts), however if you think Reviewer
also has stake (especially when other Reviewer is also Committer)
in deciding the quality of patch, then Stephen should have waited for
more time (till the Reviewer also gets satisfied).

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Mon, Sep 22, 2014 at 7:22 PM, Peter Geoghegan <pg@heroku.com> wrote:

On Mon, Sep 22, 2014 at 4:02 PM, Andres Freund <andres@anarazel.de> wrote:

This patch has been pushed in a clear violation of established policy.

Fundamental pieces of the patch have changed *after* the commitfest
started. And there wasn't a recent patch in the commitfest either - the
entry was moved over from the last round without a new patch. It didn't
receive independent review (Robert explicitly said his wasn't a full
review). It wasn't marked ready for committer. The intention to commit
wasn't announced publically. There were *clear* and unaddressed
objections to committing the patch as is, by a committer (Robert)
nonetheless.

I have no reason to doubt your version of events here

Fortunately, you don't have to take anything on faith. This is a
public mailing list, and the exact sequence of events is open to
inspection by anyone who cares to take a few minutes to do so. You
can easily verify whether my statement that I asked Stephen twice to
hold off committing it is correct or not; and you can also verify the
rest of the history that Andres and I recounted. This is all there in
black and white.

Should RLS be reverted, and revisited in a future CF?

IMHO, that would be entirely appropriate. I don't have any idea
whether the patch has remaining bugs, design issues, or security flaws
- and neither does anyone else since the normal review process was
bypassed - but I do feel that Stephen's feelings of being chastised
aren't worth the bits they are printed on. We're here to develop
software together, not to talk about our feelings; and the quality of
the software we produce depends on our willingness to follow a set of
procedures that are or should be well-understood by long-time
contributors, not on our emotional states.

It's difficult to imagine a more flagrant violation of process than
committing a patch without any warning and without even *commenting*
on the fact that clear objections to commit were made on a public
mailing list. If that is allowed to stand, what can we assume other
than that Stephen, at least, has a blank check to change anything he
wants, any time he wants, with no veto possible from anyone else?

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

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

On Mon, Sep 22, 2014 at 9:25 PM, Josh Berkus <josh@agliodbs.com> wrote:

The CommitFests were never meant to restrict when a committer could
commit a patch. The point of the CFs was to give committers time *off*
from committing patches. If a committer wants to commit something
completely outside of the CF process, they are welcome to, as long as it
receives adequate review.

Agreed.

So if there's an argument here, it's whether or not the committed RLS
patch was adequately reviewed (and if not, if it should be reverted),
not whether it should have been in the CF or not.

The point here is precisely that nobody other than the authors
reviewed it, and that I specifically asked Stephen to hold off commit
until the next CommitFest because I did not want to drop everything to
review a patch that was posted mid-CommitFest over other patches that
were timely submitted. Stephen took the technical content that
appeared in that same email, incorporated into the patch, and
committed it shortly thereafter.

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

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

Robert Haas wrote

It's difficult to imagine a more flagrant violation of process than
committing a patch without any warning and without even *commenting*
on the fact that clear objections to commit were made on a public
mailing list. If that is allowed to stand, what can we assume other
than that Stephen, at least, has a blank check to change anything he
wants, any time he wants, with no veto possible from anyone else?

I'm of a mind to agree that this shouldn't have been committed...but I'm not
seeing where Stephen has done sufficient wrong to justify crucifixion.
Especially since everything is being done publicly and you are one of the
many people in the position to flex a veto by reverting the patch.

I see this like a black swan event[1]: needs to be addressed, is thought
provoking, but ultimately shouldn't be treated as something to overreact to
until it happens more frequently. There are enough checks-and-balances when
it comes to committed code - which in this case is during pre-beta
development - that one should simply allow for a certain level human fallacy
to creep in and need periodic addressing/correcting.

At this point my hindsight says a strictly declaratory statement of "reasons
this is not ready" combined with reverting the patch would have been
sufficient; or even just a "I am going to revert this for these reasons"
post. The difference between building support for a revert and gathering a
mob is a pretty thin line.

Subsequent, possibly private, discussion between you and Stephen could then
occur before making any conclusions you form public so that others can learn
from the experience and ponder whether anything could be changed to mitigate
such situations in the future.

Though I guess if you indeed feel that his actions were truly heinous you
should also then put forth the proposal that his ability to commit be
revoked. If your not willing to go to that extent then, unless you know
Stephen personally, I'd not assume that public flogging is the best way to
get him to not mess up in the future; but the honest and cordial dialog
about cause/effect is likely to be more productive and less
self-destructing. Though, to each their own style.

As a committer you have a responsibility to work not only with code but with
those who write the code; and while I myself am not a particularly strong
(or experienced) manager I have enough life experience to give opinions on
leadership. I won't fault you for being yourself but simply put forth my
own impressions and some ideas to provoke thought.

I'm also not the one whose efforts were marginalized so don't have the same
emotional attachment to the situation as you do - an attachment that needs
to be recognized because, as I do know from experience, even when you are
right and justified an overreaction makes you come off unfavorably and
doesn't materially improve the situation beyond what it could have been
otherwise.

David J.

1. http://en.wikipedia.org/wiki/Black_swan_theory

--
View this message in context: http://postgresql.1045698.n5.nabble.com/RLS-feature-has-been-committed-tp5819983p5820020.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.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 Mon, Sep 22, 2014 at 8:25 PM, Robert Haas <robertmhaas@gmail.com> wrote:

Should RLS be reverted, and revisited in a future CF?

IMHO, that would be entirely appropriate.

That seems pretty straightforward, then. I think that it will have to
be reverted.

but I do feel that Stephen's feelings of being chastised
aren't worth the bits they are printed on.

I believe that Stephen intended to convey taking that on the chin, as
he should - up to a point.

We're here to develop
software together, not to talk about our feelings; and the quality of
the software we produce depends on our willingness to follow a set of
procedures that are or should be well-understood by long-time
contributors, not on our emotional states.

It also depends on trust, and mutual respect. We'd get very little
done without that.

It's difficult to imagine a more flagrant violation of process than
committing a patch without any warning and without even *commenting*
on the fact that clear objections to commit were made on a public
mailing list. If that is allowed to stand, what can we assume other
than that Stephen, at least, has a blank check to change anything he
wants, any time he wants, with no veto possible from anyone else?

I think that this is excessive. Do you really believe that Stephen
thinks that, or means to assert that he has a blank check? If you
don't, then why say it? If you do, then the evidence is pretty thin on
the ground. You say "if this is allowed to stand" as if you think that
it might be - I don't think that it will.

If Stephen had a track record of recklessly committing things, then
I'd understand your use of such strong words. I don't think that he
does, though. You're jumping to the worst possible conclusion about
Stephen's motivations.

That having been said, I did go and look at the RLS thread, and
Stephen's commit did seem to be made in quite a bit more haste than I
first imagined. As someone that has an interest in PostgreSQL's
success in general, and as someone that has to wait for a long time to
get serious review of my work, I must say that that does annoy me. I
submitted a patch in March, that was finally marked "Ready for
Committer" by Michael Paquier about 2 months ago.

--
Peter Geoghegan

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

On 09/23/2014 09:15 AM, Peter Geoghegan wrote:

On Mon, Sep 22, 2014 at 8:25 PM, Robert Haas <robertmhaas@gmail.com> wrote:

Should RLS be reverted, and revisited in a future CF?

IMHO, that would be entirely appropriate.

That seems pretty straightforward, then. I think that it will have to
be reverted.

OTOH, if the patch is actually OK as it was committed, there's no point
reverting it only to commit it again later. At the end of the day, the
important thing is that the patch gets sufficient review. Clearly
Stephen thinks that it did, while you and Andres do not.

To make sure this doesn't just slip by without sufficient review, I'll
add this to the next commitfest. It's a bit unusual to have a commitfest
entry for something that's already been committed, but that's fine.

- Heikki

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

On Mon, Sep 22, 2014 at 11:45 PM, Heikki Linnakangas
<hlinnakangas@vmware.com> wrote:

To make sure this doesn't just slip by without sufficient review, I'll add
this to the next commitfest. It's a bit unusual to have a commitfest entry
for something that's already been committed, but that's fine.

That seems sensible.

--
Peter Geoghegan

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

Some random comments after a quick read-through of the patch:

* Missing documentation. For a major feature like this, reference pages
for the CREATE/DROP POLICY statements are not sufficient. We'll need a
whole new chapter for this.

* In CREATE POLICY, the "USING" and "WITH CHECK" keywords are not very
descriptive. I kind of understand WITH CHECK - it's applied to new rows
like a CHECK constraint - but USING seems rather arbitrary and WITH
CHECK isn't all that clear either. Perhaps "ON SELECT CHECK" and "ON
UPDATE CHECK" or something like that would be better. I guess USING
makes sense when that's the only expression given, so that it applies to
both SELECTs and UPDATEs. But when a WITH CHECK expression is given
together with USING, it gets confusing.

+                       if (is_from)
+                               ereport(ERROR,
+                                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                                                errmsg("COPY FROM not supported with row security."),
+                                                errhint("Use direct INSERT statements instead.")));
+

Why is that not implemented? I think it should be.

* In src/backend/commands/createas.c:

@@ -420,6 +421,19 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
ExecCheckRTPerms(list_make1(rte), true);

/*
+        * Make sure the constructed table does not have RLS enabled.
+        *
+        * check_enable_rls() will ereport(ERROR) itself if the user has requested
+        * something invalid, and otherwise will return RLS_ENABLED if RLS should
+        * be enabled here.  We don't actually support that currently, so throw
+        * our own ereport(ERROR) if that happens.
+        */
+       if (check_enable_rls(intoRelationId, InvalidOid) == RLS_ENABLED)
+               ereport(ERROR,
+                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                                (errmsg("policies not yet implemented for this command"))));
+
+       /*
* Tentatively mark the target as populated, if it's a matview and we're
* going to fill it; otherwise, no change needed.
*/

Hmm. So, if a table we just created with CREATE TABLE AS has row-level
security enabled, we throw an error? Can that actually happen? AFAICS a
freshly-created table shouldn't can't have row-level security enabled.
Or is row-level security enabled/disabled status copied from the source
table?

* Row-level security is not allowed for foreign tables. Why not? It's
perhaps not a very useful feature, but I don't see any immediate reason
to forbid it either. How about views?

* The new pg_class.relhasrowsecurity column is not updated lazily like
most other relhas* columns. That's intentional and documented, but
perhaps it would've been better to name the column differently, to avoid
confusion. Maybe just "relrowsecurity"

* In rewrite/rowsecurity:

+ * Policies can be defined for specific roles, specific commands, or provided
+ * by an extension.

How do you define a policy for a specific role? There's a hook for the
extensions in there, but I didn't see any documentation mentioning that
an extension might provide policies, nor any developer documentation on
how to use the hook.

* In pg_backup_archiver.c:

/*
* Enable row-security if necessary.
*/
if (!ropt->enable_row_security)
ahprintf(AH, "SET row_security = off;\n");
else
ahprintf(AH, "SET row_security = on;\n");

That makes pg_restore to throw an error if you try to restore to a
pre-9.5 server. I'm not sure if using a newer pg_restore against an
older server is supported in general, but without the above it works in
simple cases, at least. It would be trivi

* The new --enable-row-security option to pg_restore is not documented
in the user manual.

* in src/bin/psql/describe.c:

@@ -2529,6 +2640,11 @@ describeRoles(const char *pattern, bool verbose)
appendPQExpBufferStr(&buf, "\n, r.rolreplication");
}

+               if (pset.sversion >= 90500)
+               {
+                       appendPQExpBufferStr(&buf, "\n, r.rolbypassrls");
+               }
+
appendPQExpBufferStr(&buf, "\nFROM pg_catalog.pg_roles r\n");

The rolbypassrls column isn't actually used for anything in this function.

In addition to the above, attached is a patch with some trivial fixes.

- Heikki

Heikki,

* Heikki Linnakangas (hlinnakangas@vmware.com) wrote:

Some random comments after a quick read-through of the patch:

Glad you were able to find a bit of time to take a look, thanks!

* Missing documentation. For a major feature like this, reference
pages for the CREATE/DROP POLICY statements are not sufficient.
We'll need a whole new chapter for this.

Peter mentioned this too and I've been working on adding documentation.
The general capability, at least in my view, is pretty clear but I agree
that examples and adding more about the trade-offs would be useful. Do
you have any opinion regarding additional documentation for updatable
security-barrier views? I'm trying to work out a way to share this new
documentation with that since there is overlap as they share a lot of
the caveats, given that they use the same code underneath.

* In CREATE POLICY, the "USING" and "WITH CHECK" keywords are not
very descriptive. I kind of understand WITH CHECK - it's applied to
new rows like a CHECK constraint - but USING seems rather arbitrary
and WITH CHECK isn't all that clear either. Perhaps "ON SELECT
CHECK" and "ON UPDATE CHECK" or something like that would be better.

Right, WITH CHECK matches up with the SQL standard 'WITH CHECK OPTION'.

I guess USING makes sense when that's the only expression given, so
that it applies to both SELECTs and UPDATEs. But when a WITH CHECK
expression is given together with USING, it gets confusing.

Right, USING was there in the original patch back in January (actually,
it might be farther back, there were versions of the patch prior to
that) while WITH CHECK was added more recently.

+                       if (is_from)
+                               ereport(ERROR,
+                                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                                                errmsg("COPY FROM not supported with row security."),
+                                                errhint("Use direct INSERT statements instead.")));
+

Why is that not implemented? I think it should be.

It's certainly an item that I was planning to look at addressing, though
only for completeness. COPY's focus is providing a faster interface and
the RLS checks would end up dwarfing the overall time and making COPY
not actually be usefully faster than INSERT.

* In src/backend/commands/createas.c:

@@ -420,6 +421,19 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
ExecCheckRTPerms(list_make1(rte), true);

/*
+        * Make sure the constructed table does not have RLS enabled.
+        *
+        * check_enable_rls() will ereport(ERROR) itself if the user has requested
+        * something invalid, and otherwise will return RLS_ENABLED if RLS should
+        * be enabled here.  We don't actually support that currently, so throw
+        * our own ereport(ERROR) if that happens.
+        */
+       if (check_enable_rls(intoRelationId, InvalidOid) == RLS_ENABLED)
+               ereport(ERROR,
+                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                                (errmsg("policies not yet implemented for this command"))));
+
+       /*
* Tentatively mark the target as populated, if it's a matview and we're
* going to fill it; otherwise, no change needed.
*/

Hmm. So, if a table we just created with CREATE TABLE AS has
row-level security enabled, we throw an error? Can that actually
happen?

I don't believe it actually can happen, but I wanted to be sure that the
case was covered in the event that support is added. Essentially, I
carefully went through looking at all of the existing ExecCheckRTPerms()
calls and made sure RLS was handled in all of those cases, in one way or
another. As I expect you noticed, I also updated the comments in
ExecCheckRTPerms() to reflect that RLS needs to be addressed as well as
normal checks when returning tuples or adding new tuples.

AFAICS a freshly-created table shouldn't can't have
row-level security enabled. Or is row-level security
enabled/disabled status copied from the source table?

It's not copied, no. That didn't make a lot of sense to me as we don't
even have an option to copy permissions.

* Row-level security is not allowed for foreign tables. Why not?
It's perhaps not a very useful feature, but I don't see any
immediate reason to forbid it either. How about views?

The question about views came up but with views you could simply add the
quals into the view definition instead of combining views and RLS.
Still, I'm not against it, but the patch certainly seemed large enough
to me that it made sense to move forward with the basic, but complete,
implementation for tables. Foreign tables fell into the same bucket but
perhaps it shouldn't have.. I'll definitely take a look at what the
level of complexity there is.

* The new pg_class.relhasrowsecurity column is not updated lazily
like most other relhas* columns. That's intentional and documented,
but perhaps it would've been better to name the column differently,
to avoid confusion. Maybe just "relrowsecurity"

Ok, fair enough. It had been lazy originally which is where the name
originated from but during the design discussion it was pointed out that
it'd be better to support turning on/off RLS, but I hadn't considered
changing the name for that.

* In rewrite/rowsecurity:

+ * Policies can be defined for specific roles, specific commands, or provided
+ * by an extension.

How do you define a policy for a specific role? There's a hook for
the extensions in there, but I didn't see any documentation
mentioning that an extension might provide policies, nor any
developer documentation on how to use the hook.

For a specific role you use the 'TO <role_list>' option to CREATE
POLICY.. Is that not clear? For extensions, there's documentation
where the hook is defined about how the hook is to be used and how
policies from the hook integrate with policies defined in the catalog.
We don't typically provide documentation beyond that for hooks, do we?
I specifically recall looking for that, to add to it, and not finding
any- indeed, my vague recollection from days gone by is that it was
decided that comments *are* the way to document hooks as any hook
author is almost certainly going to be looking at the code anyway.

* In pg_backup_archiver.c:

/*
* Enable row-security if necessary.
*/
if (!ropt->enable_row_security)
ahprintf(AH, "SET row_security = off;\n");
else
ahprintf(AH, "SET row_security = on;\n");

That makes pg_restore to throw an error if you try to restore to a
pre-9.5 server. I'm not sure if using a newer pg_restore against an
older server is supported in general, but without the above it works
in simple cases, at least. It would be trivi

I didn't think it was generally expected that pg_restore would work
against older versions; indeed, I didn't think we actually queried for
the version as we can't do anything for SQL scripts created by
pg_restore.

Ok, I see we do have a check for PQserverVersion() in
pg_backup_archiver.c to use TRUNCATE TABLE .. ONLY. I'll take a look at
adding a check to skip the SET row_security for 9.4-and-earlier.

* The new --enable-row-security option to pg_restore is not
documented in the user manual.

Argh. Right, sorry about that, it was added to pg_dump, of course, but
the two are (obviously) not the same.

* in src/bin/psql/describe.c:

@@ -2529,6 +2640,11 @@ describeRoles(const char *pattern, bool verbose)
appendPQExpBufferStr(&buf, "\n, r.rolreplication");
}

+               if (pset.sversion >= 90500)
+               {
+                       appendPQExpBufferStr(&buf, "\n, r.rolbypassrls");
+               }
+
appendPQExpBufferStr(&buf, "\nFROM pg_catalog.pg_roles r\n");

The rolbypassrls column isn't actually used for anything in this function.

Yup, Coverity noted that and it's on my list of things to address. I do
wish we could get Coverity and buildfarm coverage ahead of commits, but
that's a discussion for a different thread..

In addition to the above, attached is a patch with some trivial fixes.

Great, thanks!

Stephen

On Tue, Sep 23, 2014 at 4:25 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Sep 22, 2014 at 7:22 PM, Peter Geoghegan <pg@heroku.com> wrote:

On Mon, Sep 22, 2014 at 4:02 PM, Andres Freund <andres@anarazel.de> wrote:

This patch has been pushed in a clear violation of established policy.

Fundamental pieces of the patch have changed *after* the commitfest
started. And there wasn't a recent patch in the commitfest either - the
entry was moved over from the last round without a new patch. It didn't
receive independent review (Robert explicitly said his wasn't a full
review). It wasn't marked ready for committer. The intention to commit
wasn't announced publically. There were *clear* and unaddressed
objections to committing the patch as is, by a committer (Robert)
nonetheless.

I have no reason to doubt your version of events here

Fortunately, you don't have to take anything on faith. This is a
public mailing list, and the exact sequence of events is open to
inspection by anyone who cares to take a few minutes to do so. You
can easily verify whether my statement that I asked Stephen twice to
hold off committing it is correct or not; and you can also verify the
rest of the history that Andres and I recounted. This is all there in
black and white.

Just to be clear here, the *only* issue we should even be discussing
is whether the patch should or should not have been committed in the
face of those objections. As Josh has also noted, the commitfest
process was never meant to constrain what committers do or when they
do it with their own patches or ones they've worked heavily on. They
are there as a backstop to make sure that regardless of what the
committers are doing day to day, patch authors know that their patch
is expected to receive some review within N weeks.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On 2014-09-23 13:23:32 +0100, Dave Page wrote:

Just to be clear here, the *only* issue we should even be discussing
is whether the patch should or should not have been committed in the
face of those objections. As Josh has also noted, the commitfest
process was never meant to constrain what committers do or when they
do it with their own patches or ones they've worked heavily on. They
are there as a backstop to make sure that regardless of what the
committers are doing day to day, patch authors know that their patch
is expected to receive some review within N weeks.

FWIW, while not really at the core of the problem here, I don't think
this is entirely true anymore.

We certainly seem to to expect bigger feature patches to go through the
commitfest process to some degree. Just look at the discussions about
*committers* patches being committed or not at each cycles last
commitfest. Every single time the point in time they've been submitted
to which CF plays a rather prominent role in the discussion.

Also look at committers like Robert that *do* feel constrained about
when to commit or even expect review for submitted patches.

I think it's obvious that a committer doesn't need to wait till some
later commitfest to commit patches that have since gotten enough review
or are uncontroversial. Neither is the case here.

I also think committers need to be much more careful when committing
patches which they (or their employer) appear to have a business
interest in. Rushing ahead to commit the patch of somebody 'unrelated'
leaves a completely different taste than committing your colleagues
patch. *INDEPENDENT* of the actual reasons and the state of the patch.

Perhaps we can use this as a chance to make the review process a bit
better defined. There certainly have been a few patches by commiters
over the last years that have had a noticeable negative impact. Some of
which might have been cought by more diligent review.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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

On 2014-09-22 21:38:17 -0700, David G Johnston wrote:

Robert Haas wrote

It's difficult to imagine a more flagrant violation of process than
committing a patch without any warning and without even *commenting*
on the fact that clear objections to commit were made on a public
mailing list. If that is allowed to stand, what can we assume other
than that Stephen, at least, has a blank check to change anything he
wants, any time he wants, with no veto possible from anyone else?

I'm of a mind to agree that this shouldn't have been committed...but I'm not
seeing where Stephen has done sufficient wrong to justify crucifixion.

I've not seen much in the way of 'crucifixion' before this email. And I
explicitly *don't* think it's warranted. Also it's not happening.

At this point my hindsight says a strictly declaratory statement of "reasons
this is not ready" combined with reverting the patch would have been
sufficient; or even just a "I am going to revert this for these reasons"
post. The difference between building support for a revert and gathering a
mob is a pretty thin line.

The reason it's being discussed is to find a way to align the different
views about when to commit stuff. The primary goal is *not* to revert
the commit or anything but to make sure we're not slipping into
procedures we all would regret. Which *really* can happen very
easily. We're all humans and most of us have more than enough to do.

Though I guess if you indeed feel that his actions were truly heinous you
should also then put forth the proposal that his ability to commit be
revoked.

I think *you* are escalating this to something unwarranted here by the
way you're painting the discussion.

If your not willing to go to that extent then, unless you know
Stephen personally, I'd not assume that public flogging is the best way to
get him to not mess up in the future; but the honest and cordial dialog
about cause/effect is likely to be more productive and less
self-destructing. Though, to each their own style.

All the people involved know Stephen personally. There's also no "public
flogging".

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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

On Tue, Sep 23, 2014 at 9:00 AM, Andres Freund <andres@anarazel.de> wrote:

I think it's obvious that a committer doesn't need to wait till some
later commitfest to commit patches that have since gotten enough review
or are uncontroversial. Neither is the case here.

Right. I mean, the occasionally-floated notion that committers can't
commit things when there's no CommitFest ongoing is obviously flat
wrong, as a quick look at the commit log will demonstrate.

On the flip side, it's unreasonable to expect that as soon as a
committer posts 7000-line patch, everyone who possibly has an
objection to that patch will drop everything that they are doing to
object to anything they don't like about it. It can easily take a
week to review such a patch thoroughly even if you start the minute it
hits the list.

Many committers, including me, have held off on committing patches for
many months to make sure that the community got time to review them,
even though nobody explicitly objected. This patch was committed much
faster than that and over an explicit objection.

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

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

On Tue, Sep 23, 2014 at 12:38 AM, David G Johnston
<david.g.johnston@gmail.com> wrote:

I'm of a mind to agree that this shouldn't have been committed...but I'm not
seeing where Stephen has done sufficient wrong to justify crucifixion.
Especially since everything is being done publicly and you are one of the
many people in the position to flex a veto by reverting the patch.

I'd be interested to see what the reaction would be if I reverted this
patch out of the blue. I suspect it would not be positive. More
generally, I don't want the PostgreSQL source code repository to
ground zero for a revert war.

Subsequent, possibly private, discussion between you and Stephen could then
occur before making any conclusions you form public so that others can learn
from the experience and ponder whether anything could be changed to mitigate
such situations in the future.

I'd be happy to discuss this with Stephen, either in person, by phone,
or over public or private email. Unfortunately, although he's posted
many other emails to this list over the last couple of days, he hasn't
chosen to respond, publicly or privately, to my statement that he must
have read the email in which I asked him to hold off committing
(because he addressed technical feedback from that same email) and
that he went ahead and did it anyway.

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

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

On Tue, Sep 23, 2014 at 9:09 AM, Andres Freund <andres@2ndquadrant.com>
wrote:

On 2014-09-22 21:38:17 -0700, David G Johnston wrote:

Robert Haas wrote

It's difficult to imagine a more flagrant violation of process than
committing a patch without any warning and without even *commenting*
on the fact that clear objections to commit were made on a public
mailing list. If that is allowed to stand, what can we assume other
than that Stephen, at least, has a blank check to change anything he
wants, any time he wants, with no veto possible from anyone else?

I'm of a mind to agree that this shouldn't have been committed...but I'm

not

seeing where Stephen has done sufficient wrong to justify crucifixion.

I've not seen much in the way of 'crucifixion' before this email. And I
explicitly *don't* think it's warranted. Also it's not happening.

I maybe got a little carried​ away with my hyperbole...

At this point my hindsight says a strictly declaratory statement of

"reasons

this is not ready" combined with reverting the patch would have been
sufficient; or even just a "I am going to revert this for these reasons"
post. The difference between building support for a revert and

gathering a

mob is a pretty thin line.

The reason it's being discussed is to find a way to align the different
views about when to commit stuff. The primary goal is *not* to revert
the commit or anything but to make sure we're not slipping into
procedures we all would regret. Which *really* can happen very
easily. We're all humans and most of us have more than enough to do.

​So, the second option then...​and I'm sorry but "this should never have
been committed" tends to cause one to think it should therefore be reverted.

Though I guess if you indeed feel that his actions were truly heinous you
should also then put forth the proposal that his ability to commit be
revoked.

I think *you* are escalating this to something unwarranted here by the
way you're painting the discussion.

​Not everyone who reads -hackers knows all the people involved personally.
I had an initial reaction to these e-mails that I thought I would share,
nothing more. I'm not going to quote the different comments that led me to
my feeling that the response to this was disproportionate to the offense
but after a first pass - which is all many people would do - that is what I
came away with. Though you could say I fell into the very same trap by
reacting off my first impression...

David J.

On Tue, Sep 23, 2014 at 9:14 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Sep 23, 2014 at 12:38 AM, David G Johnston
<david.g.johnston@gmail.com> wrote:

I'm of a mind to agree that this shouldn't have been committed...but I'm

not

seeing where Stephen has done sufficient wrong to justify crucifixion.
Especially since everything is being done publicly and you are one of the
many people in the position to flex a veto by reverting the patch.

I'd be interested to see what the reaction would be if I reverted this
patch out of the blue. I suspect it would not be positive. More
generally, I don't want the PostgreSQL source code repository to
ground zero for a revert war.

​OK, so the actually pulling-the-trigger doesn't have to occur (though it
can if warranted but obviously usually this particular procedure is
sufficiently slow that the need for immediate action would be rare) but
framing the argument in that context focuses the attention on the patch
itself not being ready and moves it away from the personal aspects of why
it was committed in the first place.

Subsequent, possibly private, discussion between you and Stephen could

then

occur before making any conclusions you form public so that others can

learn

from the experience and ponder whether anything could be changed to

mitigate

such situations in the future.

I'd be happy to discuss this with Stephen, either in person, by phone,
or over public or private email. Unfortunately, although he's posted
many other emails to this list over the last couple of days, he hasn't
chosen to respond, publicly or privately, to my statement that he must
have read the email in which I asked him to hold off committing
(because he addressed technical feedback from that same email) and
that he went ahead and did it anyway.

Not checking timeline here but Stephen has said he was in the wrong and
that the reasons would be better discussed at the pub (i.e., offline). Is
a more vocal public admission of guilt the important thing here?

​David J.
​

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:

I'd be happy to discuss this with Stephen, either in person, by phone,
or over public or private email.

Please understand that I'm not ignoring you, the conversation, or the
concern. I was asked (by other community members) to not comment on the
thread and I have been trying to hold to that.

Posted to the list so others are aware.

I'd be more than happy to discuss it over the phone, or in person, as I
offered to do.

Thanks,

Stephen