Re: Re: [PATCH] SQLFreeStmt deletes params, but does not reset the stmt->prepared state. - Mailing list pgsql-odbc

On 25 February 2015 15:27, Oliver Freyd wrote:
>Hi,
>
>I think I'm using the psql-odbc driver hand-patched with
>IPD_free_params() commented out, like you said, and it works
>fine for us.
>
>Isnt' this already merged into the git repository?
>I thought it sounded pretty obvious at the time when I found it,
>didn't bother to check afterwards...
>
>greets,
>Oliver

Hi,

I checked out the latest from git and the problem still exists.

Regards,
Jeremy Faith

>
>
>Am 24.02.2015 um 16:03 schrieb Faith, Jeremy:
>> Hi,
>>
>> I also found this issue with the PHP odbc extension which uses
>> SQL_RESET_PARAMS.
>> It causes the following error on the second odbc_execute() call.
>>    PHP Warning:  odbc_execute(): SQL error: Unfortunatley couldn't get this
>>                  paramater's info, SQL state S1000 in SQLDescribeParameter
>>
>>
>>  >Hi Oliver,
>>  >
>>  >On 2014/10/17 19:07, Oliver Freyd wrote:
>>  >> Hello,
>>  >>
>>  >> In ResolveOneParam (convert.c:4586) there is code that converts boolean
>>  >> '-1' to '1'. This is necessary for MS Access because it uses -1 as true
>>  >> representation.
>>  >>
>>  >> In my application, an Access 2013 frontend with a postgresql backend,
>>  >> sometimes this conversion did not work, and the backend would fail like
>>  >> this:
>>  >>
>>  >> invalid input syntax for type boolean: "-1" at character 399
>>  >>
>>  >> The statement is like this:
>>  >>
>>  >> DELETE FROM "public"."someview" WHERE "id" = 13020
>>  >> AND ... "someboolean" = '-1' ...
>>  >>
>>  >> This does not happen always, and I've seen it only in DELETE queries,
>>  >> when I try to delete a row that has a checkbox that is checked (true).
>>  >> The first try always works, the second try (deleting another row) it
>> fails.
>>  >>
>>  >> Now this is how it fails (IMHO):
>>  >>
>>  >> The ResolveOneParam() function relies on the correct types being
>>  >> assigned to the parameters, In qb->ipdopts->parameters there is
>>  >> SQLType and PGType. The SQLType is and enum that does not contain
>>  >> boolean, only the PGType can be boolean.
>>  >>
>>  >> In PGAPI_Execute() (execute.c:1025) it calls prepareParameters, where
>>  >> the PGTypes are found (don't know how exactly), and put into the stmt,
>>  >> as stmt->ipd->ipdf.parameters.
>>  >>
>>  >> If the PGTypes are not 0, the query runs fine. The second time
>>  >> the query is executed, the query is already prepared (stmt->prepared=5)
>>  >> and the prepare step is omitted. But then the PGTypes are missing,
>>  >> the bool '-1' -> '1' conversion is not done and the query fails.
>>  >>
>>  >> It turns out Access calls SQLFreeStmt(option=SQL_RESET_PARAMS),
>>  >> that does SC_free_params, so the params get deleted.
>>  >>
>>  >> But the params contain the result of the prepare call,
>>  >> so IMHO the stmt->prepared state is now wrong, the transaction is no
>>  >> more prepared, so stmt->prepared should be reset to 0.
>>  >>
>>  >> And that actually fixes the bug.
>>  >>
>>  >>
>>  >> I hope this is useful, it took quite a while to track this down...
>>  >
>>  >Thank you for your investigation.
>>  >
>>  >According to the following pages
>>  >  http://msdn.microsoft.com/en-us/library/ms709284%28v=vs.85%29.aspx
>>  >  http://msdn.microsoft.com/en-us/library/ms710926%28v=vs.85%29.aspx
>>  >
>>  >, it seems better to reset APD only keeping IPD intact.
>>  >
>>  >Comments?
>>
>>
>> I tried Oliver's patch and PHP no longer has a problem.
>> Inserting 100,000 rows into a simple table takes 47.6s
>> No sign of any memory leak.
>>
>> On the other hand commenting out the IPD_free_params() call also works.
>> Same 100,000 inserts take 34.1s, so significantly faster.
>> Again no sign of a memory leak. I also tried prepare,execute,free 100K
>> times, much slower but again no sign of a memory leak.
>>
>> I think the SC_Destructor() function will free the IPD params if nothing
>> else does. So, unless there is some other path that could cause a leak,
>> I would say just commenting out the IPD_free_params() call is a better
>> solution.
>>
>> Regards,
>> Jeremy Faith
>>
>>  >
>>  >regards,
>>  >Hiroshi Inoue
>>  >
>>  >> best regards,
>>  >>
>>  >>      Oliver Freyd
>>  >>
>>  >> -----------------------------------------------------------------------
>>  >> diff --git a/statement.c b/statement.c
>>  >> index da5abf5..a019d5d 100644
>>  >> --- a/statement.c
>>  >> +++ b/statement.c
>>  >> @@ -581,6 +581,7 @@ SC_free_params(StatementClass *self, char option)
>>  >>       {
>>  >>           APD_free_params(SC_get_APDF(self), option);
>>  >>           IPD_free_params(SC_get_IPDF(self), option);
>>  >> +        if (self->prepared!=NOT_YET_PREPARED)\
>>  >> SC_set_prepared(self, NOT_YET_PREPARED);
>>  >>       }
>>  >>       PDATA_free_params(SC_get_PDTI(self), option);
>>  >>       self->data_at_exec = -1;
>>  >>
>>  >>
>>  >>
Tyco Safety Products/CEM Systems Ltd.

________________________________

This e-mail contains privileged and confidential information intended for the use of the addressees named above. If you
arenot the intended recipient of this e-mail, you are hereby notified that you must not disseminate, copy or take any
actionin respect of any information contained in it. If you have received this e-mail in error, please notify the
senderimmediately by e-mail and immediately destroy this e-mail and its attachments.