Mergeinfo.db file kept open by apache after sql error

During testing of the ra_serf code, and more specifically
when running
merge_tests 63 I encountered following error:

CMD: svn.exe cp
http://localhost/svn-test-work/repositories/merge_te
sts-63/A
http://localhost/svn-test-work/repositories/mer
ge_tests-63/A_copy -m
"create a new copy of A"
ACTUAL STDERR:
......subversionlibsvn_ra_serfutil.c:462:
(apr_err=160043)
svn: SQL logic error or missing database

Extract from apache's error log:
[Thu Aug 23 10:20:36 2007] [error] [client 127.0.0.1] Could
not MERGE
resource
"/svn-test-work/repositories/merge_tests-63/!svn/act/fd
5ccbd0-caa5-f547-ab59-8c882bafcc13"
into "/svn-test-work/repositories/merge_tests-63".
 [409, #0]
[Thu Aug 23 10:20:36 2007] [error] [client 127.0.0.1] An
error occurred
while committing the transaction.  [409, #160043]
[Thu Aug 23 10:20:36 2007] [error] [client 127.0.0.1] SQL
logic error or
missing database  [409, #160043]

When I tried to delete the repository Windows wouldn't allow
me to,
indicating the file was still open by some process. That
turned out to
be the apache process.

This was at 23/08, since then I rebuilt both svn client and
server and I
can't reproduce this issue anymore. So maybe the server has
been fixed
since, but equally likely it isn't.

In
mergeinfo-sqlite-index.c/svn_fs_mergeinfo__get_mergeinfo(),
but also
in svn_fs_mergeinfo__update_index and
svn_fs_mergeinfo__get_mergeinfo_for_tree the call to
sqlite3_close is
not in the 'finally' path (in java terms), so whenever
there's an error
accessing the database it's not closed correctly.

Does this make sense?

Lieven


------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribesubversion.tigris.org
For additional commands, e-mail: dev-helpsubversion.tigris.org

On Aug 26, 2007, at 1:05 AM, Lieven Govaerts wrote:

> During testing of the ra_serf code, and more
specifically when running
> merge_tests 63 I encountered following error:
>
> CMD: svn.exe cp
> http://localhost/svn-test-work/repositories/merge_te
sts-63/A
> http://localhost/svn-test-work/repositories/mer
ge_tests-63/A_copy -m
> "create a new copy of A"
> ACTUAL STDERR:
> ......subversionlibsvn_ra_serfutil.c:462:
(apr_err=160043)
> svn: SQL logic error or missing database
>
> Extract from apache's error log:
> [Thu Aug 23 10:20:36 2007] [error] [client 127.0.0.1]
Could not MERGE
> resource
>
"/svn-test-work/repositories/merge_tests-63/!svn/act/fd
5ccbd0-caa5- 
> f547-ab59-8c882bafcc13"
> into
"/svn-test-work/repositories/merge_tests-63". 
[409, #0]
> [Thu Aug 23 10:20:36 2007] [error] [client 127.0.0.1]
An error  
> occurred
> while committing the transaction.  [409, #160043]
> [Thu Aug 23 10:20:36 2007] [error] [client 127.0.0.1]
SQL logic  
> error or
> missing database  [409, #160043]
>
> When I tried to delete the repository Windows wouldn't
allow me to,
> indicating the file was still open by some process.
That turned out to
> be the apache process.
>
> This was at 23/08, since then I rebuilt both svn client
and server  
> and I
> can't reproduce this issue anymore. So maybe the server
has been fixed
> since, but equally likely it isn't.
>
> In
mergeinfo-sqlite-index.c/svn_fs_mergeinfo__get_mergeinfo(),
but  
> also
> in svn_fs_mergeinfo__update_index and
> svn_fs_mergeinfo__get_mergeinfo_for_tree the call to
sqlite3_close is
> not in the 'finally' path (in java terms), so whenever
there's an  
> error
> accessing the database it's not closed correctly.
>
> Does this make sense?

Thanks for the report, Lieven.  I've added some simple error
handling  
improvements in r26340 which fix this problem.

I was wondering, though -- should we also attach the same
sort of  
connection cleanup code to a pool cleanup function?  Or
would that be  
needless additional complexity?

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribesubversion.tigris.org
For additional commands, e-mail: dev-helpsubversion.tigris.org

Quoting Daniel Rall <dlrfinemaltcoding.com>:

>
> On Aug 26, 2007, at 1:05 AM, Lieven Govaerts wrote:
..
> > In
mergeinfo-sqlite-index.c/svn_fs_mergeinfo__get_mergeinfo(),
but
> > also
> > in svn_fs_mergeinfo__update_index and
> > svn_fs_mergeinfo__get_mergeinfo_for_tree the call
to sqlite3_close is
> > not in the 'finally' path (in java terms), so
whenever there's an
> > error
> > accessing the database it's not closed correctly.
> >
> > Does this make sense?
>
> Thanks for the report, Lieven.  I've added some simple
error handling
> improvements in r26340 which fix this problem.

That should do it. One small remark: What do you think of
renaming the
MAYBE_CLEANUP macro to CLEANUP_ON_ERROR, extending it
to take the 'err' variable as parameter and move to a
common include file so we can use it throughout whole the
code? I've found
hundreds of instances where we use this pattern, might as
well make it more
standard.

> I was wondering, though -- should we also attach the
same sort of
> connection cleanup code to a pool cleanup function?  Or
would that be
> needless additional complexity?

As far as I see in the code we open & close the db
connection in the scope of
one function, there's no handover of ownership involved. I
don't think there's
much value to registering a cleanup function right now, if
we're sure we handle
all return paths correctly.

Lieven


------------------------------------------------------------
----
This message was sent using IMP, the Internet Messaging
Program.

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribesubversion.tigris.org
For additional commands, e-mail: dev-helpsubversion.tigris.org

On Aug 28, 2007, at 4:39 AM, Lieven Govaerts wrote:

> Quoting Daniel Rall <dlrfinemaltcoding.com>:
>
>>
>> On Aug 26, 2007, at 1:05 AM, Lieven Govaerts
wrote:
> ..
>>> In
mergeinfo-sqlite-index.c/svn_fs_mergeinfo__get_mergeinfo(),
but
>>> also
>>> in svn_fs_mergeinfo__update_index and
>>> svn_fs_mergeinfo__get_mergeinfo_for_tree the
call to  
>>> sqlite3_close is
>>> not in the 'finally' path (in java terms), so
whenever there's an
>>> error
>>> accessing the database it's not closed
correctly.
>>>
>>> Does this make sense?
>>
>> Thanks for the report, Lieven.  I've added some
simple error handling
>> improvements in r26340 which fix this problem.
>
> That should do it. One small remark: What do you think
of renaming the
> MAYBE_CLEANUP macro to CLEANUP_ON_ERROR, extending it
> to take the 'err' variable as parameter and move to a
> common include file so we can use it throughout whole
the code?  
> I've found
> hundreds of instances where we use this pattern, might
as well make  
> it more
> standard.

That's not a bad idea -- I cribbed the pattern from
libsvn_wc.  I  
guess this would live in a header file under
include/private/?

>> I was wondering, though -- should we also attach
the same sort of
>> connection cleanup code to a pool cleanup function?
 Or would that be
>> needless additional complexity?
>
> As far as I see in the code we open & close the db
connection in  
> the scope of
> one function, there's no handover of ownership
involved. I don't  
> think there's
> much value to registering a cleanup function right now,
if we're  
> sure we handle
> all return paths correctly.

*nod*  Sounds fair to me.


------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribesubversion.tigris.org
For additional commands, e-mail: dev-helpsubversion.tigris.org

On Aug 29, 2007, at 1:36 AM, Daniel Rall wrote:

>
> On Aug 28, 2007, at 4:39 AM, Lieven Govaerts wrote:
>
>> Quoting Daniel Rall <dlrfinemaltcoding.com>:
>>
>>>
>>> On Aug 26, 2007, at 1:05 AM, Lieven Govaerts
wrote:
>> ..
>>>> In
mergeinfo-sqlite-index.c/svn_fs_mergeinfo__get_mergeinfo(),
but
>>>> also
>>>> in svn_fs_mergeinfo__update_index and
>>>> svn_fs_mergeinfo__get_mergeinfo_for_tree
the call to  
>>>> sqlite3_close is
>>>> not in the 'finally' path (in java terms),
so whenever there's an
>>>> error
>>>> accessing the database it's not closed
correctly.
>>>>
>>>> Does this make sense?
>>>
>>> Thanks for the report, Lieven.  I've added some
simple error  
>>> handling
>>> improvements in r26340 which fix this problem.
>>
>> That should do it. One small remark: What do you
think of renaming  
>> the
>> MAYBE_CLEANUP macro to CLEANUP_ON_ERROR, extending
it
>> to take the 'err' variable as parameter and move to
a
>> common include file so we can use it throughout
whole the code?  
>> I've found
>> hundreds of instances where we use this pattern,
might as well  
>> make it more
>> standard.
>
> That's not a bad idea -- I cribbed the pattern from
libsvn_wc.  I  
> guess this would live in a header file under
include/private/?

Were you thinking of something like the attached patch, or
perhaps  
with a set of macros corresponding to each common label name
(e.g.  
error, cleanup, done)?


------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribesubversion.tigris.org
For additional commands, e-mail: dev-helpsubversion.tigris.org

On Wed, 29 Aug 2007, Daniel Rall wrote:

> 
> On Aug 29, 2007, at 1:36 AM, Daniel Rall wrote:
> 
> >
> >On Aug 28, 2007, at 4:39 AM, Lieven Govaerts
wrote:
> >
> >>Quoting Daniel Rall <dlrfinemaltcoding.com>:
> >>
> >>>
> >>>On Aug 26, 2007, at 1:05 AM, Lieven
Govaerts wrote:
> >>..
> >>>>In
mergeinfo-sqlite-index.c/svn_fs_mergeinfo__get_mergeinfo(),
but
> >>>>also
> >>>>in svn_fs_mergeinfo__update_index and
>
>>>>svn_fs_mergeinfo__get_mergeinfo_for_tree the
call to  
> >>>>sqlite3_close is
> >>>>not in the 'finally' path (in java
terms), so whenever there's an
> >>>>error
> >>>>accessing the database it's not closed
correctly.
> >>>>
> >>>>Does this make sense?
> >>>
> >>>Thanks for the report, Lieven.  I've added
some simple error  
> >>>handling
> >>>improvements in r26340 which fix this
problem.
> >>
> >>That should do it. One small remark: What do
you think of renaming  
> >>the
> >>MAYBE_CLEANUP macro to CLEANUP_ON_ERROR,
extending it
> >>to take the 'err' variable as parameter and
move to a
> >>common include file so we can use it throughout
whole the code?  
> >>I've found
> >>hundreds of instances where we use this
pattern, might as well  
> >>make it more
> >>standard.
> >
> >That's not a bad idea -- I cribbed the pattern from
libsvn_wc.  I  
> >guess this would live in a header file under
include/private/?
> 
> Were you thinking of something like the attached patch,
or perhaps  
> with a set of macros corresponding to each common label
name (e.g.  
> error, cleanup, done)?

I committed this patch as-is in r26483.