Thread: Re: [INTERFACES] Postgres odbc driver bug
OK, I have created a context diff of your changes, which is attached.
Can someone comment on this? The patch appears to deal with
STMT_PREMATURE differently than our current code.
In fact, it seems to disable this code:
/*
* If the statement is premature, it means we already executed it from
* an SQLPrepare/SQLDescribeCol type of scenario. So just return
* success.
*/
if (stmt->prepare && stmt->status == STMT_PREMATURE)
> We have found a reproducible problem with the Postgres ODBC driver. It has
> to do with the parameter replacement feature in the driver. Below is a
> piece of test code that shows the problem. We were able to fix the problem
> by making a change to execute.c in the ODBC driver code (attached).
>
> odbc driver version : Found in both 6.50 and 7.01.0004.
> postgresql database version : Found with both 7.0.2 and 7.0.3 versions of
> Postgres.
> Server Operating System: Windows2000
> Client Operation System: Windows2000
>
> Explanation of the odbc driver bug:
>
>
> /*****************************************
> * ERROR HAPPENS HERE:
> * If we set the buffer that we binded to column "text1" now, it will
> FAIL.
> * The SQLExecute in doQuery() will succeed (finding 0 rows), and the
> * SQLFetchScroll call will return SQL_NO_DATA_FOUND.
> *
> * This is because of a bug in the odbc driver. The call to
> SQLNumResultCols
> * will execute the query in the driver, but the buffer that is binded is
>
> * empty, so no rows will be found. This of course assumes that the
> table
> * doesn't contain any empty records for the "text1" column, which is
> correct
> * for our test. Then when we call SQLExcute, the driver will realize it
> has
> * already done the query work and will not do it again, even though our
> * buffer for parameter substitution has changed (next line of code below
>
> * this comment).
> *****************************************/
>
> Zip file is attached. The code is currently set up to fail, but I describe
> in my comments that you can uncomment the code that sets the buffer before
> SQLNumResultCols to make things will work.
>
> Thanks for your attention,
> Keith Millard
>
>
> _____
>
> <http://www.swifttouch.com/SwiftCardJump.asp?SwiftLink=95NN9NB1Q1FWA> Alex
> Shore..
> Senior Software Engineer..
> Pumatech..
> tel 603-888-0666 x1017..
> ..
> ashore@pumatech.com..
> www.pumatech.com..
>
>
[ Attachment, skipping... ]
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo@postgresql.org so that your
> message can get through to the mailing list cleanly
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
*** e2 Tue May 8 12:49:51 2001
--- execute.c Tue May 8 12:47:48 2001
***************
*** 204,209 ****
--- 204,210 ----
it from an SQLPrepare/SQLDescribeCol type of scenario. So
just return success.
*/
+ #if 0 //chuck
if ( stmt->prepare && stmt->status == STMT_PREMATURE) {
stmt->status = STMT_FINISHED;
if (stmt->errormsg == NULL) {
***************
*** 216,221 ****
--- 217,223 ----
return SQL_ERROR;
}
}
+ #endif
mylog("%s: clear errors...\n", func);
***************
*** 248,255 ****
}
/* Check if the statement is in the correct state */
! if ((stmt->prepare && stmt->status != STMT_READY) ||
! (stmt->status != STMT_ALLOCATED && stmt->status != STMT_READY)) {
stmt->errornumber = STMT_STATUS_ERROR;
stmt->errormsg = "The handle does not point to a statement that is ready to be executed";
--- 250,259 ----
}
/* Check if the statement is in the correct state */
! //chuck if ((stmt->prepare && stmt->status != STMT_READY) ||
! //chuck (stmt->status != STMT_ALLOCATED && stmt->status != STMT_READY)) {
! if ((stmt->prepare && (stmt->status != STMT_READY && stmt->status != STMT_PREMATURE)) || //chuck
! (stmt->status != STMT_ALLOCATED && stmt->status != STMT_READY && stmt->status != STMT_PREMATURE)) { //chuck
stmt->errornumber = STMT_STATUS_ERROR;
stmt->errormsg = "The handle does not point to a statement that is ready to be executed";
***************
*** 262,270 ****
--- 266,277 ----
/* The bound parameters could have possibly changed since the last execute
of this statement? Therefore check for params and re-copy.
*/
+ mylog("%s: Check for data at execution parameters\n", func);
+ mylog("%s: stmt->parameters_allocated = %d\n", func,stmt->parameters_allocated);
stmt->data_at_exec = -1;
for (i = 0; i < stmt->parameters_allocated; i++) {
/* Check for data at execution parameters */
+ mylog("%s: stmt->parameters[%d].data_at_exec = %d\n", func,i,stmt->parameters[i].data_at_exec);
if ( stmt->parameters[i].data_at_exec == TRUE) {
if (stmt->data_at_exec < 0)
stmt->data_at_exec = 1;
Bruce Momjian wrote: > > OK, I have created a context diff of your changes, which is attached. > Can someone comment on this? The patch appears to deal with > STMT_PREMATURE differently than our current code. > > In fact, it seems to disable this code: > > /* > * If the statement is premature, it means we already executed it from > * an SQLPrepare/SQLDescribeCol type of scenario. So just return > * success. > */ > if (stmt->prepare && stmt->status == STMT_PREMATURE) > Though this fixes the bug reported by Keith, STMT_PREMATURE loses its meaning and SC_pre_execute() does a non-essential work as a result. Possibly SQLPrepare() must be rewritten though I'm not sure. regards, Hiroshi Inoue
> Bruce Momjian wrote:
> >
> > OK, I have created a context diff of your changes, which is
> attached.
> > Can someone comment on this? The patch appears to deal with
> > STMT_PREMATURE differently than our current code.
> >
> > In fact, it seems to disable this code:
> >
> > /*
> > * If the statement is premature, it means we already executed it from
> > * an SQLPrepare/SQLDescribeCol type of scenario. So just return
> > * success.
> > */
> > if (stmt->prepare && stmt->status == STMT_PREMATURE)
> >
>
> Though this fixes the bug reported by Keith, STMT_PREMATURE
> loses its meaning and SC_pre_execute() does a non-essential
> work as a result. Possibly SQLPrepare() must be rewritten
> though I'm not sure.
OK, I see you have applied your patch:
1) [ODBC] Psqlodbc and Centura: here it is a patch
posted by Matteo Cavalleli
2) [ODBC] pgsqODBC binding parameters II
posted by Ludek Finstrle
3) Invalid Page Fault in PSQLODBC.DLL
personal mail from Johann Zuschlag
I removed it from the outstanding patches page.
And you are saying that this patch fixes a different problem. The user
reported his problems were fixed by that patch, so I am inclined to
apply it.
Thanks.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Bruce Momjian wrote: > > > Bruce Momjian wrote: > > > > > > OK, I have created a context diff of your changes, which is > > attached. > > > Can someone comment on this? The patch appears to deal with > > > STMT_PREMATURE differently than our current code. > > > > > > In fact, it seems to disable this code: > > > > > > /* > > > * If the statement is premature, it means we already executed it from > > > * an SQLPrepare/SQLDescribeCol type of scenario. So just return > > > * success. > > > */ > > > if (stmt->prepare && stmt->status == STMT_PREMATURE) > > > > > > > Though this fixes the bug reported by Keith, STMT_PREMATURE > > loses its meaning and SC_pre_execute() does a non-essential > > work as a result. Possibly SQLPrepare() must be rewritten > > though I'm not sure. > [snip] > > And you are saying that this patch fixes a different problem. The user > reported his problems were fixed by that patch, so I am inclined to > apply it. > I don't agree to apply the patch because the patch makes the current code hardly understandable. regards, Hiroshi inoue
> > And you are saying that this patch fixes a different problem. The user > > reported his problems were fixed by that patch, so I am inclined to > > apply it. > > > > I don't agree to apply the patch because the patch makes > the current code hardly understandable. Agreed. I mentioned to Keith I was going to clean it up before application. Here is the cleaned up patch. Hiroshi, what do you think? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026 Index: src/interfaces/odbc/execute.c =================================================================== RCS file: /home/projects/pgsql/cvsroot/pgsql/src/interfaces/odbc/execute.c,v retrieving revision 1.29 diff -c -r1.29 execute.c *** src/interfaces/odbc/execute.c 2001/05/08 17:12:36 1.29 --- src/interfaces/odbc/execute.c 2001/05/09 02:37:04 *************** *** 217,248 **** return SQL_INVALID_HANDLE; } - /* - * If the statement is premature, it means we already executed it from - * an SQLPrepare/SQLDescribeCol type of scenario. So just return - * success. - */ - if (stmt->prepare && stmt->status == STMT_PREMATURE) - { - if (stmt->inaccurate_result) - SC_recycle_statement(stmt); - else - { - stmt->status = STMT_FINISHED; - if (stmt->errormsg == NULL) - { - mylog("%s: premature statement but return SQL_SUCCESS\n", func); - return SQL_SUCCESS; - } - else - { - SC_log_error(func, "", stmt); - mylog("%s: premature statement so return SQL_ERROR\n", func); - return SQL_ERROR; - } - } - } - mylog("%s: clear errors...\n", func); SC_clear_error(stmt); --- 217,222 ---- *************** *** 278,285 **** } /* Check if the statement is in the correct state */ ! if ((stmt->prepare && stmt->status != STMT_READY) || ! (stmt->status != STMT_ALLOCATED && stmt->status != STMT_READY)) { stmt->errornumber = STMT_STATUS_ERROR; stmt->errormsg = "The handle does not point to a statement that is ready to be executed"; --- 252,259 ---- } /* Check if the statement is in the correct state */ ! if ((stmt->prepare && (stmt->status != STMT_READY && stmt->status != STMT_PREMATURE)) || ! (stmt->status != STMT_ALLOCATED && stmt->status != STMT_READY && stmt->status != STMT_PREMATURE)) { stmt->errornumber = STMT_STATUS_ERROR; stmt->errormsg = "The handle does not point to a statement that is ready to be executed"; *************** *** 294,310 **** */ if (!stmt->pre_executing) { - /* * The bound parameters could have possibly changed since the last * execute of this statement? Therefore check for params and * re-copy. */ stmt->data_at_exec = -1; for (i = 0; i < stmt->parameters_allocated; i++) { Int4 *pcVal = stmt->parameters[i].used; if (pcVal && (*pcVal == SQL_DATA_AT_EXEC || *pcVal <= SQL_LEN_DATA_AT_EXEC_OFFSET)) stmt->parameters[i].data_at_exec = TRUE; else --- 268,286 ---- */ if (!stmt->pre_executing) { /* * The bound parameters could have possibly changed since the last * execute of this statement? Therefore check for params and * re-copy. */ + mylog("%s: Check for data at execution parameters\n", func); + mylog("%s: stmt->parameters_allocated = %d\n", func,stmt->parameters_allocated); stmt->data_at_exec = -1; for (i = 0; i < stmt->parameters_allocated; i++) { Int4 *pcVal = stmt->parameters[i].used; + mylog("%s: stmt->parameters[%d].data_at_exec = %d\n", func,i,stmt->parameters[i].data_at_exec); if (pcVal && (*pcVal == SQL_DATA_AT_EXEC || *pcVal <= SQL_LEN_DATA_AT_EXEC_OFFSET)) stmt->parameters[i].data_at_exec = TRUE; else
Bruce Momjian wrote: > > > > And you are saying that this patch fixes a different problem. The user > > > reported his problems were fixed by that patch, so I am inclined to > > > apply it. > > > > > > > I don't agree to apply the patch because the patch makes > > the current code hardly understandable. > > Agreed. I mentioned to Keith I was going to clean it up before > application. Here is the cleaned up patch. > > Hiroshi, what do you think? > Hmm that's not what I mean. The patch essentially means that the STMT_PREMATURE state is unnecessary. Well why is the concept *PREMATURE* introduced ? What meaning does the stuff related to the concept *PREMATURE* have ? IMHO we have to reconsider from the first. regards, Hiroshi Inoue
> > Agreed. I mentioned to Keith I was going to clean it up before > > application. Here is the cleaned up patch. > > > > Hiroshi, what do you think? > > > > Hmm that's not what I mean. The patch essentially means > that the STMT_PREMATURE state is unnecessary. Well why is > the concept *PREMATURE* introduced ? What meaning does the > stuff related to the concept *PREMATURE* have ? IMHO we > have to reconsider from the first. Oh, so you need Keith to explain the patch. OK. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Bruce Momjian wrote: > > > > Agreed. I mentioned to Keith I was going to clean it up before > > > application. Here is the cleaned up patch. > > > > > > Hiroshi, what do you think? > > > > > > > Hmm that's not what I mean. The patch essentially means > > that the STMT_PREMATURE state is unnecessary. Well why is > > the concept *PREMATURE* introduced ? What meaning does the > > stuff related to the concept *PREMATURE* have ? IMHO we > > have to reconsider from the first. > > Oh, so you need Keith to explain the patch. OK. > I got the following mail from Keith personally about 10 hours ago. I don't like the temporary fix. It also loses the advantge of *PREMATURE* handling. [A part of his mail] > I'm not saying that our fix is the long-term solution > to this problem but it does demonstrate the issue we > found and a possible fix. What we were not sure of > is what other ramifications are there with the fix we made. regards, Hiroshi Inoue
> I got the following mail from Keith personally about > 10 hours ago. I don't like the temporary fix. It also > loses the advantge of *PREMATURE* handling. > > [A part of his mail] > > > I'm not saying that our fix is the long-term solution > > to this problem but it does demonstrate the issue we > > found and a possible fix. What we were not sure of > > is what other ramifications are there with the fix we made. Yes, I emailed him and asked that he test your fixes. My guess is that you fixed the cause of his problem already in 7.1.1. Removing the patch from the outstanding patches list. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026