Re: [PATCHES] Custom variable class segmentation fault - Mailing list pgsql-hackers

Andrew Dunstan wrote:
> 
> Bruce,
> 
> I think you have misunderstood.
> 
> If you and Peter have reviewed it thoroughly then I see no reason the 
> patch should not be applied.

We have.  I did extensive rework, and Peter exchanged emails with the
author asking questions.  I did have questions about how the original
patch was freeing memory, but didn't second-guess the author.  Turns out
it needed rework, and that was done after build farm failures.
> My post below was merely to agree with Tom that in principle, patches 
> should be be reviewed before application and not after. I still think 
> that's right - I have been concerned lately that the buildfarm has been 
> broken a bit too much.

Well, just because they are reviewed doesn't mean they aren't going to
break the build farm.  In fact, the build farm is there to be broken ---
if all patches worked fine on all machines, we wouldn't need the build
farm.  Let's not get into a case where keeping the build farm green is
our primary goal, "Oh, let's not apply that patch or it might break the
build farm".  Hey, I have an idea, let's stop CVS update on the build
farm, and it will stay green forever.  :-)  LOL  (Of course, we don't
want the build farm to stay broken or it masks newly introduced errors.)

If you withdraw your object to the GUC patch, then with a single person
objecting, the patch either goes in or that person takes responsibility
for getting it into 8.2, or the blame for leaving it for 8.3.

---------------------------------------------------------------------------

> cheers
> 
> andrew
> 
> 
> Bruce Momjian wrote:
> > Zdenek Kotala wrote:
> >   
> >> Bruce, Andrew, Tom.
> >>
> >> I little bit confuse now, what status of this patch is? I check your 
> >> observation and I agree with them. But I don't where is "ball" now and 
> >> what I can/must do now like author of this patch?
> >>     
> >
> > I am unsure too.  I would not back out a patch for nonspecific concerns
> > from one person, but from two people I reverted it.  Tom wants more eyes
> > on it, but I don't know how that is going to happen, especially since
> > Peter, who has done a lot of GUC work, has reviewed it already, and so
> > have I.
> >
> > I will keep the patches and if no one works on it, again ask to apply it
> > before we finish 8.2, and see if there are still objections.  If there
> > are still objections, we will have to vote on whether we want it
> > applied.
> >
> > ---------------------------------------------------------------------------
> >
> >
> >
> >   
> >> Bruce Momjian napsal(a):
> >>     
> >>> OK, with two people now concerned, patch reverted.
> >>>
> >>> ---------------------------------------------------------------------------
> >>>
> >>> Andrew Dunstan wrote:
> >>>       
> >>>> Tom Lane wrote:
> >>>>         
> >>>>> I've always found it easier to review uncommitted patches than committed
> >>>>> ones anyway.  When you're playing catch-up by reviewing a committed
> >>>>> patch, you have to deal with three states of the code rather than two
> >>>>> (pre-patch, post-patch, your own mods).  That gets rapidly worse if the
> >>>>> patch has been in there awhile and other changes go in on top of it.
> >>>>> Plus, once other changes accumulate on top, it becomes extremely painful
> >>>>> to revert if the conclusion is that the patch is completely broken.
> >>>>> (A conclusion that I don't think is at all unlikely with respect to
> >>>>> this patch.)
> >>>>>
> >>>>>     
> >>>>>   
> >>>>>           
> >>>> Easy or not this strikes me as good policy. And nothing is urgent quite 
> >>>> yet - we still have another 18 days to the end of the month, which is 
> >>>> the stated deadline for getting patches reviewed and committed.
> >>>>
> >>>> cheers
> >>>>
> >>>> andrew
> >>>>
> >>>> ---------------------------(end of broadcast)---------------------------
> >>>> TIP 2: Don't 'kill -9' the postmaster
> >>>>         
> >
> >   

--  Bruce Momjian   bruce@momjian.us EnterpriseDB    http://www.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +