Re: [COMMITTERS] pgsql: Add pg_audit, an auditing extension

Robert,

Thank you for your response.

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Tue, May 26, 2015 at 8:10 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > I certainly welcome review from others and if there is not another
> > committer-level formal review before we get close on 9.5 (say, end of
> > August), then I'll revert it.  There is certainly no concern that doing
> > so would be difficult to do, as it is entirely self-contained.
>
> Like Noah, I'm unhappy that this patch went in.  I stated a few days
> before it was committed that I felt the code quality and documentation
> were well below the level we normally expect.  Your response was to do
> a little more work on the patch and then commit it without so much as
> reposting it.  Clearly, we have completely different ideas about what
> constitutes an appropriate response to the sort of comments I sent.

Just to be clear, I understand you're referring to this email:

CA+TgmobtrK9ndfBeAhui1KDKpnUp4cNv07KukD73jOAfd=4dUg@mail.gmail.com

David and I both spent a great deal of time reviewing that email and
working to address your comments.  I had David do as much of it as I
felt that I could, as it is a learning experience for him as he works to
become another dedicated PostgreSQL developer for the community.  He
then passed it on to me, and I took it further and he reviewed the
results of my changes.

To clarify, that email noted style issues (which I worked to fix,
though I should have run pgindent, of course..  the discussed
'make indent' option would help a great deal with that), concerns about
the design limitations (server crashes, using the permission system),
a similar comment regarding where log messages go to the comment in the
code which has had much discussion due to it, and issues with the
documentation and comments.  I did spend a good bit of time working to
improve the comments, but did not focus on the documentation as that
could be done post-feature freeze.

Your comment at the bottom of that email and the subsequent lack of any
other feedback from you or others regarding concern when I voiced my
intention to work on pg_audit for 9.5 in response to Tom's summary
thread lead me to believe that a full review by a qualified committer,
with appropriate improvement to the comments and code, would satisfy the
process requirements to move forward.  There was no hidden agenda here
or an attempt to hold off until the last minute, nor was it any secret
that I was working on it.  There was another, much larger, feature added
to core, rather than simply a contrib module, which I have concerns may
have issues too, but I don't believe that there was any problem with the
process there either.  The concerns were raised by one committer, a
review and quite large rework was done by another, in conjunction with
the author, and then the patch was committed.

I agree that the documentation needs improvement.  I don't agree with
your assessment that the code quality is well below the level we
normally expect, as committed.  That's clearly a difficult thing to
judge, hence my compromise proposal to ensure that it either has a full
review or gets reverted and not included in a released version.

> A near-record number of committers have objected to this patch in all
> kinds of different ways.  There have been concerns about process,
> concerns about code quality, and concerns about whether this is really
> something that we want.  The normal way that works is that you either
> (a) revise the patch or (b) try to convince the person raising the
> concern that it's OK.  The concern is addressed when the person who
> raised it says it is, and not just because you thought about it and
> decided it was OK.  This can be taken too far in the other direction,
> but we're not close to that in this instance.

I worked to address all of those concerns, including those raised by the
individuals you noted in our private discussion, all of whom have been
silent on this since before it went in.  There was clear agreement by
a number of individuals on the need for the broader capability, but I
don't believe that's going to spring forth and land in the tree in one
fell swoop.  This is a good step in that direction.  The patch was
revised numerous times and I did work to convince the individuals
raising concerns that it was OK.  I don't believe there is any doubt
about either of those, but I would be happy to go through the archives
and point out why I feel that my statements are accurate.

> I am particularly troubled by the fact that what has happened with
> this patch is very much like what happened with row-level security: a
> patch that clearly wasn't finished and clearly had not had adequate
> review got abruptly committed - by you - without any consensus so to
> do.  It seems likely to me that by now row-level security has had
> enough review that it is solid enough to be in the tree - in
> particular, I am encouraged by Dean Rasheed's work to improve the
> patch.  However, it is absolutely not the community process for that
> stuff to happen after the code is already in the tree.  It is the
> community process for that stuff to happen before the code is in the
> tree.

I agreed with you regarding row level security and we had a great
discussion about how it's a large feature which needed more review than
most and, further, that it's important us to all realize that we are
custodians of the whole tree and the patches which we commit in
particular and that being a committer is about being trusted to maintain
and address bugs and issues which arise from what we have committed,
across the years.  I have been in this community for a long time, nearly
all of that time in a purely volunteer fashion, working as best I could
in my spare time.  Should things change for me in the future, though I
truely hope they don't, I will continue to fulfill my obligations to the
community in my role as a committer to the best of my abilities.

> It will be impossible for our community to continue delivering quality
> software if you persist in taking this approach.  Either the other
> committers will have to spend an inordinate effort fixing (or
> convincing you to fix) the stuff you break - instead of working on our
> own projects - and other people's patches - or we will have to ignore
> your commits, and the things that are broken in those commits will
> become bugs in our releases.  Either way, the community loses.

I find it confusing that there is no appreciation here for the level of
interest and excitment which has happened around these features, or how
having these capabilities will grow our community, or how working with
David and others to develop new PostgreSQL developers who may some day
become committers and continue to carry PostgreSQL forward long past
when we are gone is a worthwhile goal.

I do believe that this is a quality piece of software and that it will
be a benefit to the community, or I wouldn't have committed it in the
first place.  There are certainly other patches which I've bounced back
while doing reviews, others that I've rewritten nearly from scratch, or
reviewed, tweaked, and committed.  I've also reverted my share of
patches when issues have been pointed out with them, and held off on
moving forward when there has been clear opposition to the patch being
committed at the time.  Further, as I said above, I will continue to
maintain these features and any others which I commit (which I truely
hope to be numerous) over the years to come.  I hope that's clear from
the history which I have with the community.  I'm sure bugs will be
found in our releases in many areas, but I do not believe that this
feature will have substantially more than others, even so, I'm willing
to set that aside and work with another committer to review the feature,
to make sure that doesn't happen with this patch, and have agreed to
revert it if I'm unable to do so because there is no one willing to.  I
am not asking anyone to do this against their will or to take time away
from their own projects, nor do I wish for the community to lose, as
that would mean I'd be losing too.
Thanks!
    Stephen