Horton

Thread

Commits

  1. Give up on testing guc.c's behavior for "infinity" inputs.

  2. In guc.c, ignore ERANGE errors from strtod().

  3. Include GUC's unit, if it has one, in out-of-range error messages.

  1. pgsql: Include GUC's unit, if it has one, in out-of-range error message

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-03-10T19:18:20Z

    Include GUC's unit, if it has one, in out-of-range error messages.

This should reduce confusion in cases where we've applied a units
conversion, so that the number being reported (and the quoted range
limits) are in some other units than what the user gave in the
setting we're rejecting.

Some of the changes here assume that float GUCs can have units,
which isn't true just yet, but will be shortly.

Discussion: https://postgr.es/m/3811.1552169665@sss.pgh.pa.us

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/28a65fc3607a0f45c39a9418f747459bb4f1592a

Modified Files
--------------
src/backend/utils/misc/guc.c      | 113 ++++++++++++++++++++++----------------
src/test/regress/expected/guc.out |   2 +-
2 files changed, 66 insertions(+), 49 deletions(-)

  2. Re: pgsql: Include GUC's unit, if it has one, in out-of-range error message

    Michael Paquier <michael@paquier.xyz> — 2019-03-11T01:11:14Z

    On Sun, Mar 10, 2019 at 07:18:20PM +0000, Tom Lane wrote:
> Include GUC's unit, if it has one, in out-of-range error messages.
> 
> This should reduce confusion in cases where we've applied a units
> conversion, so that the number being reported (and the quoted range
> limits) are in some other units than what the user gave in the
> setting we're rejecting.
> 
> Some of the changes here assume that float GUCs can have units,
> which isn't true just yet, but will be shortly.

It does not seem to have cooled down all animals yet, whelk is
still complaining:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=whelk&dt=2019-03-11%2000%3A41%3A13

-ERROR:  -Infinity is outside the valid range for parameter "geqo_selection_bias" (1.5 .. 2)
+ERROR:  invalid value for parameter "geqo_selection_bias": "-infinity"

It would be nice if we could avoid an alternate output.
--
Michael

  3. Portability of strtod (was Re: pgsql: Include GUC's unit, if it has one, in out-of-range error message)

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-03-11T03:15:40Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Sun, Mar 10, 2019 at 07:18:20PM +0000, Tom Lane wrote:
>> Include GUC's unit, if it has one, in out-of-range error messages.

> It does not seem to have cooled down all animals yet, whelk is
> still complaining:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=whelk&dt=2019-03-11%2000%3A41%3A13

Yeah, also the HPUX animals (gaur isn't booted up at the moment, but
I bet it'd fail too).

I think what's going on here is what's mentioned in the comments in
float8in_internal:

         * C99 requires that strtod() accept NaN, [+-]Infinity, and [+-]Inf,
         * but not all platforms support all of these (and some accept them
         * but set ERANGE anyway...)

Specifically, these symptoms would be explained if these platforms'
strtod() sets ERANGE for infinity.

I can think of three plausible responses.  In decreasing order of
amount of work:

1. Decide that we'd better wrap strtod() with something that ensures
platform-independent behavior for all our uses of strtod (and strtof?)
rather than only float8in_internal.

2. Put in a hack in guc.c to make it ignore ERANGE as long as the result
satisfies isinf().  This would ensure GUC cases would go through the
value-out-of-range path rather than the syntax-error path.  We've got
a bunch of other strtod() calls that are potentially subject to similar
platform dependencies though ...

3. Decide this isn't worth avoiding platform dependencies for, and just
take out the new regression test case.  I'd only put in that test on
the spur of the moment anyway, so it's hard to argue that it's worth
much.

Thoughts?

			regards, tom lane

  4. Re: Portability of strtod (was Re: pgsql: Include GUC's unit, if it has one, in out-of-range error message)

    Amit Kapila <amit.kapila16@gmail.com> — 2019-03-11T03:54:28Z

    On Mon, Mar 11, 2019 at 8:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Michael Paquier <michael@paquier.xyz> writes:
> > On Sun, Mar 10, 2019 at 07:18:20PM +0000, Tom Lane wrote:
> I think what's going on here is what's mentioned in the comments in
> float8in_internal:
>
>          * C99 requires that strtod() accept NaN, [+-]Infinity, and [+-]Inf,
>          * but not all platforms support all of these (and some accept them
>          * but set ERANGE anyway...)
>
> Specifically, these symptoms would be explained if these platforms'
> strtod() sets ERANGE for infinity.
>
> I can think of three plausible responses.  In decreasing order of
> amount of work:
>
> 1. Decide that we'd better wrap strtod() with something that ensures
> platform-independent behavior for all our uses of strtod (and strtof?)
> rather than only float8in_internal.
>

This sounds like a good approach, but won't it has the risk of change
in behaviour?

> 2. Put in a hack in guc.c to make it ignore ERANGE as long as the result
> satisfies isinf().  This would ensure GUC cases would go through the
> value-out-of-range path rather than the syntax-error path.  We've got
> a bunch of other strtod() calls that are potentially subject to similar
> platform dependencies though ...
>

Yeah, this won't completely fix the symptom.

> 3. Decide this isn't worth avoiding platform dependencies for, and just
> take out the new regression test case.  I'd only put in that test on
> the spur of the moment anyway, so it's hard to argue that it's worth
> much.
>

For the time being option-3 sounds like a reasonable approach to fix
buildfarm failures and then later if we want to do some bigger surgery
based on option-1 or some other option, we can anyways do it.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  5. Re: Portability of strtod (was Re: pgsql: Include GUC's unit, if it has one, in out-of-range error message)

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-03-11T15:06:33Z

    Amit Kapila <amit.kapila16@gmail.com> writes:
> On Mon, Mar 11, 2019 at 8:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I can think of three plausible responses.  In decreasing order of
>> amount of work:
>> 
>> 1. Decide that we'd better wrap strtod() with something that ensures
>> platform-independent behavior for all our uses of strtod (and strtof?)
>> rather than only float8in_internal.

> This sounds like a good approach, but won't it has the risk of change
> in behaviour?

Well, the point would be to produce consistent behavior across platforms
where it's not consistent now.  So yeah, some platforms would necessarily
see a behavior change.  But I think your point is that changing this
everywhere is solving a problem that hasn't been complained about,
and that's a valid concern.

>> 2. Put in a hack in guc.c to make it ignore ERANGE as long as the result
>> satisfies isinf().  This would ensure GUC cases would go through the
>> value-out-of-range path rather than the syntax-error path.  We've got
>> a bunch of other strtod() calls that are potentially subject to similar
>> platform dependencies though ...

> Yeah, this won't completely fix the symptom.

It would fix things in an area where we're changing the behavior anyway,
so maybe that's the right scope to work at.  After thinking about this
a little, it seems like simply ignoring ERANGE from strtod might get the
behavior we want: per POSIX strtod's result should be infinity for overflow
or zero for underflow, and proceeding with either of those should give
better behavior than treating the case as a syntax error.  Anyway
I think I'll try that and see what the buildfarm says.

>> 3. Decide this isn't worth avoiding platform dependencies for, and just
>> take out the new regression test case.  I'd only put in that test on
>> the spur of the moment anyway, so it's hard to argue that it's worth
>> much.

> For the time being option-3 sounds like a reasonable approach to fix
> buildfarm failures and then later if we want to do some bigger surgery
> based on option-1 or some other option, we can anyways do it.

Yeah, if I can't fix it pretty easily then I'll just remove the test
case.  But the behavior shown in the expected result is a bit nicer than
what we're actually getting from these buildfarm animals, so ideally
we'd fix it.

			regards, tom lane