Re: Problem retrieving a numeric(38,0) value as SQL_NUMERIC_STRUCT if value needs to use all 16 SQLCHAR elements of the val array - Mailing list pgsql-odbc

Ok, committed.

On 06/11/2014 03:05 PM, Hiroshi Inoue wrote:
> Thanks for your investigation.
>
> I don't object to the change.
> Probably few people used SQL_C_NUMERIC extensively.
>
> regards,
> Hiroshi Inoue
>
> (2014/06/10 17:34), Heikki Linnakangas wrote:
>> Ugh, the decimal->binary conversion algorithm is really ugly even before
>> this patch, and the patch doesn't make it any prettier. It took me quite
>> a while to understand how it works: it repeatedly divides the base-10
>> representation by two, and outputs the reminder bit. There are much
>> better and faster ways to convert from decimal to binary, and they're
>> not really any more complicated either.
>>
>> The binary->decimal conversion in ResolveNumericParam is even more ugly.
>> To be honest, I still don't understand the algorithm behind it, but it
>> looks complicated and slow. There's a fast-path for small numbers that
>> fit in an UInt4, which doesn't make it any simpler.
>>
>> Aside from ugly and slow, the binary->decimal conversion is also buggy:
>>
>> 1.The following input creates a core dump with "Floating point exception":
>>
>> precision=3, scale=50, val=0x02 0x03
>>
>> 2. Building the final output string is broken if scale is large, e.g.
>> this produces garbage output:
>>
>> precision=25, scale=80
>>
>> ERROR: invalid input syntax for type numeric:
>> "p.000`000H0000000000000000000000aT000000000000000000000000000000000000000000000770";
>>
>>
>> 3. The param_string buffer allocated for the result of the
>> binary->decimal conversion is too small. The buffer is 128 bytes, but a
>> numeric can have a scale as large as 127. That won't fit, as the sign
>> and decimal dot use two more bytes.
>>
>> I propose the attached patch to fix all of the above, rewriting the
>> binary->decimal and decimal->binary conversions altogether. It fixes all
>> of the known issues, the ones listed above and the one Walter reported.
>> It also adds a regression test for all of them.
>>
>> Can anyone come up with some more special cases that this patched or the
>> old conversion might handle incorrectly, for adding to the regression
>> suite?
>>
>>
>> One more thing: The precision works in a funny way, if you pass a "val"
>> with more bits than fit in precision. I filled the val-bytes with 0x9F
>> 0x86 0x01, which corresponds to 99999 in decimal. I passed that as a
>> param with different precisions:
>>
>> sign 1 prec 1 scale 0 val 9F8601:
>>       159
>> sign 1 prec 2 scale 0 val 9F8601:
>>       159
>> sign 1 prec 3 scale 0 val 9F8601:
>>       34463
>> sign 1 prec 4 scale 0 val 9F8601:
>>       34463
>> sign 1 prec 5 scale 0 val 9F8601:
>>       99999
>> sign 1 prec 6 scale 0 val 9F8601:
>>       99999
>>
>> The conversion algorithm seems to assume that there are no extra bits in
>> the val-array that don't fit within the precision. Perhaps that's a
>> reasonable assumption, but I think it would be more robust to ignore any
>> extra bits if they are there, rather than sometimes take some of them
>> into account. (Or, always parse all the bits, ignoring the precision.) I
>> did that in the attached patch.
>>
>> However, according to this article I found from Microsoft:
>> https://support.microsoft.com/kb/222831
>>
>> "The precision and scale fields of the numeric structure are never used
>> for input from an application, only for output from the driver to the
>> application."
>>
>> So we probably should be ignoring them completely, and get the precision
>> and scale from elsewhere.
>>
>>
>> PS. ResolveNumericParam always returns TRUE. That's fortunate, because
>> the caller would incorrectly fall through the switch-case if it didn't..
>> When the code was added, it fell through to the "default" case, but the
>> interval-cases were later added in between, breaking it. But it's not a
>> live bug, since ResolveNumericParam always returns TRUE. Fixed that too
>> in the attached patch.
>>
>> - Heikki
>>


--
- Heikki