Thread: Re: [PATCHES] Custom variable class segmentation fault
Bruce Momjian <bruce@momjian.us> writes:
> There were three things wrong with the original patch:
> o spacing, e.g. "if( x =- 1 )"
> o an incorrect API for memory freeing by parse_value()
> o verify_config_option() didn't consider custom variables
> These have all been corrected, so I don't see the value in removing the
> patch now that it is working.
You mean, there were three things wrong that we've identified so far,
and as for "it's working now", you thought it worked each previous
time you applied or patched it. I repeat my statement that I've got
zero confidence in it. It needs line-by-line review, and not by you.
I don't have time for it right now (I have more important things
to worry about, like bitmap indexes). Unless Peter or someone else
who knows the GUC code very well is willing to look it over pronto,
I want it out so it's not destabilizing things while we try to get
other patches committed.
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.)
regards, tom lane
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
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. +
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? Thanks for explanation Zdenek 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 >
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. +
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. 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. 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 >>>> > >
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. +
Bruce Momjian wrote: > > >> 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.) > I certainly expect buildfarm to break. But it is not intended as a substitute for review either. We shouldn't be in the business of saying "let's apply it and see if buildfarm breaks". We should be saying "I have looked at this and my best guess is that it won't break." That won't avoid all breakage, certainly. But it will keep it down. > 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. > > As I pointed out - I did not object to the patch being applied, I just stated an agreement with a general principle, so there is nothing to withdraw. cheers andrew
Andrew Dunstan wrote: > If you and Peter have reviewed it thoroughly then I see no reason the > patch should not be applied. I merely said that the way the patch was presented, with significant code refactoring mixed in, I couldn't review it (as effectively as perhaps otherwise). FWIW, I believe that the general approach is sound, but I haven't actually "reviewed" the patch completely. -- Peter Eisentraut http://developer.postgresql.org/~petere/
Peter Eisentraut <peter_e@gmx.net> writes:
> I merely said that the way the patch was presented, with significant
> code refactoring mixed in, I couldn't review it (as effectively as
> perhaps otherwise). FWIW, I believe that the general approach is
> sound, but I haven't actually "reviewed" the patch completely.
Given the multiple problems already found, I feel quite uncomfortable
with putting it back in until another set of eyeballs has been over
it. Do you have time to do a full review anytime soon? I'm trying
to deal with all the other stuff in the queue, and so would prefer
not to spend my own time on it ...
regards, tom lane
Andrew Dunstan wrote: > Bruce Momjian wrote: > > > > > >> 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.) > > > > I certainly expect buildfarm to break. But it is not intended as a > substitute for review either. We shouldn't be in the business of saying > "let's apply it and see if buildfarm breaks". We should be saying "I > have looked at this and my best guess is that it won't break." That > won't avoid all breakage, certainly. But it will keep it down. Are you saying that's what is happening, that people aren't reviewing and letting the buildfarm catch it. I have seen that only in cases where we can't guess how an platform will be affected by the patch. -- Bruce Momjian bruce@momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Tom Lane wrote: > Peter Eisentraut <peter_e@gmx.net> writes: > > I merely said that the way the patch was presented, with significant > > code refactoring mixed in, I couldn't review it (as effectively as > > perhaps otherwise). FWIW, I believe that the general approach is > > sound, but I haven't actually "reviewed" the patch completely. > > Given the multiple problems already found, I feel quite uncomfortable > with putting it back in until another set of eyeballs has been over > it. Do you have time to do a full review anytime soon? I'm trying > to deal with all the other stuff in the queue, and so would prefer > not to spend my own time on it ... Peter, you also commented on an error message displayed, so I assume you did look it over somewhat. I agree it would be best for Peter to review it. -- Bruce Momjian bruce@momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +