Re: [PATCHES] Custom variable class segmentation fault

The latest HEAD is segfaulting on startup if I have the following
lines in postgresql.conf:

custom_variable_classes = 'plperl'
plperl.use_strict = on

If I comment out the second line then the server starts successfully.
Platform is Solaris 9/sparc.

(gdb) bt
#0 0xff0340a0 in strcmp () from /usr/lib/libc.so.1
#1 0x00257504 in verify_config_option (name=0x0, value=0x397258 "on", context=PGC_POSTMASTER, source=PGC_S_FILE, isNewEqual=0xffbff2cb "\001",
isContextOK=0xffbff2ca "\001\001") at guc.c:5513
#2 0x0025d3b4 in ProcessConfigFile (context=PGC_POSTMASTER) at guc-file.l:152
#3 0x0025d80c in SelectConfigFiles (userDoption=0x1 <Address 0x1 out of bounds>, progname=0x390610 "postgres") at guc.c:2867
#4 0x0019a450 in PostmasterMain (argc=5, argv=0x391240) at postmaster.c:602
#5 0x001518c8 in main (argc=5, argv=0x391240) at main.c:187

--
Michael Fuhr

Michael Fuhr wrote:
> The latest HEAD is segfaulting on startup if I have the following
> lines in postgresql.conf:
>
> custom_variable_classes = 'plperl'
> plperl.use_strict = on
>
> If I comment out the second line then the server starts successfully.
> Platform is Solaris 9/sparc.
>
> (gdb) bt
> #0 0xff0340a0 in strcmp () from /usr/lib/libc.so.1
> #1 0x00257504 in verify_config_option (name=0x0, value=0x397258 "on", context=PGC_POSTMASTER, source=PGC_S_FILE, isNewEqual=0xffbff2cb "\001",
> isContextOK=0xffbff2ca "\001\001") at guc.c:5513
> #2 0x0025d3b4 in ProcessConfigFile (context=PGC_POSTMASTER) at guc-file.l:152
> #3 0x0025d80c in SelectConfigFiles (userDoption=0x1 <Address 0x1 out of bounds>, progname=0x390610 "postgres") at guc.c:2867
> #4 0x0019a450 in PostmasterMain (argc=5, argv=0x391240) at postmaster.c:602
> #5 0x001518c8 in main (argc=5, argv=0x391240) at main.c:187

Thanks for the report. This attached, applied patch fixes it. The
problem was that custom variables have _no_ default value for strings on
startup, and the checking code was being too agressive.

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Michael Fuhr <mike(at)fuhr(dot)org> writes:
> The latest HEAD is segfaulting on startup if I have the following
> lines in postgresql.conf:

> custom_variable_classes = 'plperl'
> plperl.use_strict = on

Bruce, please re-revert that GUC patch and don't put it back in until
someone like Peter or me has actually reviewed it. My faith in it has
gone to zero, and I don't think you are able to fix it either.

BTW, do I need to mention that the plperl patch is breaking the
buildfarm again?

regards, tom lane

Tom Lane wrote:
> BTW, do I need to mention that the plperl patch is breaking the
> buildfarm again?
>
>
>

No :-) See my comments from about an hour ago. It needs to be removed also.

cheers

andrew

Tom Lane wrote:
> Michael Fuhr <mike(at)fuhr(dot)org> writes:
> > The latest HEAD is segfaulting on startup if I have the following
> > lines in postgresql.conf:
>
> > custom_variable_classes = 'plperl'
> > plperl.use_strict = on
>
> Bruce, please re-revert that GUC patch and don't put it back in until
> someone like Peter or me has actually reviewed it. My faith in it has
> gone to zero, and I don't think you are able to fix it either.

Peter had already reviewed it and given comments.

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. I have attached the three GUC patches
that were applied to CVS, so if someone wants corrections or removal
based on specific issues, please let me know.

> BTW, do I need to mention that the plperl patch is breaking the
> buildfarm again?

That has been reverted, because of the regression failures and Andrew's
analysis.

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)gmx(dot)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(at)momjian(dot)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(at)gmx(dot)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(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Where are we on the GUC comment/reload to defaults patch? Do we have
multiple people objecting to its application? Previously only Tom
objected, and Andrew partially.

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

Bruce Momjian wrote:
> Tom Lane wrote:
> > Michael Fuhr <mike(at)fuhr(dot)org> writes:
> > > The latest HEAD is segfaulting on startup if I have the following
> > > lines in postgresql.conf:
> >
> > > custom_variable_classes = 'plperl'
> > > plperl.use_strict = on
> >
> > Bruce, please re-revert that GUC patch and don't put it back in until
> > someone like Peter or me has actually reviewed it. My faith in it has
> > gone to zero, and I don't think you are able to fix it either.
>
> Peter had already reviewed it and given comments.
>
> 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. I have attached the three GUC patches
> that were applied to CVS, so if someone wants corrections or removal
> based on specific issues, please let me know.
>
> > BTW, do I need to mention that the plperl patch is breaking the
> > buildfarm again?
>
> That has been reverted, because of the regression failures and Andrew's
> analysis.
>
> --
> Bruce Momjian bruce(at)momjian(dot)us
> EnterpriseDB http://www.enterprisedb.com
>
> + If your life is a hard drive, Christ can be your backup. +

>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: In versions below 8.0, the planner will ignore your desire to
> choose an index scan if your joining column's datatypes do not
> match

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +