Thread: Problem retrieving a numeric(38,0) value as SQL_NUMERIC_STRUCT if value needs to use all 16 SQLCHAR elements of the val array

Hi Walter,

Could you test a patch if I send it to you?

regards,
Hiroshi Inoue

(2014/06/03 4:03), Walter Couto wrote:
> Make sure you have the following table and data created in the database:
>
> CREATE TABLE NUMBER_TEST
>
> (
>
>      test               integer         NOT NULL,
>
>      numeric_double_col numeric(38,0)
>
>      CONSTRAINT number_test_pkey PRIMARY KEY (test)
>
> )
>
> ;
>
> INSERT INTO NUMBER_TEST VALUES ( 6,
> 12345678901234567890123456789012345678 );
>
> ;
>
> Execute the statement: select numeric_double_col from trunit.number_test
> where test = 6
>
> Using the ODBC driver, bind using SQL_NUMERIC_STRUCT.
>
> Expected values in the structure:
>
> precision = 38
>
> scale = 0
>
> sign = 1
>
> val = 0949B0F6F0023313C4499050DE38F34E
>
> Actual values in the structure:
>
> precision = 38
>
> scale = -1
>
> sign = 1
>
> val = 0049B0F6F0023313C4499050DE38F34E
>
> Other smaller values for the same column work fine.  Any ideas?
>
> Regards,
>
> Walter


--
I am using the free version of SPAMfighter.
SPAMfighter has removed 10487 of my spam emails to date.
Get the free SPAMfighter here: http://www.spamfighter.com/len

Do you have a slow PC? Try a Free scan
http://www.spamfighter.com/SLOW-PCfighter?cid=sigen

(2014/06/03 19:56), Walter Couto wrote:
> Yes I can

Thanks.
Please try the attached patch.

regards,
Hiroshi Inoue

> -------- Original message --------
> From: "Inoue, Hiroshi"
> Date:06/02/2014 11:47 PM (GMT-05:00)
> To: Walter Couto ,pgsql-odbc@postgresql.org
> Subject: Re: [ODBC] Problem retrieving a numeric(38,0) value as
> SQL_NUMERIC_STRUCT if value needs to use all 16 SQLCHAR elements of the
> val array
>
> Hi Walter,
>
> Could you test a patch if I send it to you?
>
> regards,
> Hiroshi Inoue

Hi Hiroshi,

This patch fixes the problem.

Regards,
Walter

-----Original Message-----
From: Hiroshi Inoue [mailto:inoue@tpf.co.jp]
Sent: Tuesday, June 03, 2014 11:24 PM
To: Walter Couto; pgsql-odbc@postgresql.org
Subject: Re: [ODBC] Problem retrieving a numeric(38,0) value as SQL_NUMERIC_STRUCT if value needs to use all 16 SQLCHAR
elementsof the val array 

(2014/06/03 19:56), Walter Couto wrote:
> Yes I can

Thanks.
Please try the attached patch.

regards,
Hiroshi Inoue

> -------- Original message --------
> From: "Inoue, Hiroshi"
> Date:06/02/2014 11:47 PM (GMT-05:00)
> To: Walter Couto ,pgsql-odbc@postgresql.org
> Subject: Re: [ODBC] Problem retrieving a numeric(38,0) value as
> SQL_NUMERIC_STRUCT if value needs to use all 16 SQLCHAR elements of
> the val array
>
> Hi Walter,
>
> Could you test a patch if I send it to you?
>
> regards,
> Hiroshi Inoue


CONFIDENTIALITY NOTICE: This email message is for the sole use of the intended recipient(s) and may contain
confidentialand privileged information. Any unauthorized review, use, disclosure or distribution is prohibited. If you
arenot the intended recipient, please contact the sender by reply email and destroy all copies of the original
message.

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

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
>

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