Re: Skytools committed without hackers discussion/review

On Wed, Oct 10, 2007 at 11:50:12AM +0300, Marko Kreen wrote:
> On 10/10/07, Joshua D. Drake <jd(at)commandprompt(dot)com> wrote:
> > On Tue, 9 Oct 2007 18:35:52 -0500
> > Michael Glaesemann <grzm(at)seespotcode(dot)net> wrote:
> > > On Oct 9, 2007, at 0:06 , Bruce Momjian wrote:
> > > > I am surprised we are not backing
> > > > out the patch and requiring that the patch go through the formal
> > > > review
> > > > process.
> > >
> > > I have no opinion as to the patch itself (other than the fact that
> > > it's a not bug fix), but I think this patch should be reverted
> > > because it's (a) after feature freeze, (b) had no discussion on
> > > hackers (or patches), (c) is not a bug fix. IMO rules can be bent
> > > but there should always at least be discussion before a new feature
> > > is committed after feature freeze and definitely after beta.
> > > Otherwise, the rule appears to be if you can get it in somehow, it's
> > > in.
> >
> > I think this almost says it all. My particular gripe about this whole
> > thing is that there are other features that are not too intrusive (or
> > appear so anyway) that are easily more useful that are not being
> > considered at all. Namely,
> > http://archives.postgresql.org/pgsql-patches/2007-10/msg00087.php . It
> > makes the whole process seem tilted and subjective.
> >
> > IMO, the patch is reverted, and submitted for 8.4 or pgfoundry.
>
> Yes, reverting is an option, but please, do that at least with
> an understanding what actually happened. Current discussion
> seems to give picture that Jan committed some private piece of
> code without consulting anybody which was not the case.

At least I am fully aware that it's not a private piece of code. And in
general, I trust Jan (and of course Tom as well) to take a patch from
elsewhere and put it in.

My objections are twofold:

1) We don't add things after beta. I can live with adding it during feature
freeze since it's contrib, and reviewed by these people, but I think it's
horrible to do it after we've shipped beta1.

2) I get the strong feeling that it's going into contrib only because it
missed feature freeze. If it hadn't missed feature freeze, it wuold be in
the backend and not contrib. If the plan is that it lives in contrib
forever, that argument falls. But if the plan is to migrate it into the
backend for 8.4, then I strongly object to using contrib just as a way to
"get it in even though we're feature-frozen".

> It was actually my patch that was reviewed by 2 senior PostgreSQL
> developers: Jan and Tom, then later committed by Jan. I don't
> think the fact that Jan was an interested party by being Slony
> developer invalidates his status as PostgreSQL developer.

Certainly not.

> Obviously that does not make skipping -hackers less mistake,
> but there was no evil from anybody and the "process" for such
> exceptional case was _mostly_ followed.

I don't think anybody thinks there were "evil intentions" behind it. I
certainly don't think Jan would've committed it if he expected people to
dislike it technically.

All objections have been procedural, AFICS.

> Now the skipping -hackers part - that was also my mistake,
> I should have Cc-d the design and code review discussion here
> also. I just saw the contrib-acceptance as minor question,
> the main issue was whether Slony was prepared to such a major
> rewrite of its core parts on such short notice, so I wanted
> to sync with them first.
>
> Also I think several people are annoyed by the "Jan asked permission
> from -core" part of the process. But I think if you replace the
> -core with "release manager" it will become more understandable.
> The fact is there are only few people responsible for releases and
> non-technical decisions need to be made by them. And yes, it should
> have been accompanied by technical review in -hackers.

AFAIK, our "release manager" is -hackers concensus. We don't *have* a
release manager as such. The closest thing you'd get in that case is the
-packagers list which is for those building the binary packages, and they
were not consulted...

But again, I don't see any issues with the technical side of things. It's
procedural, and placement (is contrib really the right place for it).

//Magnus