Thread: [PATCH] SQLFreeStmt deletes params, but does not reset the stmt->prepared state.
[PATCH] SQLFreeStmt deletes params, but does not reset the stmt->prepared state.
From
Oliver Freyd
Date:
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... 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;
Attachment
Re: [PATCH] SQLFreeStmt deletes params, but does not reset the stmt->prepared state.
From
"Inoue, Hiroshi"
Date:
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? 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; > > >
Re: [PATCH] SQLFreeStmt deletes params, but does not reset the stmt->prepared state.
From
Oliver Freyd
Date:
>> 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? > Thank you for your quick reply, at first sight that seems to be better, more efficient as the statement keeps its prepared state. Only thing is I don't know if that would lead to a memory leak later, no idea when the IPD params are freed. I can try if that works, too... regards, Oliver Freyd > 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; >> >> >> > >