You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Bojan Smojver <bo...@rexursive.com> on 2006/04/28 06:57:49 UTC
[PATCH]: Introduce apr_dbd_transaction_rollback
This was never compiled, let alone tested. It is here as a prototype
for Ronen's suggestion.
--
Bojan
Re: [PATCH]: Introduce apr_dbd_transaction_rollback
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting Bojan Smojver <bo...@rexursive.com>:
> This was never compiled, let alone tested. It is here as a prototype
> for Ronen's suggestion.
For folks using MySQL...
--
Bojan
Re: [PATCH]: Introduce apr_dbd_transaction_rollback
Posted by Bojan Smojver <bo...@rexursive.com>.
On Sat, 2006-04-29 at 10:02 -0400, Chris Darroch wrote:
> #define APR_DBD_TRANS_COMMIT_ON_SUCCESS 0
> #define APR_DBD_TRANS_COMMIT 1
> #define APR_DBD_TRANS_ROLLBACK 2
> #define APR_DBD_TRANS_DEFAULT APR_DBD_TRANS_COMMIT_ON_SUCCESS
OK, I started working on this (skeleton of of the patch is ready), but
before I go any further, could you give me a hint on the semantics of
the above. What I'm not getting is APR_DBD_TRANS_COMMIT v.
APR_DBD_TRANS_COMMIT_ON_SUCCESS. Isn't that the same? I mean, you can't
forcefully commit anything - the database won't take it. So, there is no
point trying the commit if previous errors within the transaction show
that the transaction is going to fail - the only thing that can be done
is rollback. In other words, commit is only commit on success.
So, I think we should just have:
#define APR_DBD_TRANS_COMMIT 0
#define APR_DBD_TRANS_ROLLBACK 1
Or is there something I missed here...
--
Bojan
Re: [PATCH]: Introduce apr_dbd_transaction_rollback
Posted by Nick Kew <ni...@webthing.com>.
On Sunday 30 April 2006 23:05, Chris Darroch wrote:
> > OK, I started working on this (skeleton of of the patch is ready), but
> > before I go any further, could you give me a hint on the semantics of
> > the above. What I'm not getting is APR_DBD_TRANS_COMMIT v.
> > APR_DBD_TRANS_COMMIT_ON_SUCCESS. Isn't that the same? I mean, you can't
> > forcefully commit anything - the database won't take it.
There's rather more than that: after any failed operation within
a transaction, apr_dbd assumes transaction failure and declines
to attempt any further ops, without reference to the backend.
Thus:
> > Or is there something I missed here...
>
> Well, this may not make sense, but what I was thinking was
> that most drivers currently seem to decide whether to rollback
> or commit based on whether some errors have occurred in other
> calls and an internal flag has been set. For example, from pgsql:
>
> if (trans->errnum) {
> trans->errnum = 0;
> res = PQexec(trans->handle->conn, "ROLLBACK");
> }
> else {
> res = PQexec(trans->handle->conn, "COMMIT");
> }
Indeedie.
> What I was thinking was that this is the COMMIT_ON_SUCCESS
> default logic. ROLLBACK obviously forces a rollback. COMMIT
> would attempt a DB commit regardless of whether the driver
> had flagged anything in previously calls. So, like this:
Bearing in mind the above, under what circumstances would
that be meaningful? It's predicated on an assumption that
we have recoverable errors within a transaction. But apr_dbd
doesn't support that (except insofar as a driver may correct
errors within its black box). Or am I missing something?
> switch (trans->mode) {
> case APR_DBD_TRANSACTION_COMMIT:
> res = PQexec(trans->handle->conn, "COMMIT");
> break;
The only usage case I can see where that would make sense is
immediately after an error return, to perform some error-correction
then commit. But any such error-correction would necessarily
take you outside the apr_dbd layer and into direct ops on the
native handle. So having apr_dbd support the commit seems
to have little point, AFAICS.
> case APR_DBD_TRANSACTION_COMMIT_ON_SUCCESS:
> if (trans->errnum) {
> trans->errnum = 0;
> res = PQexec(trans->handle->conn, "ROLLBACK");
> } else
> res = PQexec(trans->handle->conn, "COMMIT");
> break;
> case APR_DBD_TRANSACTION_ROLLBACK:
> res = PQexec(trans->handle->conn, "ROLLBACK");
> break;
> }
That's fine.
> That's just a quick sketch ... if that's dumb, by all means
> do something else! I should be able to focus on this a bit
> next week, as I said, if that helps any. Cheers,
I did express a preference before, but it's not a strong one.
I would also accept Bojan's original patch (simply add
apr_dbd_transaction_rollback to the API), or your scheme
outlined here if you can convince me force-commit has a role.
--
Nick Kew
Re: [PATCH]: Introduce apr_dbd_transaction_rollback
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting Chris Darroch <ch...@pearsoncmg.com>:
> So, like this:
>
> switch (trans->mode) {
> case APR_DBD_TRANSACTION_COMMIT:
> res = PQexec(trans->handle->conn, "COMMIT");
> break;
> case APR_DBD_TRANSACTION_COMMIT_ON_SUCCESS:
> if (trans->errnum) {
> trans->errnum = 0;
> res = PQexec(trans->handle->conn, "ROLLBACK");
> } else
> res = PQexec(trans->handle->conn, "COMMIT");
> break;
> case APR_DBD_TRANSACTION_ROLLBACK:
> res = PQexec(trans->handle->conn, "ROLLBACK");
> break;
> }
Right. That was my understanding of it as well.
Currently the API relies on all query/select call status codes to
determine if the transaction as a whole is going to be success or
failure - it doesn't determine that from the status code of
apr_dbd_transaction_end (its status code is relevant to that function
call only). In fact, once the transaction goes bad, subsequent calls
to query/select don't actually execute anything within the database -
they just return bad status code. In other words, by the time we
reached transaction end, we must have been checking the error codes,
otherwise we're screwed for sure and things will rollback anyway.
So, I would be in favour of keeping that logic - the caller needs to
know *before* calling transaction end if there is a chance of the
whole transaction succeeding or not. And mode can also be chosen
explicitly if everything is OK. Something like:
- begin transaction
- all OK?
- if yes, start loop
- begin loop
- query/select
- all OK?
- if not, exit loop (no need to change mode here!)
- if yes, do nothing
- results as expected?
- if not, set mode rollback, exit loop (explicit rollback!)
- end loop
- end transaction
- all OK?
- if not, notify upstream or maybe retry
Put differently, the need for explicit rollback is really based on
some unexpected result from query/select - not a bad status code. On
bad status code there is nothing that can be done to save the
transaction (bar savepoints, which the current logic probably won't
even withstand, because we can't force transaction status code change
using the API) - rollback is the only option.
That's why I was thinking that we only need two modes: COMMIT and
ROLLBACK. SQL experts, please feel free to tear my argument to shreds
:-)
--
Bojan
Re: [PATCH]: Introduce apr_dbd_transaction_rollback
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting Ronen Mizrahi <ro...@tversity.com>:
> Jut a small comment, if you do go ahead with the flag and not add a
> rollback funciton (which I think is much cleaner) then please be aware
> that the caller will need to know if the result was successful commit,
> successful rollback, or some other failure.
Yes, that would most definitely be the case. That's because the return
codes of apr_dbd_transaction_end function are local in their effects.
They only signal what happened within the function, not what happened
to the transaction.
So, here are the relevant cases:
Mode COMMIT:
- transaction status bad: execute rollback, success on successful rollback
- transaction status good: execute commit, success on successful commit
Mode ROLLBACK:
- execute rollback, success on successful rollback
--
Bojan
Re: [PATCH]: Introduce apr_dbd_transaction_rollback
Posted by Ronen Mizrahi <ro...@tversity.com>.
Jut a small comment, if you do go ahead with the flag and not add a
rollback funciton (which I think is much cleaner) then please be aware
that the caller will need to know if the result was successful commit,
successful rollback, or some other failure.
Chris Darroch wrote:
>Bojan:
>
>
>
>>Nice. Do you want me to do a prototype patch or do you have something
>>already?
>>
>>
>
> Thanks! Alas, I have nada in the way of an implementation; I'm
>still trying to finish up my scoreboard/slow child_init/etc. fixes
>for httpd. I've also been away from the keyboard much of the previous
>week; next week should be different, though, so I may have some
>time to share/spare.
>
>
>
>>>>#define APR_DBD_TRANS_COMMIT_ON_SUCCESS 0
>>>>#define APR_DBD_TRANS_COMMIT 1
>>>>#define APR_DBD_TRANS_ROLLBACK 2
>>>>#define APR_DBD_TRANS_DEFAULT APR_DBD_TRANS_COMMIT_ON_SUCCESS
>>>>
>>>>
>
> It occurs to me now that these should perhaps be yet more
>verbose, e.g., APR_DBD_TRANSACTION_COMMIT, etc. to match
>apr_dbd_transaction_start/end().
>
>
>
>>OK, I started working on this (skeleton of of the patch is ready), but
>>before I go any further, could you give me a hint on the semantics of
>>the above. What I'm not getting is APR_DBD_TRANS_COMMIT v.
>>APR_DBD_TRANS_COMMIT_ON_SUCCESS. Isn't that the same? I mean, you can't
>>forcefully commit anything - the database won't take it.
>>
>>
>
>
>
>>Or is there something I missed here...
>>
>>
>
> Well, this may not make sense, but what I was thinking was
>that most drivers currently seem to decide whether to rollback
>or commit based on whether some errors have occurred in other
>calls and an internal flag has been set. For example, from pgsql:
>
> if (trans->errnum) {
> trans->errnum = 0;
> res = PQexec(trans->handle->conn, "ROLLBACK");
> }
> else {
> res = PQexec(trans->handle->conn, "COMMIT");
> }
>
> What I was thinking was that this is the COMMIT_ON_SUCCESS
>default logic. ROLLBACK obviously forces a rollback. COMMIT
>would attempt a DB commit regardless of whether the driver
>had flagged anything in previously calls. So, like this:
>
> switch (trans->mode) {
> case APR_DBD_TRANSACTION_COMMIT:
> res = PQexec(trans->handle->conn, "COMMIT");
> break;
> case APR_DBD_TRANSACTION_COMMIT_ON_SUCCESS:
> if (trans->errnum) {
> trans->errnum = 0;
> res = PQexec(trans->handle->conn, "ROLLBACK");
> } else
> res = PQexec(trans->handle->conn, "COMMIT");
> break;
> case APR_DBD_TRANSACTION_ROLLBACK:
> res = PQexec(trans->handle->conn, "ROLLBACK");
> break;
> }
>
> That's just a quick sketch ... if that's dumb, by all means
>do something else! I should be able to focus on this a bit
>next week, as I said, if that helps any. Cheers,
>
>Chris.
>
>
>
Re: [PATCH]: Introduce apr_dbd_transaction_rollback
Posted by Chris Darroch <ch...@pearsoncmg.com>.
Bojan:
> Nice. Do you want me to do a prototype patch or do you have something
> already?
Thanks! Alas, I have nada in the way of an implementation; I'm
still trying to finish up my scoreboard/slow child_init/etc. fixes
for httpd. I've also been away from the keyboard much of the previous
week; next week should be different, though, so I may have some
time to share/spare.
>>> #define APR_DBD_TRANS_COMMIT_ON_SUCCESS 0
>>> #define APR_DBD_TRANS_COMMIT 1
>>> #define APR_DBD_TRANS_ROLLBACK 2
>>> #define APR_DBD_TRANS_DEFAULT APR_DBD_TRANS_COMMIT_ON_SUCCESS
It occurs to me now that these should perhaps be yet more
verbose, e.g., APR_DBD_TRANSACTION_COMMIT, etc. to match
apr_dbd_transaction_start/end().
> OK, I started working on this (skeleton of of the patch is ready), but
> before I go any further, could you give me a hint on the semantics of
> the above. What I'm not getting is APR_DBD_TRANS_COMMIT v.
> APR_DBD_TRANS_COMMIT_ON_SUCCESS. Isn't that the same? I mean, you can't
> forcefully commit anything - the database won't take it.
> Or is there something I missed here...
Well, this may not make sense, but what I was thinking was
that most drivers currently seem to decide whether to rollback
or commit based on whether some errors have occurred in other
calls and an internal flag has been set. For example, from pgsql:
if (trans->errnum) {
trans->errnum = 0;
res = PQexec(trans->handle->conn, "ROLLBACK");
}
else {
res = PQexec(trans->handle->conn, "COMMIT");
}
What I was thinking was that this is the COMMIT_ON_SUCCESS
default logic. ROLLBACK obviously forces a rollback. COMMIT
would attempt a DB commit regardless of whether the driver
had flagged anything in previously calls. So, like this:
switch (trans->mode) {
case APR_DBD_TRANSACTION_COMMIT:
res = PQexec(trans->handle->conn, "COMMIT");
break;
case APR_DBD_TRANSACTION_COMMIT_ON_SUCCESS:
if (trans->errnum) {
trans->errnum = 0;
res = PQexec(trans->handle->conn, "ROLLBACK");
} else
res = PQexec(trans->handle->conn, "COMMIT");
break;
case APR_DBD_TRANSACTION_ROLLBACK:
res = PQexec(trans->handle->conn, "ROLLBACK");
break;
}
That's just a quick sketch ... if that's dumb, by all means
do something else! I should be able to focus on this a bit
next week, as I said, if that helps any. Cheers,
Chris.
--
GPG Key ID: 366A375B
GPG Key Fingerprint: 485E 5041 17E1 E2BB C263 E4DE C8E3 FA36 366A 375B
Re: [PATCH]: Introduce apr_dbd_transaction_rollback
Posted by Bojan Smojver <bo...@rexursive.com>.
On Sat, 2006-04-29 at 10:02 -0400, Chris Darroch wrote:
> Otherwise, a nested function that wants to force a rollback
> has to either do so, and then signal to all the callers that
> the transaction is dead (and they then have to handle that
> condition), or it has to set some internal application flag
> that must be checked before trying to end (i.e., commit) or
> rollback the transaction. With the mode flag in APR, I think
> some of this application-side logic might be eliminated. Thoughts?
Nice. Do you want me to do a prototype patch or do you have something
already?
--
Bojan
Re: [PATCH]: Introduce apr_dbd_transaction_rollback
Posted by Chris Darroch <ch...@pearsoncmg.com>.
Bojan:
> By far the most important piece of software that uses APR 1.2.x is
> Apache 2.2 and associated modules. An upgrade of APR to 2.x there would
> most likely require an MMN bump. I'm not sure if Apache development
> policies permit this any more mid minor version. However, using APR
> 1.3.x, given it is binary compatible with 1.2.x, would probably be an
> option. So, the upgrade here is all about timing, AFAIK.
Good point. Well, here's a thought I'll just throw out there,
that might make things a little more like the way the Perl DBI
AutoCommit thingy works, and which might satisfy both binary
compability and Nick's preference for automatic commit/rollback
as the default in apr_dbd_transaction_end().
Instead of adding an apr_dbd_transaction_rollback() function,
what about adding the following:
#define APR_DBD_TRANS_COMMIT_ON_SUCCESS 0
#define APR_DBD_TRANS_COMMIT 1
#define APR_DBD_TRANS_ROLLBACK 2
#define APR_DBD_TRANS_DEFAULT APR_DBD_TRANS_COMMIT_ON_SUCCESS
APU_DECLARE(int) apr_dbd_transaction_mode_get(apr_dbd_transaction_t *trans);
APU_DECLARE(int) apr_dbd_transaction_mode_set(apr_dbd_transaction_t *trans,
int mode);
Then instead of the following sort of logic using
apr_dbd_transaction_rollback():
if (application_error) {
apr_dbd_transaction_rollback(...);
} else if (apr_dbd_transaction_end(...)) {
## handle unexpected error
}
you'd use instead something like this:
## this statement could be in a nested sub-function
if (application_error)
apr_dbd_transaction_mode_set(trans, APR_DBD_TRANS_ROLLBACK);
if (apr_dbd_transaction_end(...)) {
## handle unexpected error
}
Among other things, I'm thinking that this has some advantages
because it means that if an application encounters an error
deep in a nested set of functions, it can simply set the "mode"
flag on the transaction it's been passed and return up through
the stack. The high-level caller that created the transaction
can then simply end it, and if it wants to beforehand, check
the mode state.
Otherwise, a nested function that wants to force a rollback
has to either do so, and then signal to all the callers that
the transaction is dead (and they then have to handle that
condition), or it has to set some internal application flag
that must be checked before trying to end (i.e., commit) or
rollback the transaction. With the mode flag in APR, I think
some of this application-side logic might be eliminated. Thoughts?
Chris.
--
GPG Key ID: 366A375B
GPG Key Fingerprint: 485E 5041 17E1 E2BB C263 E4DE C8E3 FA36 366A 375B
Re: [PATCH]: Introduce apr_dbd_transaction_rollback
Posted by Bojan Smojver <bo...@rexursive.com>.
On Fri, 2006-04-28 at 09:41 -0400, Chris Darroch wrote:
> Now I'm partly assuming that there can't be zillions of users
> yet relying on it in a 1.3 environment who really also need a
> forceable rollback feature, and that those who do can upgrade to
> 2.x. (After all, isn't it good to provide these little inducements
> to encourage folks to upgrade? :-)
By far the most important piece of software that uses APR 1.2.x is
Apache 2.2 and associated modules. An upgrade of APR to 2.x there would
most likely require an MMN bump. I'm not sure if Apache development
policies permit this any more mid minor version. However, using APR
1.3.x, given it is binary compatible with 1.2.x, would probably be an
option. So, the upgrade here is all about timing, AFAIK.
It would be good if Apache developers could enlighten us a bit more
here...
--
Bojan
Re: [PATCH]: Introduce apr_dbd_transaction_rollback
Posted by Chris Darroch <ch...@pearsoncmg.com>.
Bojan:
> On Fri, 2006-04-28 at 10:31 +0100, Nick Kew wrote:
>
>> What's missing is the option to rollback even when successful.
>> In principle, a "rollback" argument to transaction_end would be
>> a better way to accomplish this. What level of version bump would
>> we need to introduce that?
>
> Given that this would break binary compatibility, it would have to be
> done in 2.x. The "new function approach" could be done for 1.3.
This is great either way -- it's been on my to-do list but
I've had to keep punting on it. Delighted to see something disappear
off the list!
As a not-committer, I don't really have a say here, but either
option seems fine to me in terms of functionality (obviously).
My gut instinct would be that I prefer the "rollback" argument
Nick suggests purely for elegance, and that since apr_dbd is
relatively new, breaking binary compatibility (so long as it's done
with the appropriate version bumps) in the name of a clean interface
isn't too horrible.
Now I'm partly assuming that there can't be zillions of users
yet relying on it in a 1.3 environment who really also need a
forceable rollback feature, and that those who do can upgrade to
2.x. (After all, isn't it good to provide these little inducements
to encourage folks to upgrade? :-)
Whatever is chosen, kudos for dealing with this. Thanks!
Chris.
--
GPG Key ID: 366A375B
GPG Key Fingerprint: 485E 5041 17E1 E2BB C263 E4DE C8E3 FA36 366A 375B
Re: [PATCH]: Introduce apr_dbd_transaction_rollback
Posted by Bojan Smojver <bo...@rexursive.com>.
On Fri, 2006-04-28 at 10:31 +0100, Nick Kew wrote:
> What's missing is the option to rollback even when successful.
> In principle, a "rollback" argument to transaction_end would be
> a better way to accomplish this. What level of version bump would
> we need to introduce that?
Given that this would break binary compatibility, it would have to be
done in 2.x. The "new function approach" could be done for 1.3.
--
Bojan
Re: [PATCH]: Introduce apr_dbd_transaction_rollback
Posted by Ronen Mizrahi <ro...@tversity.com>.
To add to that, the logic of ANY commit operation not just APR's
end_transaction() is to commit a successful transaciton and rollback if
an error occurs. This is what commit does by definition and it has
nothing to do with an applicative decision to rollback the transaction
which happens in case of an applicative error and not database error.
Ronen Mizrahi wrote:
> In my opinion two separate functions, one for commit and one for
> rollback will be better and produce more readable code for the
> application. I am not aware of a single DB API out there that combines
> these two operations into one function. This does not mean it does not
> exist of-course, however it does suggest that using one function is
> somewhat unusual and probably undesirable.
>
> Nick Kew wrote:
>
>> On Friday 28 April 2006 05:57, Bojan Smojver wrote:
>>
>>
>>> This was never compiled, let alone tested. It is here as a prototype
>>> for Ronen's suggestion.
>>>
>>
>>
>> Hmmm. Not too keen on this.
>>
>> The original logic of APR end transaction is that it'll commit a
>> successful transaction otr rollback one where an error occurred.
>>
>> What's missing is the option to rollback even when successful.
>> In principle, a "rollback" argument to transaction_end would be
>> a better way to accomplish this. What level of version bump would
>> we need to introduce that?
>>
>>
>>
>
>
Re: [PATCH]: Introduce apr_dbd_transaction_rollback
Posted by Ronen Mizrahi <ro...@tversity.com>.
In my opinion two separate functions, one for commit and one for
rollback will be better and produce more readable code for the
application. I am not aware of a single DB API out there that combines
these two operations into one function. This does not mean it does not
exist of-course, however it does suggest that using one function is
somewhat unusual and probably undesirable.
Nick Kew wrote:
>On Friday 28 April 2006 05:57, Bojan Smojver wrote:
>
>
>>This was never compiled, let alone tested. It is here as a prototype
>>for Ronen's suggestion.
>>
>>
>
>Hmmm. Not too keen on this.
>
>The original logic of APR end transaction is that it'll commit a
>successful transaction otr rollback one where an error occurred.
>
>What's missing is the option to rollback even when successful.
>In principle, a "rollback" argument to transaction_end would be
>a better way to accomplish this. What level of version bump would
>we need to introduce that?
>
>
>
Re: [PATCH]: Introduce apr_dbd_transaction_rollback
Posted by Nick Kew <ni...@webthing.com>.
On Friday 28 April 2006 05:57, Bojan Smojver wrote:
> This was never compiled, let alone tested. It is here as a prototype
> for Ronen's suggestion.
Hmmm. Not too keen on this.
The original logic of APR end transaction is that it'll commit a
successful transaction otr rollback one where an error occurred.
What's missing is the option to rollback even when successful.
In principle, a "rollback" argument to transaction_end would be
a better way to accomplish this. What level of version bump would
we need to introduce that?
--
Nick Kew
Re: [PATCH]: Introduce apr_dbd_transaction_rollback
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting Bojan Smojver <bo...@rexursive.com>:
> This was never compiled, let alone tested. It is here as a prototype
> for Ronen's suggestion.
OOPS! Before rollback in SQLite2/3, we need to make sure we blow away
error codes, or the query won't run. Hopefully better patch attached.
--
Bojan