Horton

Thread

Commits

  1. Second try at fixing numeric data passed through an ECPG SQLDA.

  2. Fix incorrect results for numeric data passed through an ECPG SQLDA.

  1. [Bug Fix]ECPG: cancellation of significant digits on ECPG

    Higuchi, Daisuke <higuchi.daisuke@jp.fujitsu.com> — 2018-05-17T04:09:38Z

    Hi, 
Currently our customer uses PostgreSQL 9.6 and hits ECPG's bug during using numeric data type by SQLDA. 
I confirmed that this problem is occurred on master and 9.6 latest branch. 

PROBLEM
---------------------------------------
When the integer part of numeric data type is "0", cancellation of significant digits is occurred. 
For example, I expect "0.12345", but I got "0.12340". When I expect "0.01234", I got "0.01200"
I attached the sample application code to reproduce this problem. 

CAUSE
---------------------------------------
When copy the data of numeric data, the size is wrong. 
"src/interfaces/ecpg/ecpglib/sqlda.c" has problem. 

ecpg_set_native_sqlda(int lineno, struct sqlda_struct ** _sqlda, const PGresult *res, int row, enum COMPAT_MODE compat)
{
...
    if (num->ndigits)
    {
        ecpg_sqlda_align_add_size(next_offset, sizeof(int), num->ndigits + 1, &offset, &next_offset);
        memcpy((char *) sqlda + offset, num->buf, num->ndigits + 1);

        ((numeric *) sqlda->sqlvar[i].sqldata)->buf = (NumericDigit *) sqlda + offset;
        ((numeric *) sqlda->sqlvar[i].sqldata)->digits = (NumericDigit *) sqlda + offset + (num->digits - num->buf);
    }
...

When numeric data is "0.12345", num->buf has "0 0 1 2 3 4 5" and num->digits has "1 2 3 4 5". 
num->ndigits has the number of digits which is or later "1", it means 5.

In this code, currently copy "num->ndigits + 1" as size of numeric data. 
As a result, (char *) sqlda + offset has "0 0 1 2 3 4", not "0 0 1 2 3 4 5". 
So, "num->digits - num->buf + num->ndigits" should be copied. 

FIX
---------------------------------------
Above source code should be fixed and other similar bugs are fixed too.
I attached patches for bug fix and regression test for master branch. 
I hope this bug fix will be backport to other versions. 

Regards, 
Daisuke Higuchi

  2. RE: [Bug Fix]ECPG: cancellation of significant digits on ECPG

    Higuchi, Daisuke <higuchi.daisuke@jp.fujitsu.com> — 2018-05-25T02:11:05Z

    Hi, 
I have a question which is related to numeric data type bugs on ECPG sqlda. 

Currently, I think following functions have a problem when using numeric data type on sqlda. 
Please see the details of problem from the mail I sent before. 
[src/ecpg/ecpglib/sqlda.c]
- ecpg_set_native_sqlda()
- sqlda_common_total_size()
- ecpg_set_compat_sqlda()

However, I think some codes which have problem in ecpg_set_compat_sqlda() are not used. 

On ecpg_set_compat_sqlda(), there are own cases for numeric and decimal data type. 
-------------
ecpg_set_compat_sqlda(int lineno, struct sqlda_compat **_sqlda, const PGresult *res, int row, enum COMPAT_MODE compat)
{
     for (i = 0; i < sqlda->sqld; i++)
     {
          switch (sqlda->sqlvar[i].sqltype)
          {
          case ECPGt_decimal:
               ecpg_sqlda_align_add_size(offset, sizeof(double), sizeof(double), &offset, &next_offset);
          
          case ECPGt_numeric:
               ecpg_sqlda_align_add_size(next_offset, sizeof(int), num->ndigits + 1, &offset, &next_offset);
          
-------------

ecpg_set_compat_sqlda() is used only when INFORMIX_MODE(stmt->compat) is true. 
-------------
bool
ecpg_process_output(struct statement *stmt, bool clear_result)
{

        if (INFORMIX_MODE(stmt->compat))
        {
                sqlda_new = ecpg_build_compat_sqlda(stmt->lineno, stmt->results, i, stmt->compat);
                
                ecpg_set_compat_sqlda(stmt->lineno, _sqlda, stmt->results, i, stmt->compat);
                
        }
-------------

However, numeric data type could be changed to decimal by ecpg_build_compat_sqlda() in this case. 
-------------
ecpg_build_compat_sqlda(int line, PGresult *res, int row, enum COMPAT_MODE compat)
{
                sqlda->sqlvar[i].sqltype = sqlda_dynamic_type(PQftype(res, i), compat);

sqlda_dynamic_type(Oid type, enum COMPAT_MODE compat)
{
        switch (type)
        {
                case NUMERICOID:
                        return INFORMIX_MODE(compat) ? ECPGt_decimal : ECPGt_numeric;
-------------

Could we remove some codes for numeric in ecpg_set_compat_sqlda()? 

Regards, 
Daisuke, Higuchi

  3. Re: [Bug Fix]ECPG: cancellation of significant digits on ECPG

    Dmitry Dolgov <9erthalion6@gmail.com> — 2018-11-05T21:51:39Z

    > On Thu, 17 May 2018 at 06:10, Higuchi, Daisuke <higuchi.daisuke@jp.fujitsu.com> wrote:
>
> Currently our customer uses PostgreSQL 9.6 and hits ECPG's bug during using numeric data type by SQLDA.
> I confirmed that this problem is occurred on master and 9.6 latest branch.
>
> FIX
> ---------------------------------------
> Above source code should be fixed and other similar bugs are fixed too.
> I attached patches for bug fix and regression test for master branch.
> I hope this bug fix will be backport to other versions.

Hi,

Thanks for the patches. Unfortunately, judging from the cfbot.cputube.org they
can't be applied anymore to the current master, could you please rebase them?

  4. RE: [Bug Fix]ECPG: cancellation of significant digits on ECPG

    Higuchi, Daisuke <higuchi.daisuke@jp.fujitsu.com> — 2018-11-06T09:19:36Z

    From: Dmitry Dolgov [mailto:9erthalion6@gmail.com] 
> Thanks for the patches. Unfortunately, judging from the cfbot.cputube.org they
> can't be applied anymore to the current master, could you please rebase them?

Thank you for checking!
I rebased patches on the current master, so I attach them. 

Regards, 
Daisuke Higuchi

  5. Re: [Bug Fix]ECPG: cancellation of significant digits on ECPG

    Dmitry Dolgov <9erthalion6@gmail.com> — 2018-11-06T14:03:03Z

    > On Tue, 6 Nov 2018 at 10:19, Higuchi, Daisuke <higuchi.daisuke@jp.fujitsu.com> wrote:
>
> Thank you for checking!
> I rebased patches on the current master, so I attach them.

After adding 'EXEC SQL ALLOCATE DESCRIPTOR sqlda' I've managed to reproduce the
problem you're talking about, and indeed it looks strange:

=# table testtab ;
   c1
---------
1.23456
0.12345
0.01234
(3 rows)

but in ecpg program we've got from gdb:

# for the second record 0.12345
$$1 = {
  ndigits = 5,
  weight = -1,
  rscale = 5,
  dscale = 5,
  sign = 0,
  buf = 0x5555557636d8 "",
  digits = 0x5555557636da "\001\002\003\004"
}

# for the third record 0.01234
$$0 = {
  ndigits = 4,
  weight = -2,
  rscale = 5,
  dscale = 5,
  sign = 0,
  buf = 0x555555763578 "",
  digits = 0x55555576357b "\001\002"
}

Also what's strange for me is that after applying your patches I still got the
same output, not sure why:

    ./numeric_test
    ndigits :6
    buf     :0   1   2   3   4   5   6
    digits  :1   2   3   4   5   6
    numeric :1.23456
    ----------------------------
    ndigits :5
    buf     :0   0   1   2   3   4   0
    digits  :1   2   3   4   0
    numeric :0.12340
    ----------------------------
    ndigits :4
    buf     :0   0   0   1   2   0   0
    digits  :1   2   0   0
    numeric :0.01200
    ----------------------------

  6. RE: [Bug Fix]ECPG: cancellation of significant digits on ECPG

    Higuchi, Daisuke <higuchi.daisuke@jp.fujitsu.com> — 2018-11-07T08:22:34Z

    From: Dmitry Dolgov [mailto:9erthalion6@gmail.com] 
>Also what's strange for me is that after applying your patches I still got the
>same output, not sure why:
Hmm... In my environment, the output was changed. 
I also updated regression test and cfbot.cputube.org reports no problem now. 

Do you use the sample application which I attached on the first posted mail? 
If not, could you please share the details? 

Regards, 
Daisuke Higuchi

  7. Re: [Bug Fix]ECPG: cancellation of significant digits on ECPG

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-11-12T23:07:20Z

    "Higuchi, Daisuke" <higuchi.daisuke@jp.fujitsu.com> writes:
> From: Dmitry Dolgov [mailto:9erthalion6@gmail.com] 
>> Thanks for the patches. Unfortunately, judging from the cfbot.cputube.org they
>> can't be applied anymore to the current master, could you please rebase them?

> Thank you for checking!
> I rebased patches on the current master, so I attach them. 

I took a quick look at this.  I concur that the code is broken as-is;
it's failing to reproduce the state of the digit buffer accurately.
However, I think there's a second bug, which is that it doesn't even
try to duplicate the state when ndigits = 0.  That basically means that
the sqlda area might contain dangling pointers (they'll point to the
PGTYPESnumeric_from_asc output, which is freed immediately after we
set up the sqlda copy).  Now *maybe* that doesn't lead to any obvious
problem, but I don't think that this code deserves any expectation of
correctness given that you just found such a large bug in it.  So
I think that we ought to unconditionally make the sqlda value's digit
buffer look just like the one we're copying, even when ndigits = 0,
which just requires removing the tests on ndigits.

I also noted that the comment adjacent to this code was badly obsolete.

In short, the attached.  (I didn't bother to keep the test changes
separate from the code fix.)

			regards, tom lane

  8. RE: [Bug Fix]ECPG: cancellation of significant digits on ECPG

    Higuchi, Daisuke <higuchi.daisuke@jp.fujitsu.com> — 2018-11-14T00:53:48Z

    From: Tom Lane [mailto:tgl@sss.pgh.pa.us] 
>I took a quick look at this. 
Thank you for review. 

>So I think that we ought to unconditionally make the sqlda value's digit 
>buffer look just like the one we're copying, even when ndigits = 0, 
>which just requires removing the tests on ndigits.

I agree with you. Seeing this thread[1], 'if (ndigits = 0)' was introduced only to avoid memcpy() crash. I do not know this solution was best or not, but no crash occurs in the current version. So, I also think 'if (ndigits = 0)' should be removed.

>In short, the attached.
Thank you for updating. This patch looks good to me. 

[1]
https://www.postgresql.org/message-id/4EC825F3.5080504%40cybertec.at

Regards,
Daisuke Higuchi

  9. Re: [Bug Fix]ECPG: cancellation of significant digits on ECPG

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-11-14T01:09:37Z

    "Higuchi, Daisuke" <higuchi.daisuke@jp.fujitsu.com> writes:
> From: Tom Lane [mailto:tgl@sss.pgh.pa.us] 
>> So I think that we ought to unconditionally make the sqlda value's digit 
>> buffer look just like the one we're copying, even when ndigits = 0, 
>> which just requires removing the tests on ndigits.

> I agree with you. Seeing this thread[1], 'if (ndigits = 0)' was introduced only to avoid memcpy() crash. I do not know this solution was best or not, but no crash occurs in the current version. So, I also think 'if (ndigits = 0)' should be removed.

Hmmm ... looking at PGTYPESnumeric_from_asc, it seems like the current
behavior is different from what was described in that old thread; the only
case where a digit buffer wouldn't be created is a NaN.  But maybe a crash
could occur for NaN.  Perhaps we should use "if (num->sign !=
NUMERIC_NAN)" as a guard?

			regards, tom lane

  10. Re: [Bug Fix]ECPG: cancellation of significant digits on ECPG

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-11-14T16:32:15Z

    I wrote:
> Hmmm ... looking at PGTYPESnumeric_from_asc, it seems like the current
> behavior is different from what was described in that old thread; the only
> case where a digit buffer wouldn't be created is a NaN.  But maybe a crash
> could occur for NaN.  Perhaps we should use "if (num->sign !=
> NUMERIC_NAN)" as a guard?

After sleeping on it, it seems like the right thing to check for is
whether num->buf is NULL, which describes precisely the situation
where we should not try to make a copy of the digit buffer (and the
initial struct memcpy has made a valid copy of the null pointers).
So I fixed it like that.

			regards, tom lane