Re: [HACKERS] RLS feature has been committed
On 09/26/2014 01:07 AM, Stephen Frost wrote: * Simon Riggs (si...@2ndquadrant.com) wrote: My major reason to revert is the following: the documentation contains no examples of real world usage, making the feature essentially unusable out of the box. I find this to be an interesting argument considering most of our documentation doesn't include real-world examples. +1 for adding examples. This wouldn't be the only case of documentation (indeed, *any* documentation) being added after a commit, and so I'm mystified by this requirement for *real-world* examples in documentation to be provided prior to commit. IMO it depends on the situation. Yeah, we've had a lot of commits without docs, where the documentation has been added later. I'm OK with that, if for example the committed patch is part of a series of patches, where it doesn't make sense to add documentation until the whole series has been committed. Then there are situations where the patch author just hasn't gotten around to adding documentation yet (like in this case). That's more questionable; documentation is an important part of adding a feature, and there's the risk that the author never gets around to add documentation, after the code has been committed, or does only lip service to it. It's usually worked out fine, though - people who have successfully added major features are conscientious enough to get it done. But in many cases, lack of good documentation make even reviewing the patch difficult. How do you determine if the patch works as intended, if you don't know what it's supposed to do? Wrt. reverting or not, I'm strongly of the opinion that whatever. If you just add the docs, I'm happy. This is a big and security-sensitive feature, and it requires documentation not only on how to use the feature, but an introduction to what row-level security is, what level of security to expect, what circumstances you would use it in, comparison to other approaches, the side-channels, etc. We'll probably have to go through a few rounds of review on the docs alone. I'd leave it up to Stephen to decided how he wants to handle this. But I'd really like to see the documentation added (at least posted as a patch if not committed), well before the next commitfest, so that Robert and others who want to review the code, have the documentation available. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] RLS feature has been committed
On 26 September 2014 08:48, Heikki Linnakangas hlinnakan...@vmware.com wrote: But in many cases, lack of good documentation makes even reviewing the patch difficult. How do you determine if the patch works as intended, if you don't know what it's supposed to do? Exactly. Lack of review and lack of consensus are often caused by the author not making the patch fully and genuinely accessible to peer review. Don't say you're having problems getting buy in when you've done very little to encourage that. Committing early is not the solution. -- Simon Riggs 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
Re: [HACKERS] RLS feature has been committed
On Fri, Sep 26, 2014 at 5:12 AM, Simon Riggs si...@2ndquadrant.com wrote: On 26 September 2014 08:48, Heikki Linnakangas hlinnakan...@vmware.com wrote: But in many cases, lack of good documentation makes even reviewing the patch difficult. How do you determine if the patch works as intended, if you don't know what it's supposed to do? Exactly. Lack of review and lack of consensus are often caused by the author not making the patch fully and genuinely accessible to peer review. Don't say you're having problems getting buy in when you've done very little to encourage that. Committing early is not the solution. That is quite true. Furthermore, in this particular case, I had already put a lot of effort into reviewing the patch and had expressed a clear intention to put in more. If the worst that happens is that the patch has a few bugs, no great harm will have been done by committing it. Things get a lot more thorny if there are still design-level issues. I think we made a lot of progress on those issues in previous rounds of review, but I'm not sure we squashed them all, and I didn't appreciate having that process short-circuited. -- 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
Re: [HACKERS] RLS feature has been committed
On 23 September 2014 07:45, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 09/23/2014 09:15 AM, Peter Geoghegan wrote: On Mon, Sep 22, 2014 at 8:25 PM, Robert Haas robertmh...@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. I would observe that not requesting a revert would be inconsistent against all other situations I have seen or been involved with. Stephen has acted against explicit requests not to commit. I think Robert should apply more care about issuing lock instructions, since the concurrency of our community is important, but nonetheless, an explicit request was made and that should be honored, even if it does very quickly get re-committed. My major reason to revert is the following: the documentation contains no examples of real world usage, making the feature essentially unusable out of the box. I attended Stephen's talk at PostgesOpen and even that didn't contain real cases either, leaving me to ask questions about stuff I thought I knew. This would be sufficient for me to reject commit of any other patch, so it is sufficient reason here also. Yeb can supply a useful real world case if there is some restriction on explaining what this might be used for. Stephen, I want this patch in 9.5 and I would very much like to see it go in before Oct 31. But please follow consensus, revert the patch now, address the minor issues as requested and then ask for re-commit later, as you should have done in the first place. -- Simon Riggs 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
Re: [HACKERS] RLS feature has been committed
Simon, * Simon Riggs (si...@2ndquadrant.com) wrote: On 23 September 2014 07:45, Heikki Linnakangas hlinnakan...@vmware.com wrote: 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. I would observe that not requesting a revert would be inconsistent against all other situations I have seen or been involved with. A revert wasn't requested by the individual who raised the concern (or, indeed, explicitly by *anyone*.. it was hinted at, but I felt the individuals who were hinting at it were leaving it up to that individual who had a concern), and I discussed it with him extensively and we came to what I believe is an understanding. I'm not sure that I understand the need to bring it up again, unless you have a concern regarding what was committed. I agree that I jumped the gun on it and said as much. On the flip side, it's not had quite a bit of review and there is a promise of more, which I feel is great for the project as a whole. My major reason to revert is the following: the documentation contains no examples of real world usage, making the feature essentially unusable out of the box. I find this to be an interesting argument considering most of our documentation doesn't include real-world examples. I'm happy to add more examples than those listed thus far, certainly, though I've understood documentation to be of less general importance than code quality and maintainability- and someone willing and committed to maintain it. I attended Stephen's talk at PostgesOpen and even that didn't contain real cases either, leaving me to ask questions about stuff I thought I knew. This would be sufficient for me to reject commit of any other patch, so it is sufficient reason here also. Yeb can supply a useful real world case if there is some restriction on explaining what this might be used for. This wouldn't be the only case of documentation (indeed, *any* documentation) being added after a commit, and so I'm mystified by this requirement for *real-world* examples in documentation to be provided prior to commit. Stephen, I want this patch in 9.5 and I would very much like to see it go in before Oct 31. But please follow consensus, revert the patch now, address the minor issues as requested and then ask for re-commit later, as you should have done in the first place. Simon, if you want me to revert it because of an objection over the design, code quality, maintainability, or utter lack of documentation, then I absolutely respect that request and would be happy to do so. This request I am completely lost on. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] RLS feature has been committed
On Thu, Sep 25, 2014 at 3:07 PM, Stephen Frost sfr...@snowman.net wrote: A revert wasn't requested by the individual who raised the concern (or, indeed, explicitly by *anyone*.. it was hinted at, but I felt the individuals who were hinting at it were leaving it up to that individual who had a concern), and I discussed it with him extensively and we came to what I believe is an understanding. I'm not sure that I understand the need to bring it up again, unless you have a concern regarding what was committed. FWIW, I think Heikki's proposal to revisit this in the next commitfest is fine. I don't think that this is something that needs to be pored over. -- 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
Re: [HACKERS] RLS feature has been committed
Heikki, * Heikki Linnakangas (hlinnakan...@vmware.com) wrote: Some random comments after a quick read-through of the patch: Many thanks for this, again. I've pushed updates along with the fix for relcache which was identified by the buildfarm. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] RLS feature has been committed
On 09/23/2014 09:15 AM, Peter Geoghegan wrote: On Mon, Sep 22, 2014 at 8:25 PM, Robert Haas robertmh...@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
Re: [HACKERS] RLS feature has been committed
On Mon, Sep 22, 2014 at 11:45 PM, Heikki Linnakangas hlinnakan...@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
Re: [HACKERS] RLS feature has been committed
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
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 76d6405..bae08ee 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1943,6 +1943,7 @@
row
entrystructfieldrelhasrowsecurity/structfield/entry
entrytypebool/type/entry
+ entry/entry
entry
True if table has row-security enabled; see
link linkend=catalog-pg-rowsecuritystructnamepg_rowsecurity/structname/link catalog
diff
Re: [HACKERS] RLS feature has been committed
Heikki, * Heikki Linnakangas (hlinnakan...@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
Re: [HACKERS] RLS feature has been committed
On Tue, Sep 23, 2014 at 4:25 AM, Robert Haas robertmh...@gmail.com wrote: On Mon, Sep 22, 2014 at 7:22 PM, Peter Geoghegan p...@heroku.com wrote: On Mon, Sep 22, 2014 at 4:02 PM, Andres Freund and...@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
Re: [HACKERS] RLS feature has been committed
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
Re: [HACKERS] RLS feature has been committed
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
Re: [HACKERS] RLS feature has been committed
On Tue, Sep 23, 2014 at 9:00 AM, Andres Freund and...@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
Re: [HACKERS] RLS feature has been committed
On Tue, Sep 23, 2014 at 12:38 AM, David G Johnston david.g.johns...@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
Re: [HACKERS] RLS feature has been committed
On Tue, Sep 23, 2014 at 9:09 AM, Andres Freund and...@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.
Re: [HACKERS] RLS feature has been committed
Robert, * Robert Haas (robertmh...@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 signature.asc Description: Digital signature
Re: [HACKERS] RLS feature has been committed
On Tue, Sep 23, 2014 at 2:00 PM, Andres Freund and...@anarazel.de wrote: 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. I'm not aware that we've made any such changes since the process was originally developed. The fact that developers may constrain their own review/commit work to certain periods is a personal choice, not policy or requirement. 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. They should be tracked on the app certainly, but that doesn't prevent review/commits being made outside of the commitfest. Also look at committers like Robert that *do* feel constrained about when to commit or even expect review for submitted patches. Regardless of what Robert may feel, review should only generally be *expected* during a commitfest, but it can be done at any time. Committers are free to commit at any time. The process was never intended to restrict what committers do or when - in fact when I introduced the process to -hackers, it was specifically worded to say that developers are strongly encouraged to take part in the commitfest reviews, but not forced to, and may continue to work on their patches as they see fit. -- 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
Re: [HACKERS] RLS feature has been committed
On Tue, Sep 23, 2014 at 11:16 AM, Dave Page dp...@pgadmin.org wrote: Regardless of what Robert may feel, review should only generally be *expected* during a commitfest, but it can be done at any time. Committers are free to commit at any time. The process was never intended to restrict what committers do or when - in fact when I introduced the process to -hackers, it was specifically worded to say that developers are strongly encouraged to take part in the commitfest reviews, but not forced to, and may continue to work on their patches as they see fit. So, just to be clear, you're saying that committers are free to commit things even when other community members (who may themselves be committers) ask them not to? -- 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
Re: [HACKERS] RLS feature has been committed
On Tue, Sep 23, 2014 at 4:19 PM, Robert Haas robertmh...@gmail.com wrote: On Tue, Sep 23, 2014 at 11:16 AM, Dave Page dp...@pgadmin.org wrote: Regardless of what Robert may feel, review should only generally be *expected* during a commitfest, but it can be done at any time. Committers are free to commit at any time. The process was never intended to restrict what committers do or when - in fact when I introduced the process to -hackers, it was specifically worded to say that developers are strongly encouraged to take part in the commitfest reviews, but not forced to, and may continue to work on their patches as they see fit. So, just to be clear, you're saying that committers are free to commit things even when other community members (who may themselves be committers) ask them not to? At what point did I say anything like that? Do not twist my words. -- 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
Re: [HACKERS] RLS feature has been committed
On 2014-09-23 16:16:18 +0100, Dave Page wrote: On Tue, Sep 23, 2014 at 2:00 PM, Andres Freund and...@anarazel.de wrote: 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. I'm not aware that we've made any such changes since the process was originally developed. The fact that developers may constrain their own review/commit work to certain periods is a personal choice, not policy or requirement. I do think that it has widely lead to a bit more formal expectance of committers patches being reviewed by others. 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. They should be tracked on the app certainly, but that doesn't prevent review/commits being made outside of the commitfest. And I've explicitly stated that I don't believe that they should be. Also look at committers like Robert that *do* feel constrained about when to commit or even expect review for submitted patches. Regardless of what Robert may feel, review should only generally be *expected* during a commitfest, but it can be done at any time. I think you're misunderstanding my point here. I would never ever protest against *more* reviews. Stephen quoted the delay of getting review as a reason for committing the patch at the point he did. And that seems unwarranted, because the current form of the patch (which is significantly different!) was only posted in the middle of the commitfest. The reason I mentioned the commitfest is that he had couldn't expect a review in the couple of days in which the patch existed in the current form - because other reviewers were still trying to keep up (and failing!) with the stuff that was submitted to the commitest. 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
Re: [HACKERS] RLS feature has been committed
On 09/23/2014 11:19 AM, Robert Haas wrote: On Tue, Sep 23, 2014 at 11:16 AM, Dave Page dp...@pgadmin.org wrote: Regardless of what Robert may feel, review should only generally be *expected* during a commitfest, but it can be done at any time. Committers are free to commit at any time. The process was never intended to restrict what committers do or when - in fact when I introduced the process to -hackers, it was specifically worded to say that developers are strongly encouraged to take part in the commitfest reviews, but not forced to, and may continue to work on their patches as they see fit. So, just to be clear, you're saying that committers are free to commit things even when other community members (who may themselves be committers) ask them not to? I don't think anyone is suggesting that. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] RLS feature has been committed
On Tue, Sep 23, 2014 at 11:23 AM, Dave Page dp...@pgadmin.org wrote: On Tue, Sep 23, 2014 at 4:19 PM, Robert Haas robertmh...@gmail.com wrote: On Tue, Sep 23, 2014 at 11:16 AM, Dave Page dp...@pgadmin.org wrote: Regardless of what Robert may feel, review should only generally be *expected* during a commitfest, but it can be done at any time. Committers are free to commit at any time. The process was never intended to restrict what committers do or when - in fact when I introduced the process to -hackers, it was specifically worded to say that developers are strongly encouraged to take part in the commitfest reviews, but not forced to, and may continue to work on their patches as they see fit. So, just to be clear, you're saying that committers are free to commit things even when other community members (who may themselves be committers) ask them not to? At what point did I say anything like that? Do not twist my words. Well, if that's not what you're saying, then good, because I sure as heck don't think that. I think it's the role of a committer to commit things that are generally agreed to be good ideas, not things that that particular committer personally thinks are a good idea regardless of the opinions of others. The committer is entitled to weigh their own opinion more heavily than the opinions of others because, hey, that's why they have the commit bit and the other people don't. But they're not entitled to run roughshod over contrary opinions. Now, the way that CommitFests come into it is that people can't do two things at once. It's completely reasonable, in my opinion, for me to say, hey look, I really want to review your patch but I don't have time to do that right now because we're in the middle of a CommitFest; please therefore submit it to the next CommitFest. And if I say that, the person should either (1) do it or (2) disagree with the necessity of doing it not (3) commit anyway. Whether you've been paying attention or not, most committer patches ARE submitted to CommitFest and go through the exact same review process as patches from non-committers. That's a good thing. It contributes to our delivery of quality software, and it reduces arguments about whether something was rushed through. There are appropriate times to leapfrog that process, when it's clear that the patch is uncontroversial and that delay is merely dithering. But this is not one of those times. It's a giant patch implementing a complex feature for which more review time was explicitly requested. -- 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
Re: [HACKERS] RLS feature has been committed
On 9/23/14, 9:00 AM, Andres Freund wrote: 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. I haven't been doing much personal development work around here lately, but I did more than a little of the planning on how to handle this as responsibly as I felt it deserved. I think this is worth talking about a little bit because this whole topic, how to deal with a funded project where the committer is also a contributor, isn't something a lot of people have visibility into. (Not talking about you, Andres, you know the deal) This commit didn't happen until I first succeeded in getting Stephen Frost fully funded as a community PostgreSQL contributor (for this job, as well as others with a broader community appeal). Everyone attached to the budget side very explicitly understands that includes an open-ended responsibility to the community for addressing fall-out from RLS, going from unforeseen issues to revisiting the things left on the known open items list. It's hard to reach perfect before commit; eventually you just have to just shoot and see what happens. Just to be really safe, I also kicked off training a whole new PostgreSQL contributor (Adam Brightwell) five months ago, so that by the time we got to here he's also making his own contributions to the security code of the database. And that's included exercises tracking down bugs in the early RLS POC deployments already going on here at Crunchy, so that Stephen isn't the sole Postgres community expertise bottleneck on the code even at this company. (I am NOT on the hook for fixing RLS bugs) The decision on when to commit was strictly Stephen's. During our last chat, we agreed this seemed like an ideal time of year in the development cycle for such a thing to land though. 9.5 is a fresh branch, and there is plenty of time to clean up and/or revert if that's the unfortunate end for anything going in today. Plus the ongoing CommitFest schedule means everyone in the community already is arranging to provide review resources needed in the near future pipeline. I can understand that Robert feels a procedural sting and/or slight due to the exact sequence of events here, and I'm staying out of that. But in general, I don't know what we could have done much better to be a responsible contributor, to finally push this long in development feature over the finish line. A description like rushing ahead would feel inappropriate to me for this one, given how much care went into timing even roughly when this particular commit should happen. The time of year in particular was very specifically aimed at for minimal disruption, based on both major version release dates and the related community development cycle around them. -- Greg Smith greg.sm...@crunchydatasolutions.com Chief PostgreSQL Evangelist - http://crunchydatasolutions.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] RLS feature has been committed
On 09/22/2014 08:23 PM, Amit Kapila wrote: 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. Well, outside of the CF process, it becomes up to the committer to get adequate review of the patch so it can be committed, and adequate review in one of those cases generally means another committer who didn't work on the patch previously. It's also standard for our committers, up to and including Tom Lane, to say things like I'm going to commit this in 24 hours if nobody objects further. 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. That's certainly what it looks like to me, and on that basis Stephen should have held back the patch until he got reviewer OK. Fortunately, since we use Git and not CVS, reverting patches isn't the trauma it once was. -- 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
Re: [HACKERS] RLS feature has been committed
All, Robert and I had a great discussion on the phone where we were both able to voice our concerns and feelings about RLS being pushed. By way of summary, we agree that it was pushed ahead of its time and that it should have waited for at least another review, which likely would have happened even prior to the October commitfest. I've apologized for jumping the gun on pushing it, it wasn't the best move for the project as a whole and I realize that now. Having other committers review and +1 a large patch before moving forward with it is important, even if there is already consensus on the overall design, as it will help minimize the bugs and issues in PostgreSQL. Thanks, Stephen signature.asc Description: Digital signature
[HACKERS] RLS feature has been committed
Hi, I'm posting my reply to Stephen's mail at http://archives.postgresql.org/message-id/20140919163839.GH16422%40tamriel.snowman.net in a new thread because I think it's a important discussion and many people probably stopped following the RLS thread at some point. On 2014-09-19 12:38:39 -0400, Stephen Frost wrote: * Robert Haas (robertmh...@gmail.com) wrote: I specifically asked you to hold off on committing this until there was adequate opportunity for review, and explained my reasoning. You committed it anyway. Hum- my apologies, I honestly don't recall you specifically asking for it to be held off indefinitely. :( There was discussion back and forth, quite a bit of it with you, and I thank you for your help with that and certainly welcome any additional comments. This patch, on the other hand, was massively revised after the start of the CommitFest after many months of inactivity and committed with no thorough review by anyone who was truly independent of the development effort. It was then committed with no warning over a specific request, from another committer, that more time be allowed for review. I would not (nor do I feel that I did..) have committed it over a specific request to not do so from another committer. I had been hoping that there would be another review coming from somewhere, but there is always a trade-off between waiting longer to get a review ahead of a commit and having it committed and then available more easily for others to work with, review, and generally moving forward. c.f. http://archives.postgresql.org/message-id/CA%2BTgmoaX%2BptioOxx42rxJxsgrvxPfUVyndkpeR0JsRiTeZ36Ng%40mail.gmail.com I'm really disappointed by that. I feel I'm essentially getting punished for trying to follow what I understand to the process, which has involved me doing huge amounts of review of other people's patches and waiting a very long time to get my own stuff committed, while you bull ahead with your own patches. While I wasn't public about it, I actually specifically discussed this question with others, a few times even, to try and make sure that I wasn't stepping out of line by moving forward. That said, I do see that Andres feels similairly. It certainly wasn't my intent to surprise anyone by it but simply to continue to move forward- in part, to allow me to properly break from it and work on other things, including reviewing other patches in the commitfest. I fear I've simply been overly focused on it these past few weeks, for a variety of reasons that would likely best be discussed at the pub. I've now slept a couple days on this. And my position hasn't changed. 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. It'd be a somewhat different thing if this weren't about a security sensitive feature, the patch only needed minor cleanup after the start of the CF, or there at least had been constant public progress over the last months. Neither is the case. The circumstances around this being committed make me deeply uncomfortable. It's setting a very bad precedent. I don't think we want to go down that route. All-in-all, I feel appropriately chastised and certainly don't wish to be surprising fellow committers. Perhaps we can discuss at the dev meeting. I really don't know what you understand under appropriately chastised - but I think there's a pretty obvious way to do handle this appropriately. And waiting till the dev meeting obviously makes the whole discussion moot. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] RLS feature has been committed
On Mon, Sep 22, 2014 at 4:02 PM, Andres Freund and...@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
Re: [HACKERS] RLS feature has been committed
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
Re: [HACKERS] RLS feature has been committed
On Tue, Sep 23, 2014 at 6:55 AM, Josh Berkus j...@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
Re: [HACKERS] RLS feature has been committed
On Mon, Sep 22, 2014 at 7:22 PM, Peter Geoghegan p...@heroku.com wrote: On Mon, Sep 22, 2014 at 4:02 PM, Andres Freund and...@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
Re: [HACKERS] RLS feature has been committed
On Mon, Sep 22, 2014 at 9:25 PM, Josh Berkus j...@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
Re: [HACKERS] RLS feature has been committed
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
