Bizarre 7.3.2 bug

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&amp;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 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

--
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, it was 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