You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Ronen Mizrahi <ro...@tversity.com> on 2005/09/07 00:15:02 UTC
possible bug in apr_dbd_sqlite3.c
Hello,
In sqlite3 "END TRANSACTION" and "COMMIT" are synonyms, yet the code
below, taken from the svn head of apr_dbd_sqlite3.c, invokes them both
resulting in sqlite returning an SQL error when doing the commit becuase
there is no active transaction. Can someone that knows sqlite better
please check it and confirm this? I would like to know if I am correct
or I am missing something. As a side comment, in case you are wondering,
I just realized that this issue was not detected earlier due to various
bugs in dbd_sqlite3_query() (now they are fixed, thank you to whoever
fixed them, BTW) that prevented the correct error code from being returned.
Thank you,
Ronen
static int dbd_sqlite3_end_transaction(apr_dbd_transaction_t *trans)
{
int ret = 0;
int nrows = 0;
if (trans) {
ret = dbd_sqlite3_query(trans->handle, &nrows, "END TRANSACTION;");
if (trans->errnum) {
trans->errnum = 0;
ret = dbd_sqlite3_query(trans->handle, &nrows, "ROLLBACK;");
} else {
ret = dbd_sqlite3_query(trans->handle, &nrows, "COMMIT;");
}
trans->handle->trans = NULL;
}
return ret;
}
Re: possible bug in apr_dbd_sqlite3.c
Posted by Ronen Mizrahi <ro...@tversity.com>.
Yes, I meant query and not select. I ended up doing exactly what you
suggest, i.e. ignore the start and end transaction altogether. Yet, IMHO
a DB api that has transactions is conceptually incomplete without the
possibility to roll back, I mean how can the application writer enjoy
the atomicity offered by the database without it? and if no atomicity is
possible, why use transactions?
While it is of-course important to rollback a transaction when an error
condition occurs in the DB (or DB driver), the far more common situation
for rollback is to have errors in the application level, of which the DB
and the DB driver know nothing of (and should know nothing), this is why
I was saying before that apr_dbd is a conceptually incomplete interface
with regard to transactions. I love the simplicity of the interface and
I think it should be maintained, but ending up with a conceptually
incomplete interface is not the right thing to do. I think there are
only two possibilities to fix this, either apr_dbd_transaction is
removed or the ability to roll back is added.
Nick Kew wrote:
>On Friday 16 September 2005 19:55, Ronen Mizrahi wrote:
>
>
>>Thank you for responding.
>>May I ask also a question about the apr_dbd interface and how it should
>>be used with sqlite in order to roll back a transaction. I was looking
>>for a way to rollback a transaction, and it looks like the interface
>>right now allows one to start a transaction and to commit it but it does
>>not allow one to roll it back. I ended up using apr_dbd_select() with an
>>SQL string of "ROLLBACK",
>>
>>
>
>Erm, presumably you mean query rather than select?
>
>
>
>>is this how the interface intends for users to
>>do it?
>>
>>
>
>Yes. It's a simple interface, leaving you the option to use SQL or
>native-C-API to extend it where necessary. If your application wants
>rollback in cases other than an error condition, you should probably
>ignore apr_dbd_transaction and do it yourself. Doing it with
>apr_dbd_query and SQL commands should be portable across most
>backends:-)
>
>
>
>>Should there perhaps be function in apr_dbd for rollback?
>>
>>
>
>It's a thought for future versions: maybe end_transaction could have
>a parameter to indicate commit or rollback. But API changes that break
>back-compatibility will be treated with caution, and can only happen in
>a major version change.
>
>
>
Re: possible bug in apr_dbd_sqlite3.c
Posted by Nick Kew <ni...@webthing.com>.
On Friday 16 September 2005 19:55, Ronen Mizrahi wrote:
> Thank you for responding.
> May I ask also a question about the apr_dbd interface and how it should
> be used with sqlite in order to roll back a transaction. I was looking
> for a way to rollback a transaction, and it looks like the interface
> right now allows one to start a transaction and to commit it but it does
> not allow one to roll it back. I ended up using apr_dbd_select() with an
> SQL string of "ROLLBACK",
Erm, presumably you mean query rather than select?
> is this how the interface intends for users to
> do it?
Yes. It's a simple interface, leaving you the option to use SQL or
native-C-API to extend it where necessary. If your application wants
rollback in cases other than an error condition, you should probably
ignore apr_dbd_transaction and do it yourself. Doing it with
apr_dbd_query and SQL commands should be portable across most
backends:-)
> Should there perhaps be function in apr_dbd for rollback?
It's a thought for future versions: maybe end_transaction could have
a parameter to indicate commit or rollback. But API changes that break
back-compatibility will be treated with caution, and can only happen in
a major version change.
--
Nick Kew
Re: possible bug in apr_dbd_sqlite3.c
Posted by Ronen Mizrahi <ro...@tversity.com>.
That's my point exactly, it may lead to problems and there is no doubt
that there are cases where despite the fact that no error occured in all
the invocations of the apr_dbd APIs the application may encounter some
other error that would require a rollback. These kind of cases could
create some problems when rollink back using the workaround I mentioned
since some internal state (related to transactions) maintained in
apr_dbd (or one of it's drivers) might become inconsistent.
Rick Keiner wrote:
>The interface only provides a way to start the
>transaction and end the transaction. The "ROLLBACK"
>will be performed if there is an error reported from
>the database during any part of the transaction. This
>doesn't give the application, through the interface,
>any way to roll back the transaction based on some
>other criteria. I say "through the interface" since
>the apr_dbd_query does give you a way to roll it back.
>
>Is this adequate? Should there be an explicit function
>in the API for rolling back a transaction? I can see
>where mixing API calls and using explicit SQL commands
>can lead to problems.
>
>Rick Keiner
>
>--- Ronen Mizrahi <ro...@tversity.com> wrote:
>
>
>
>>Thank you for responding.
>>May I ask also a question about the apr_dbd
>>interface and how it should
>>be used with sqlite in order to roll back a
>>transaction. I was looking
>>for a way to rollback a transaction, and it looks
>>like the interface
>>right now allows one to start a transaction and to
>>commit it but it does
>>not allow one to roll it back. I ended up using
>>apr_dbd_select() with an
>>SQL string of "ROLLBACK", is this how the interface
>>intends for users to
>>do it? Should there perhaps be function in apr_dbd
>>for rollback?
>>
>>Rick Keiner wrote:
>>
>>
>>
>>>I have been working on that and you're right, the
>>>
>>>
>>"END TRANSACTION" is not
>>
>>
>>>needed and makes transactions fail. I removed it
>>>
>>>
>>and the test case works
>>
>>
>>>fine. I'll get a patch to the list soon. I've been
>>>
>>>
>>working on a seperate
>>
>>
>>>problem with the new release of SQLite that may
>>>
>>>
>>effect the driver. I would
>>
>>
>>>like to complete that first and then submit a
>>>
>>>
>>patch to the list.
>>
>>
>>>*/Garrett Rooney <ro...@electricjellyfish.net>/*
>>>
>>>
>>wrote:
>>
>>
>>> On 9/15/05, Ronen Mizrahi wrote:
>>> > Yet again, I would like to ask if anyone has
>>>
>>>
>>any feedback on
>>
>>
>>> this? If
>>> > someone out there is pondering some issues
>>>
>>>
>>about this, please
>>
>>
>>> respond,
>>> > perhaps a discussion may help and at the
>>>
>>>
>>least I would know that
>>
>>
>>> someone
>>> > is planning to address this issue.
>>>
>>> I suspect nobody has gotten around to looking
>>>
>>>
>>at it. If you think
>>
>>
>>> there's a problem, a patch to correct it or at
>>>
>>>
>>least to add a test
>>
>>
>>> that shows the issue would certainly be
>>>
>>>
>>appreciated. As it stands,
>>
>>
>>> it'll just have to wait until someone who
>>>
>>>
>>understands the sqlite code
>>
>>
>>> gets some free time.
>>>
>>> -garrett
>>>
>>>
>>>
>>
>>
>
>
>
>
Re: possible bug in apr_dbd_sqlite3.c
Posted by Rick Keiner <ri...@yahoo.com>.
The interface only provides a way to start the
transaction and end the transaction. The "ROLLBACK"
will be performed if there is an error reported from
the database during any part of the transaction. This
doesn't give the application, through the interface,
any way to roll back the transaction based on some
other criteria. I say "through the interface" since
the apr_dbd_query does give you a way to roll it back.
Is this adequate? Should there be an explicit function
in the API for rolling back a transaction? I can see
where mixing API calls and using explicit SQL commands
can lead to problems.
Rick Keiner
--- Ronen Mizrahi <ro...@tversity.com> wrote:
> Thank you for responding.
> May I ask also a question about the apr_dbd
> interface and how it should
> be used with sqlite in order to roll back a
> transaction. I was looking
> for a way to rollback a transaction, and it looks
> like the interface
> right now allows one to start a transaction and to
> commit it but it does
> not allow one to roll it back. I ended up using
> apr_dbd_select() with an
> SQL string of "ROLLBACK", is this how the interface
> intends for users to
> do it? Should there perhaps be function in apr_dbd
> for rollback?
>
> Rick Keiner wrote:
>
> > I have been working on that and you're right, the
> "END TRANSACTION" is not
> > needed and makes transactions fail. I removed it
> and the test case works
> > fine. I'll get a patch to the list soon. I've been
> working on a seperate
> > problem with the new release of SQLite that may
> effect the driver. I would
> > like to complete that first and then submit a
> patch to the list.
> >
> > */Garrett Rooney <ro...@electricjellyfish.net>/*
> wrote:
> >
> > On 9/15/05, Ronen Mizrahi wrote:
> > > Yet again, I would like to ask if anyone has
> any feedback on
> > this? If
> > > someone out there is pondering some issues
> about this, please
> > respond,
> > > perhaps a discussion may help and at the
> least I would know that
> > someone
> > > is planning to address this issue.
> >
> > I suspect nobody has gotten around to looking
> at it. If you think
> > there's a problem, a patch to correct it or at
> least to add a test
> > that shows the issue would certainly be
> appreciated. As it stands,
> > it'll just have to wait until someone who
> understands the sqlite code
> > gets some free time.
> >
> > -garrett
> >
>
>
Re: possible bug in apr_dbd_sqlite3.c
Posted by Ronen Mizrahi <ro...@tversity.com>.
Thank you for responding.
May I ask also a question about the apr_dbd interface and how it should
be used with sqlite in order to roll back a transaction. I was looking
for a way to rollback a transaction, and it looks like the interface
right now allows one to start a transaction and to commit it but it does
not allow one to roll it back. I ended up using apr_dbd_select() with an
SQL string of "ROLLBACK", is this how the interface intends for users to
do it? Should there perhaps be function in apr_dbd for rollback?
Rick Keiner wrote:
> I have been working on that and you're right, the "END TRANSACTION" is not
> needed and makes transactions fail. I removed it and the test case works
> fine. I'll get a patch to the list soon. I've been working on a seperate
> problem with the new release of SQLite that may effect the driver. I would
> like to complete that first and then submit a patch to the list.
>
> */Garrett Rooney <ro...@electricjellyfish.net>/* wrote:
>
> On 9/15/05, Ronen Mizrahi wrote:
> > Yet again, I would like to ask if anyone has any feedback on
> this? If
> > someone out there is pondering some issues about this, please
> respond,
> > perhaps a discussion may help and at the least I would know that
> someone
> > is planning to address this issue.
>
> I suspect nobody has gotten around to looking at it. If you think
> there's a problem, a patch to correct it or at least to add a test
> that shows the issue would certainly be appreciated. As it stands,
> it'll just have to wait until someone who understands the sqlite code
> gets some free time.
>
> -garrett
>
Re: possible bug in apr_dbd_sqlite3.c
Posted by Rick Keiner <ri...@yahoo.com>.
I have been working on that and you're right, the "END TRANSACTION" is not
needed and makes transactions fail. I removed it and the test case works
fine. I'll get a patch to the list soon. I've been working on a seperate
problem with the new release of SQLite that may effect the driver. I would
like to complete that first and then submit a patch to the list.
Garrett Rooney <ro...@electricjellyfish.net> wrote: On 9/15/05, Ronen Mizrahi wrote:
> Yet again, I would like to ask if anyone has any feedback on this? If
> someone out there is pondering some issues about this, please respond,
> perhaps a discussion may help and at the least I would know that someone
> is planning to address this issue.
I suspect nobody has gotten around to looking at it. If you think
there's a problem, a patch to correct it or at least to add a test
that shows the issue would certainly be appreciated. As it stands,
it'll just have to wait until someone who understands the sqlite code
gets some free time.
-garrett
Re: possible bug in apr_dbd_sqlite3.c
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 9/15/05, Ronen Mizrahi <ro...@tversity.com> wrote:
> Yet again, I would like to ask if anyone has any feedback on this? If
> someone out there is pondering some issues about this, please respond,
> perhaps a discussion may help and at the least I would know that someone
> is planning to address this issue.
I suspect nobody has gotten around to looking at it. If you think
there's a problem, a patch to correct it or at least to add a test
that shows the issue would certainly be appreciated. As it stands,
it'll just have to wait until someone who understands the sqlite code
gets some free time.
-garrett
Re: possible bug in apr_dbd_sqlite3.c
Posted by Ronen Mizrahi <ro...@tversity.com>.
Yet again, I would like to ask if anyone has any feedback on this? If
someone out there is pondering some issues about this, please respond,
perhaps a discussion may help and at the least I would know that someone
is planning to address this issue.
Ronen Mizrahi wrote:
> Any feedback of this?
>
> Ronen Mizrahi wrote:
>
>> Hello,
>>
>> In sqlite3 "END TRANSACTION" and "COMMIT" are synonyms, yet the code
>> below, taken from the svn head of apr_dbd_sqlite3.c, invokes them
>> both resulting in sqlite returning an SQL error when doing the commit
>> becuase there is no active transaction. Can someone that knows sqlite
>> better please check it and confirm this? I would like to know if I am
>> correct or I am missing something. As a side comment, in case you are
>> wondering, I just realized that this issue was not detected earlier
>> due to various bugs in dbd_sqlite3_query() (now they are fixed, thank
>> you to whoever fixed them, BTW) that prevented the correct error code
>> from being returned.
>>
>> Thank you,
>>
>> Ronen
>>
>> static int dbd_sqlite3_end_transaction(apr_dbd_transaction_t *trans)
>> {
>> int ret = 0;
>> int nrows = 0;
>>
>> if (trans) {
>> ret = dbd_sqlite3_query(trans->handle, &nrows, "END
>> TRANSACTION;");
>> if (trans->errnum) {
>> trans->errnum = 0;
>> ret = dbd_sqlite3_query(trans->handle, &nrows, "ROLLBACK;");
>> } else {
>> ret = dbd_sqlite3_query(trans->handle, &nrows, "COMMIT;");
>> }
>> trans->handle->trans = NULL;
>> }
>>
>> return ret;
>> }
>>
>
>
Re: possible bug in apr_dbd_sqlite3.c
Posted by Ronen Mizrahi <ro...@tversity.com>.
Any feedback of this?
Ronen Mizrahi wrote:
> Hello,
>
> In sqlite3 "END TRANSACTION" and "COMMIT" are synonyms, yet the code
> below, taken from the svn head of apr_dbd_sqlite3.c, invokes them both
> resulting in sqlite returning an SQL error when doing the commit
> becuase there is no active transaction. Can someone that knows sqlite
> better please check it and confirm this? I would like to know if I am
> correct or I am missing something. As a side comment, in case you are
> wondering, I just realized that this issue was not detected earlier
> due to various bugs in dbd_sqlite3_query() (now they are fixed, thank
> you to whoever fixed them, BTW) that prevented the correct error code
> from being returned.
>
> Thank you,
>
> Ronen
>
> static int dbd_sqlite3_end_transaction(apr_dbd_transaction_t *trans)
> {
> int ret = 0;
> int nrows = 0;
>
> if (trans) {
> ret = dbd_sqlite3_query(trans->handle, &nrows, "END
> TRANSACTION;");
> if (trans->errnum) {
> trans->errnum = 0;
> ret = dbd_sqlite3_query(trans->handle, &nrows, "ROLLBACK;");
> } else {
> ret = dbd_sqlite3_query(trans->handle, &nrows, "COMMIT;");
> }
> trans->handle->trans = NULL;
> }
>
> return ret;
> }
>