Thread: Bizarre 7.3.2 bug
On one machine (installed as 7.3.1, upgraded to 7.3.2): usa=# select version(); version ---------------------------------------------------------------------PostgreSQL 7.3.2 on i386-portbld-freebsd4.7, compiledby GCC 2.95.4 (1 row) usa=# select ''::integer; ERROR: pg_atoi: zero-length string On another machine (installed as 7.3.2): australia=# select version(); version ---------------------------------------------------------------------PostgreSQL 7.3.2 on i386-portbld-freebsd4.4, compiledby GCC 2.95.3 (1 row) australia=# select ''::integer; WARNING: pg_atoi: zero-length stringint4 ------ 0 (1 row) What the??? Chris
On Mon, 2003-04-21 at 22:23, Christopher Kings-Lynne wrote: > usa=# select ''::integer; > ERROR: pg_atoi: zero-length string > australia=# select ''::integer; > WARNING: pg_atoi: zero-length string > int4 > ------ > 0 > (1 row) > > What the??? Looks like the FreeBSD maintainer (you know who you are, seanc :-) ) has decided to be clever: http://www.freebsd.org/cgi/cvsweb.cgi/ports/databases/postgresql7/files/patch-src%3a%3abackend%3a%3autils%3a%3aadt%3a%3anumutils.c?rev=1.1&content-type=text/x-cvsweb-markup Frankly, this is exactly the kind of modification that distributions should *not* be making, IMHO. Cheers, Neil
--On Monday, April 21, 2003 22:44:23 -0400 Neil Conway <neilc@samurai.com> wrote: > On Mon, 2003-04-21 at 22:23, Christopher Kings-Lynne wrote: >> usa=# select ''::integer; >> ERROR: pg_atoi: zero-length string > >> australia=# select ''::integer; >> WARNING: pg_atoi: zero-length string >> int4 >> ------ >> 0 >> (1 row) >> >> What the??? > > Looks like the FreeBSD maintainer (you know who you are, seanc :-) ) has > decided to be clever: > > http://www.freebsd.org/cgi/cvsweb.cgi/ports/databases/postgresql7/files/p > atch-src%3a%3abackend%3a%3autils%3a%3aadt%3a%3anumutils.c?rev=1.1&content > -type=text/x-cvsweb-markup > > Frankly, this is exactly the kind of modification that distributions > should *not* be making, IMHO. I had asked that it be ****OPTIONAL****, and Sean had ignored that request. LER > > Cheers, > > Neil > > > ---------------------------(end of broadcast)--------------------------- > TIP 6: Have you searched our list archives? > > http://archives.postgresql.org > -- Larry Rosenman http://www.lerctr.org/~ler Phone: +1 972-414-9812 E-Mail: ler@lerctr.org US Mail: 1905 Steamboat Springs Drive, Garland, TX 75044-6749
"Christopher Kings-Lynne" <chriskl@familyhealth.com.au> writes:
> australia=# select ''::integer;
> WARNING: pg_atoi: zero-length string
That's got to be a locally-modified copy. The "zero-length string"
text has only existed since last August, and it's always been an ERROR.
regards, tom lane
> "Christopher Kings-Lynne" <chriskl@familyhealth.com.au> writes: > > australia=# select ''::integer; > > WARNING: pg_atoi: zero-length string > > That's got to be a locally-modified copy. The "zero-length string" > text has only existed since last August, and it's always been an ERROR. Yes, I agree the distro should not modify this, but I do think it would have been better to have it as a WARNING at least for one release... Chris
> > usa=# select ''::integer; > > ERROR: pg_atoi: zero-length string > > > australia=# select ''::integer; > > WARNING: pg_atoi: zero-length string > > int4 > > ------ > > 0 > > (1 row) > > > > What the??? > > Looks like the FreeBSD maintainer (you know who you are, seanc :-) ) > has decided to be clever: > > http://www.freebsd.org/cgi/cvsweb.cgi/ports/databases/postgresql7/files/patch-src%3a%3abackend%3a%3autils%3a%3aadt%3a%3anumutils.c?rev=1.1&content-type=text/x-cvsweb-markup I don't think it was clever so much as bowing to the whining masses of RT and Horde users and I caved... > I had asked that it be ****OPTIONAL****, and Sean had ignored that > request. LER, you were only one of thirty people that nagged me on this, but you were the one who submitted the patch and were the 1st to point it out. :) > > australia=# select ''::integer; > > WARNING: pg_atoi: zero-length string > > That's got to be a locally-modified copy. The "zero-length string" > text has only existed since last August, and it's always been an ERROR. Hrm, well, if I don't get any criticism or a heads up for when the 7.3.3 release is (hint hint) in the next 48hrs, I'll dawn my flame retardant suite and hope that the Horde/RT folks have a less broken product at this time. I'll remove the patch and bump the port version for those interested. > Yes, I agree the distro should not modify this, but I do think it > would have been better to have it as a WARNING at least for one > release... One of the goals of the FreeBSD ports system is to mitigate these inconsistencies in programs/packages and to help provide an easier to use platform that is more controlled than native programs found out in the wild. These kinds of patches are pretty common, but it is rare that PostgreSQL has 'em because of it's releng process is substantially better than other projects out there. -sc -- Sean Chittenden
--On Monday, April 21, 2003 21:24:40 -0700 Sean Chittenden <sean@chittenden.org> wrote: >> > usa=# select ''::integer; >> > ERROR: pg_atoi: zero-length string >> >> > australia=# select ''::integer; >> > WARNING: pg_atoi: zero-length string >> > int4 >> > ------ >> > 0 >> > (1 row) >> > >> > What the??? >> >> Looks like the FreeBSD maintainer (you know who you are, seanc :-) ) >> has decided to be clever: >> >> http://www.freebsd.org/cgi/cvsweb.cgi/ports/databases/postgresql7/files/ >> patch-src%3a%3abackend%3a%3autils%3a%3aadt%3a%3anumutils.c?rev=1.1&conte >> nt-type=text/x-cvsweb-markup > > I don't think it was clever so much as bowing to the whining masses of > RT and Horde users and I caved... > >> I had asked that it be ****OPTIONAL****, and Sean had ignored that >> request. > > LER, you were only one of thirty people that nagged me on this, but > you were the one who submitted the patch and were the 1st to point it > out. :) > >> > australia=# select ''::integer; >> > WARNING: pg_atoi: zero-length string >> >> That's got to be a locally-modified copy. The "zero-length string" >> text has only existed since last August, and it's always been an ERROR. > > Hrm, well, if I don't get any criticism or a heads up for when the > 7.3.3 release is (hint hint) in the next 48hrs, I'll dawn my flame > retardant suite and hope that the Horde/RT folks have a less broken > product at this time. I'll remove the patch and bump the port version > for those interested. both **STILL** need it :-( > >> Yes, I agree the distro should not modify this, but I do think it >> would have been better to have it as a WARNING at least for one >> release... > > One of the goals of the FreeBSD ports system is to mitigate these > inconsistencies in programs/packages and to help provide an easier to > use platform that is more controlled than native programs found out in > the wild. These kinds of patches are pretty common, but it is rare > that PostgreSQL has 'em because of it's releng process is > substantially better than other projects out there. -sc hehe. can we find someway to MAKE THE PATCH OPTIONAL in the Port? LER > > -- > Sean Chittenden -- Larry Rosenman http://www.lerctr.org/~ler Phone: +1 972-414-9812 E-Mail: ler@lerctr.org US Mail: 1905 Steamboat Springs Drive, Garland, TX 75044-6749
Sean Chittenden <sean@chittenden.org> writes:
> Hrm, well, if I don't get any criticism or a heads up for when the
> 7.3.3 release is (hint hint) in the next 48hrs,
I doubt 7.3.3 will be out in the next 48hrs, but I do think we are
overdue for it. Core already discussed this some weeks ago and agreed
in principle that it was time. Either Bruce or I have been out-of-town
most of the time since then, but right now I don't think there's
anything much standing in the way of pushing out a release.
regards, tom lane
> > Hrm, well, if I don't get any criticism or a heads up for when the > > 7.3.3 release is (hint hint) in the next 48hrs, > > I doubt 7.3.3 will be out in the next 48hrs, but I do think we are > overdue for it. Core already discussed this some weeks ago and > agreed in principle that it was time. Either Bruce or I have been > out-of-town most of the time since then, but right now I don't think > there's anything much standing in the way of pushing out a release. Hrm, alright, well, with the coming of 7.3.3 soon-ish, and given that RT and Horde are still broken (does anyone have any pull with that crowd and the ability to submit patches?), I'll make sure to conditionalize patch when the next version comes out, but I'm not going to force admins to rebuild their DBs unless really necessary. For those interested, just nuke postgresql7/files/patch-src::backend::utils::adt::numutils.c before building the port and you'll get the originally intended behavior. -sc -- Sean Chittenden
--On Monday, April 21, 2003 21:43:54 -0700 Sean Chittenden <sean@chittenden.org> wrote: >> > Hrm, well, if I don't get any criticism or a heads up for when the >> > 7.3.3 release is (hint hint) in the next 48hrs, >> >> I doubt 7.3.3 will be out in the next 48hrs, but I do think we are >> overdue for it. Core already discussed this some weeks ago and >> agreed in principle that it was time. Either Bruce or I have been >> out-of-town most of the time since then, but right now I don't think >> there's anything much standing in the way of pushing out a release. > > Hrm, alright, well, with the coming of 7.3.3 soon-ish, and given that > RT and Horde are still broken (does anyone have any pull with that > crowd and the ability to submit patches?), I'll make sure to > conditionalize patch when the next version comes out, but I'm not > going to force admins to rebuild their DBs unless really necessary. RT's author hasn't released a release in a LONG time, and I don't have any pull. As to Horde, I don't have any insight. Sorry. LER > > For those interested, just nuke > postgresql7/files/patch-src::backend::utils::adt::numutils.c before > building the port and you'll get the originally intended behavior. > -sc > > -- > Sean Chittenden -- Larry Rosenman http://www.lerctr.org/~ler Phone: +1 972-414-9812 E-Mail: ler@lerctr.org US Mail: 1905 Steamboat Springs Drive, Garland, TX 75044-6749
Larry Rosenman <ler@lerctr.org> writes:
> --On Monday, April 21, 2003 21:43:54 -0700 Sean Chittenden
> <sean@chittenden.org> wrote:
>> Hrm, alright, well, with the coming of 7.3.3 soon-ish, and given that
>> RT and Horde are still broken (does anyone have any pull with that
>> crowd and the ability to submit patches?), I'll make sure to
>> conditionalize patch when the next version comes out, but I'm not
>> going to force admins to rebuild their DBs unless really necessary.
> RT's author hasn't released a release in a LONG time, and I don't have any
> pull.
> As to Horde, I don't have any insight. Sorry.
The obvious solution to this sort of problem would be to introduce a
GUC variable ("allow_zero_length_integers" or some such name). I do
not recall why we decided not to do that to begin with --- should we
reconsider, or is it too late to worry about it?
regards, tom lane
> >> Hrm, alright, well, with the coming of 7.3.3 soon-ish, and given
> >> that RT and Horde are still broken (does anyone have any pull
> >> with that crowd and the ability to submit patches?), I'll make
> >> sure to conditionalize patch when the next version comes out, but
> >> I'm not going to force admins to rebuild their DBs unless really
> >> necessary.
>
> > RT's author hasn't released a release in a LONG time, and I don't
> > have any pull.
>
> > As to Horde, I don't have any insight. Sorry.
>
> The obvious solution to this sort of problem would be to introduce a
> GUC variable ("allow_zero_length_integers" or some such name). I do
> not recall why we decided not to do that to begin with --- should we
> reconsider, or is it too late to worry about it?
I'd bury it behind a #define + configure option, if anything. I
wouldn't want any runtime cycles spent on something like this.
I hate to take the side of legacy and know it'd be feature thrash, but
I'd think it'd be appropriate to have this be a WARNING for the life
of 7.3.X and then make it an error in 7.4 where major changes/breakage
like this are expected. -sc
--
Sean Chittenden
> >> The obvious solution to this sort of problem would be to
> >> introduce a GUC variable ("allow_zero_length_integers" or some
> >> such name). I do not recall why we decided not to do that to
> >> begin with --- should we reconsider, or is it too late to worry
> >> about it?
>
> > I agree that, in the future, using a GUC variable is the best way
> > to manage these kinds of backward-incompatible changes. When this
> > was suggested for the pg_atoi problem some time after the 7.3
> > release, someone (you, I believe) suggested that it was too late
> > to add a GUC variable at that point.
>
> I think I did opine that way --- but I too am open to being
> persuaded. Adding a simple boolean GUC variable is foolproof enough
> that I don't believe it could break anything, and so the rationale
> for the "no new features in stable releases" rule doesn't really
> apply. The argument that has to be answered at this point is
> "hasn't everyone who's not completely asleep at the wheel already
> fixed their application code? How much should we cater to them as
> are asleep?"
Well, as I mentioned to Neil on IRC, too much time has been spent on
this as is, IMHO so minimal effort/time should be spent on it. Here
are my thoughts on this:
1) Roll the ERROR back to a WARNING: doesn't really break anything other than POLS re: an out of the way error
conditionthat broken code depends on. Please correct my assertion if I'm wrong.
2) Keep things as an error in 7.4.
3) Add an upgrading note in the 7.4 release notes to the extent of ''::INT is now an error and code must be fixed
accordingly. Major version bumps are the only time that most developers fix their code and look at release notes
anyway.
4) Don't use a GUC for this feature. Using a GUC sets a precedent for supporting a feature that's been depreciated
andtool authors will cater to the list of GUCs available. Putting this behind a #define would be better. Supporting
everyquirk of external applications when PostgreSQL changes its behavior is an anti-feature that should be avoided.
What's the harm/problem with changing things back to a warning? -sc
--
Sean Chittenden
Sean Chittenden <sean@chittenden.org> writes:
> 3) Add an upgrading note in the 7.4 release notes to the extent of
> ''::INT is now an error and code must be fixed accordingly.
How exactly would this differ from the statement already prominently
made in the 7.3 release notes?
An empty string (<literal>''</literal>) is no longer allowed as the input into an integer field. Formerly,
itwas silently interpreted as 0.
I'm not sure that giving a warning for one release cycle would really
reduce the pain all that much. There are way too many client-side
setups in which notices and warnings disappear into the bit-bucket.
regards, tom lane
> > 3) Add an upgrading note in the 7.4 release notes to the extent of > > ''::INT is now an error and code must be fixed accordingly. > > How exactly would this differ from the statement already prominently > made in the 7.3 release notes? > > An empty string (<literal>''</literal>) is no longer allowed as > the input into an integer field. Formerly, it was silently > interpreted as 0. Hrm... good point. ::shrugs:: Horde and RT be damned. It's been ~8mo since that change was made so I'm not that inclined to continue to support their brokenness and would rather remove the patch and bring BSD inline than be the catalyst for having PostgreSQL make a bad change (adding a GUC) to support this. User communities need to chirp to their nearby Horde/RT developer to have them fix their code. The patch will live on in CVS if people really need this behavior. Actually, the "correct" behavior is for a patch to be made for horde and RT to fix their behavior and for that to be submitted back to the respective horde && RT developers. Once such a patches exist, I can add them to the ports so that FreeBSD folks still continue to have their applications function as expected. -sc -- Sean Chittenden
At 09:43 PM 21/04/2003 -0700, Sean Chittenden wrote: >RT and Horde are still broken I know that RT3 works with 7.3, and I *think* 2.1 does as well. It's under pretty active development, current version is 3.01, and there's 3.02pre3 out as well. ---------------------------------------------------------------- Philip Warner | __---_____ Albatross Consulting Pty. Ltd. |----/ - \ (A.B.N. 75 008 659 498) | /(@) ______---_ Tel: (+61) 0500 83 82 81 | _________ \ Fax: (+61) 03 5330 3172 | ___________ | Http://www.rhyme.com.au | / \| | --________-- PGP key available upon request, | / and from pgp5.ai.mit.edu:11371 |/
On Mon, 21 Apr 2003, Sean Chittenden wrote: > this as is, IMHO so minimal effort/time should be spent on it. Here > are my thoughts on this: > > 1) Roll the ERROR back to a WARNING: doesn't really break anything > other than POLS re: an out of the way error condition that broken > code depends on. Please correct my assertion if I'm wrong. > > 2) Keep things as an error in 7.4. > > 3) Add an upgrading note in the 7.4 release notes to the extent of > ''::INT is now an error and code must be fixed accordingly. Major > version bumps are the only time that most developers fix their code > and look at release notes anyway. > > 4) Don't use a GUC for this feature. Using a GUC sets a precedent for > supporting a feature that's been depreciated and tool authors will > cater to the list of GUCs available. Putting this behind a #define > would be better. Supporting every quirk of external applications > when PostgreSQL changes its behavior is an anti-feature that should > be avoided. > > What's the harm/problem with changing things back to a warning? -sc I can see the situation where, rightly or wrongly, someone is actually relying on the transaction being aborted and detecting an error in the broken statement will be caught out by changing to a warning. I say leave as an error. Let *BSD patch if they want, but let's not encourage empty string to take meanings it doesn't have. This is just as bad as assuming empty string means null. Indeed, given the call for empty string to mean null from 'compatibility' folk when exactly should the backend assume 0 and not null? -- Nigel J. Andrews