Обсуждение: A platform-specific defect in the ODBC driver

Поиск
Список
Период
Сортировка

A platform-specific defect in the ODBC driver

От
Alex Goncharov
Дата:
Hello Hiroshi,

I hope I am addressing this message correctly.

Consider this code in `statement.c':

------------------------------------------------------------
static void SC_init_parse_method(StatementClass *self)
{
        if (self->multi_statement <= 0 && ...)
                SC_set_parse_tricky(self);
}

StatementClass *
SC_Constructor(ConnectionClass *conn)
{
                rv->multi_statement = -1; /* unknown */
}
------------------------------------------------------------

There is obviously an assumption here that `multi_statement' can,
under certain circumstances, be negative.  And, indeed, this is what
`statement.h' says about it:

------------------------------------------------------------
struct StatementClass_
{
        char            multi_statement; /* -1:unknown 0:single 1:multi */
};

------------------------------------------------------------

so, `multi_statement' is a three-valued entity, with -1 being one of
the possible values.

But what if `char' is an unsigned type?  Then assigning -1 to
`multi_statement' will result in it being 255, and comparisons like
`<= 0' won't make sense.

Can `char' be unsigned?  Yes, it can, per paragraph 15 in the ISO
C, Section 6.2.5:

   15 The three types char, signed char, and unsigned char are
      collectively called the character types. The implementation
      shall define char to have the same range, representation, and
      behavior as either signed char or unsigned char.35)

Various compilers on AIX consider `char' to be unsigned, and the code
compiled there (with gcc), will fail miserably on some operations.

A possible and probably the best resolution is to give
`multi_statement' and similar "to-be-three-valued" variables a good
"indicator" type, which can allow all the expected values, -1, 0 and 1
and legit comparisons between them.

The enclosed patch does it and resolves the failures on AIX
completely.

Please consider including this or a similar fix into the official
distribution of the ODBC driver.

Thanks,

-- Alex -- alex-goncharov@comcast.net --

