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

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;
>>
>>
>>