Thread: pgsql-server/src backend/utils/adt/numutils.c ...
pgsql-server/src backend/utils/adt/numutils.c ...
From
momjian@postgresql.org (Bruce Momjian - CVS)
Date:
CVSROOT: /cvsroot
Module name: pgsql-server
Changes by: momjian@postgresql.org 02/08/27 16:29:11
Modified files:
src/backend/utils/adt: numutils.c
src/test/regress/expected: arrays.out copy2.out
src/test/regress/sql: arrays.sql
Log message:
Throw error on pg_atoi(''), regression adjustments.
momjian@postgresql.org (Bruce Momjian - CVS) writes:
> Throw error on pg_atoi(''), regression adjustments.
Have you considered the backward compatibility implications of this
change?
Cheers,
Neil
--
Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC
Well, not really. It is more of a tightening up, where '' is not really
zero, it is ''. Regression tests pass. Let's see how it flies during
beta. If we get pushback, then we can back out the patch, but we will
not know if we can tighten it unless we put it into the code.
---------------------------------------------------------------------------
Neil Conway wrote:
> momjian@postgresql.org (Bruce Momjian - CVS) writes:
> > Throw error on pg_atoi(''), regression adjustments.
>
> Have you considered the backward compatibility implications of this
> change?
>
> Cheers,
>
> Neil
>
> --
> Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Don't 'kill -9' the postmaster
>
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian <pgman@candle.pha.pa.us> writes: > Well, not really. It is more of a tightening up, where '' is not > really zero, it is ''. Sure, but "tightening up" is just another way to say "backward incompatible change". I'd agree this is a change we should make, I'm just not convinced that it's worth the backward compatibility hit. What if we just added an elog(WARNING) that said "Zero-length integer conversion is deprecated and will be removed in the near future", and then fixed this properly in 7.4? I usually dislike adding warnings, but I don't think that noting in the documentation that this feature is deprecated is sufficient in this case: information on this kind of behavior is nearly impossible to find in the docs. Cheers, Neil -- Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC
Neil Conway <neilc@samurai.com> writes:
> momjian@postgresql.org (Bruce Momjian - CVS) writes:
>> Throw error on pg_atoi(''), regression adjustments.
> Have you considered the backward compatibility implications of this
> change?
Do you want to argue against it? If so, let's hear it ...
regards, tom lane
Neil Conway wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Well, not really. It is more of a tightening up, where '' is not > > really zero, it is ''. > > Sure, but "tightening up" is just another way to say "backward > incompatible change". I'd agree this is a change we should make, I'm > just not convinced that it's worth the backward compatibility hit. > > What if we just added an elog(WARNING) that said "Zero-length integer > conversion is deprecated and will be removed in the near future", and > then fixed this properly in 7.4? I usually dislike adding warnings, > but I don't think that noting in the documentation that this feature > is deprecated is sufficient in this case: information on this kind of > behavior is nearly impossible to find in the docs. I don't even think it was ever a feature. We are changing COPY's handling of missing trailing fields, so this will fit right in. I assume that will be the place it hits most. Let's see if we get any pushback. We can always downgrade it to a warning during beta. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I don't even think it was ever a feature.
It was certainly never documented as something that would work, unless
you chanced to read the one comment in numutils.c. A normal person
would not expect an empty string to be taken as a valid representation
of "zero".
> Let's see if we get any pushback. We can always downgrade it to a
> warning during beta.
I agree; tighten up and wait to see if anyone yowls.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > I don't even think it was ever a feature. > > It was certainly never documented as something that would work, unless > you chanced to read the one comment in numutils.c. A normal person > would not expect an empty string to be taken as a valid representation > of "zero". Well, assuming we are matching atoi, it would make sense to return 0 for '', I think. > > Let's see if we get any pushback. We can always downgrade it to a > > warning during beta. > > I agree; tighten up and wait to see if anyone yowls. Yes, we can loosen during beta, but not tighten. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Tom Lane wrote:
>> It was certainly never documented as something that would work, unless
>> you chanced to read the one comment in numutils.c. A normal person
>> would not expect an empty string to be taken as a valid representation
>> of "zero".
> Well, assuming we are matching atoi, it would make sense to return 0 for
> '', I think.
If we wanted to match atoi, we'd use atoi. pg_atoi exists to provide
more bulletproof behavior --- such as detecting overflow and invalid
input and erroring out when it happens.
regards, tom lane