============================================================
diff -rwup psqlodbc-08.03.0400.orig/bind.c psqlodbc-08.03.0400/bind.c
--- psqlodbc-08.03.0400.orig/bind.c    2008-10-06 17:46:09.000000000 -0400
+++ psqlodbc-08.03.0400/bind.c    2009-04-20 16:34:06.000000000 -0400
@@ -471,7 +471,7 @@ inolog("num_params=%d,%d\n", stmt->num_p
     }
     else
     {
-        char    multi = FALSE, proc_return = 0;
+        indicator_type    multi = FALSE, proc_return = 0;

         stmt->proc_return = 0;
         SC_scanQueryAndCountParams(stmt->statement, SC_get_conn(stmt), NULL, pcpar, &multi, &proc_return);
diff -rwup psqlodbc-08.03.0400.orig/convert.c psqlodbc-08.03.0400/convert.c
--- psqlodbc-08.03.0400.orig/convert.c    2008-10-29 09:40:54.000000000 -0400
+++ psqlodbc-08.03.0400/convert.c    2009-04-20 16:35:50.000000000 -0400
@@ -2323,7 +2323,8 @@ RETCODE    prep_params(StatementClass *stmt
     BOOL        ret, once_descr;
     ConnectionClass *conn = SC_get_conn(stmt);
     QResultClass    *res, *dest_res = NULL;
-    char        plan_name[32], multi;
+    char        plan_name[32];
+        indicator_type    multi;
     int        func_cs_count = 0;
     const char    *orgquery = NULL, *srvquery = NULL;
     Int4        endp1, endp2;
diff -rwup psqlodbc-08.03.0400.orig/pgtypes.h psqlodbc-08.03.0400/pgtypes.h
--- psqlodbc-08.03.0400.orig/pgtypes.h    2008-07-04 09:49:33.000000000 -0400
+++ psqlodbc-08.03.0400/pgtypes.h    2009-04-20 16:37:36.000000000 -0400
@@ -119,4 +119,7 @@ SQLSMALLINT    sqltype_to_default_ctype(con
 Int4        ctype_length(SQLSMALLINT ctype);

 #define    USE_ZONE    FALSE
+
+typedef signed char indicator_type;
+
 #endif
diff -rwup psqlodbc-08.03.0400.orig/statement.c psqlodbc-08.03.0400/statement.c
--- psqlodbc-08.03.0400.orig/statement.c    2008-09-21 11:35:44.000000000 -0400
+++ psqlodbc-08.03.0400/statement.c    2009-04-20 17:03:11.000000000 -0400
@@ -856,7 +856,7 @@ inolog("%s statement=%p ommitted=0\n", f
 void
 SC_scanQueryAndCountParams(const char *query, const ConnectionClass *conn,
         Int4 *next_cmd, SQLSMALLINT * pcpar,
-        char *multi_st, char *proc_return)
+        indicator_type *multi_st, indicator_type *proc_return)
 {
     CSTR func = "SC_scanQueryAndCountParams";
     char    literal_quote = LITERAL_QUOTE, identifier_quote = IDENTIFIER_QUOTE, dollar_quote = DOLLAR_QUOTE;
@@ -865,7 +865,8 @@ SC_scanQueryAndCountParams(const char *q
     char    tchar, bchar, escape_in_literal = '\0';
     char    in_literal = FALSE, in_identifier = FALSE,
         in_dollar_quote = FALSE, in_escape = FALSE,
-        del_found = FALSE, multi = FALSE;
+        del_found = FALSE;
+        indicator_type multi = FALSE;
     SQLSMALLINT    num_p;
     encoded_str    encstr;

diff -rwup psqlodbc-08.03.0400.orig/statement.h psqlodbc-08.03.0400/statement.h
--- psqlodbc-08.03.0400.orig/statement.h    2008-08-09 19:30:53.000000000 -0400
+++ psqlodbc-08.03.0400/statement.h    2009-04-20 16:44:43.000000000 -0400
@@ -225,25 +225,25 @@ struct StatementClass_
     Int2        current_exec_param;    /* The current parameter for
                          * SQLPutData */
     PutDataInfo    pdata_info;
-    char        parse_status;
-    char        proc_return;
-    char        put_data;    /* Has SQLPutData been called ? */
-    char        catalog_result;    /* Is this a result of catalog function ? */
-    char        prepare;    /* is this a prepared statement ? */
-    char        prepared;    /* is this statement already
+    indicator_type parse_status;
+    indicator_type proc_return;
+    indicator_type put_data;    /* Has SQLPutData been called ? */
+    indicator_type catalog_result;    /* Is this a result of catalog function ? */
+    indicator_type prepare;    /* is this a prepared statement ? */
+    indicator_type prepared;    /* is this statement already
                      * prepared at the server ? */
-    char        internal;    /* Is this statement being called
+    indicator_type internal;    /* Is this statement being called
                              * internally ? */
-    char        transition_status;    /* Transition status */
-    char        multi_statement; /* -1:unknown 0:single 1:multi */
-    char        rbonerr;    /* rollback on error */
-    char        discard_output_params;     /* discard output parameters on parse stage */
-    char        cancel_info;    /* cancel information */
-    char        ref_CC_error;    /* refer to CC_error ? */
-    char        lock_CC_for_rb;    /* lock CC for statement rollback ? */
-    char        join_info;    /* have joins ? */
-    char        parse_method;    /* parse_statement is forced or ? */
-    char        curr_param_result; /* current param result is set ? */
+    indicator_type transition_status;    /* Transition status */
+    indicator_type multi_statement; /* -1:unknown 0:single 1:multi */
+    indicator_type rbonerr;    /* rollback on error */
+    indicator_type discard_output_params;     /* discard output parameters on parse stage */
+    indicator_type cancel_info;    /* cancel information */
+    indicator_type ref_CC_error;    /* refer to CC_error ? */
+    indicator_type lock_CC_for_rb;    /* lock CC for statement rollback ? */
+    indicator_type join_info;    /* have joins ? */
+    indicator_type parse_method;    /* parse_statement is forced or ? */
+    indicator_type curr_param_result; /* current param result is set ? */
     pgNAME        cursor_name;
     char        *plan_name;

@@ -485,7 +485,7 @@ int        SC_set_current_col(StatementClass *
 void        SC_setInsertedTable(StatementClass *, RETCODE);
 void        SC_scanQueryAndCountParams(const char *, const ConnectionClass *,
             Int4 *next_cmd, SQLSMALLINT *num_params,
-            char *multi, char *proc_return);
+            indicator_type *multi, indicator_type *proc_return);

 BOOL    SC_IsExecuting(const StatementClass *self);
 BOOL    SC_SetExecuting(StatementClass *self, BOOL on);
============================================================

Re: A platform-specific defect in the ODBC driver

От
Hiroshi Inoue
Дата:
Hi Alex,

Alex Goncharov wrote:
> Hello Hiroshi,
>
> I hope I am addressing this message correctly.
>
> Consider this code in `statement.c':
>
> ------------------------------------------------------------
> static void SC_init_parse_method(StatementClass *self)
> {
>         if (self->multi_statement <= 0 && ...)
>                 SC_set_parse_tricky(self);
> }
>
> StatementClass *
> SC_Constructor(ConnectionClass *conn)
> {
>                 rv->multi_statement = -1; /* unknown */
> }
> ------------------------------------------------------------
>
> There is obviously an assumption here that `multi_statement' can,
> under certain circumstances, be negative.  And, indeed, this is what
> `statement.h' says about it:
>
> ------------------------------------------------------------
> struct StatementClass_
> {
>         char            multi_statement; /* -1:unknown 0:single 1:multi */
> };
>
> ------------------------------------------------------------
>
> so, `multi_statement' is a three-valued entity, with -1 being one of
> the possible values.
>
> But what if `char' is an unsigned type?  Then assigning -1 to
> `multi_statement' will result in it being 255, and comparisons like
> `<= 0' won't make sense.
>
> Can `char' be unsigned?  Yes, it can, per paragraph 15 in the ISO
> C, Section 6.2.5:
>
>    15 The three types char, signed char, and unsigned char are
>       collectively called the character types. The implementation
>       shall define char to have the same range, representation, and
>       behavior as either signed char or unsigned char.35)
>
> Various compilers on AIX consider `char' to be unsigned, and the code
> compiled there (with gcc), will fail miserably on some operations.

I see.

> A possible and probably the best resolution is to give
> `multi_statement' and similar "to-be-three-valued" variables a good
> "indicator" type, which can allow all the expected values, -1, 0 and 1
> and legit comparisons between them.
>
> The enclosed patch does it and resolves the failures on AIX
> completely.
>
> Please consider including this or a similar fix into the official
> distribution of the ODBC driver.

OK I would take care of it in 8.4.0100 release.

regards,
Hiroshi Inoue

Re: A platform-specific defect in the ODBC driver

От
Alex Goncharov
Дата:
,--- You/Hiroshi (Wed, 22 Apr 2009 22:04:21 +0900) ----*
| Alex Goncharov wrote:
| > A possible and probably the best resolution is to give
| > `multi_statement' and similar "to-be-three-valued" variables a good
| > "indicator" type, which can allow all the expected values, -1, 0 and 1
| > and legit comparisons between them.
| >
| > The enclosed patch does it and resolves the failures on AIX
| > completely.
| >
| > Please consider including this or a similar fix into the official
| > distribution of the ODBC driver.
|
| OK I would take care of it in 8.4.0100 release.
|
| regards,
| Hiroshi Inoue

Thank you, Hiroshi!

-- Alex -- alex-goncharov@comcast.net --