resetting sqlite statements and cancellation

[Stefan Sperling <st...@elego.de>]

We have this pattern in several places within wc_db:

  svn_sqlite__get_statement(&stmt, ...)
  svn_sqlite_step(&have_row, stmt);
  while (have_row)
    {
      if (cancel_func)
        SVN_ERR(cancel_func(cancel_baton));

      /* do stuff */  

      svn_sqlite_step(&have_row, stmt);
    }
  svn_sqlite_reset(stmt);

So a cancellation handler can return without resetting the SQL statement.

This is probably not an issue with the CLI client which will just exit.
But what about GUI clients? If the user clicks a cancel button and tries
another operation that reuses the statement, will there be a problem?

If so, what's the best way to solve this?
I think it would be ugly to check the return value of cancel_func manually.
What about a convenience SVN_ERR-like macro that also accepts a statement
and resets it in case an error is thrown?

      if (cancel_func)
        SVN_ERR_RESET_STMT(cancel_func(cancel_baton), stmt);

RE: resetting sqlite statements and cancellation

[Bert Huijben <be...@qqmail.nl>]

> -----Original Message-----
> From: Branko Čibej [mailto:brane@xbc.nu] On Behalf Of Branko Cibej
> Sent: maandag 28 maart 2011 14:54
> To: dev@subversion.apache.org
> Subject: Re: resetting sqlite statements and cancellation
> 
> On 28.03.2011 14:13, Greg Stein wrote:
> > Stepping back... should cancellation even be in the API? When it was first
> > designed, I never imagined any function taking long enough to require a
> > cancel func.
> 
> As an example, I can cite that recursive proplist that I turned upside
> down some time ago. On a large working copy, it takes quite a bit of
> time, so it checks for cancelation between callbacks. Of course it does
> that only while reading from the temporary table of intermediate
> results, so that's per-connection and doesn't affect the wc-db proper.
> 
> >  That would be left to callers, since each function would be
> > short/atomic/quick.
> >
> > And what happens with the transaction here?
> 
> I'd imagine clearing some scratch pool will cause the sqlite handle to
> be closed, which should roll back any pending transactions?

The SQLite handle is stored in the svn_wc__db_t instance, which in turn is stored in the svn_wc_context_t, which is kept in svn_client_ctx_t. So only when the pool of the client context is recycled the database handle is closed. So, "No: we can't wait for sqlite instance cleanup"

> > And is it really expected for this function to run for more than a second?
> 
> I don't know about "this" specifically, but certainly "some" will.

Some of the recent additions from Stephan might include comparing files to their pristines within a db transaction. So given that this can be on network paths we really want cancelation support here.

Or (like I suggested in an earlier mail) we can move the comparision to a callback outside wc_db.c. 
In that case wc_db just has to provide proper error handling for the callback, which it already had to do for normal io errors.

	Bert

Re: resetting sqlite statements and cancellation

[Branko Čibej <br...@e-reka.si>]

On 28.03.2011 14:13, Greg Stein wrote:
> Stepping back... should cancellation even be in the API? When it was first
> designed, I never imagined any function taking long enough to require a
> cancel func.

As an example, I can cite that recursive proplist that I turned upside
down some time ago. On a large working copy, it takes quite a bit of
time, so it checks for cancelation between callbacks. Of course it does
that only while reading from the temporary table of intermediate
results, so that's per-connection and doesn't affect the wc-db proper.

>  That would be left to callers, since each function would be
> short/atomic/quick.
>
> And what happens with the transaction here?

I'd imagine clearing some scratch pool will cause the sqlite handle to
be closed, which should roll back any pending transactions?

> And is it really expected for this function to run for more than a second?

I don't know about "this" specifically, but certainly "some" will.

-- Brane

> On Mar 28, 2011 8:06 AM, "Stefan Sperling" <st...@elego.de> wrote:
>> We have this pattern in several places within wc_db:
>>
>> svn_sqlite__get_statement(&stmt, ...)
>> svn_sqlite_step(&have_row, stmt);
>> while (have_row)
>> {
>> if (cancel_func)
>> SVN_ERR(cancel_func(cancel_baton));
>>
>> /* do stuff */
>>
>> svn_sqlite_step(&have_row, stmt);
>> }
>> svn_sqlite_reset(stmt);
>>
>> So a cancellation handler can return without resetting the SQL statement.
>>
>> This is probably not an issue with the CLI client which will just exit.
>> But what about GUI clients? If the user clicks a cancel button and tries
>> another operation that reuses the statement, will there be a problem?
>>
>> If so, what's the best way to solve this?
>> I think it would be ugly to check the return value of cancel_func
> manually.
>> What about a convenience SVN_ERR-like macro that also accepts a statement
>> and resets it in case an error is thrown?
>>
>> if (cancel_func)
>> SVN_ERR_RESET_STMT(cancel_func(cancel_baton), stmt);

Re: resetting sqlite statements and cancellation

[Greg Stein <gs...@gmail.com>]

Stepping back... should cancellation even be in the API? When it was first
designed, I never imagined any function taking long enough to require a
cancel func. That would be left to callers, since each function would be
short/atomic/quick.

And what happens with the transaction here?

And is it really expected for this function to run for more than a second?
On Mar 28, 2011 8:06 AM, "Stefan Sperling" <st...@elego.de> wrote:
> We have this pattern in several places within wc_db:
>
> svn_sqlite__get_statement(&stmt, ...)
> svn_sqlite_step(&have_row, stmt);
> while (have_row)
> {
> if (cancel_func)
> SVN_ERR(cancel_func(cancel_baton));
>
> /* do stuff */
>
> svn_sqlite_step(&have_row, stmt);
> }
> svn_sqlite_reset(stmt);
>
> So a cancellation handler can return without resetting the SQL statement.
>
> This is probably not an issue with the CLI client which will just exit.
> But what about GUI clients? If the user clicks a cancel button and tries
> another operation that reuses the statement, will there be a problem?
>
> If so, what's the best way to solve this?
> I think it would be ugly to check the return value of cancel_func
manually.
> What about a convenience SVN_ERR-like macro that also accepts a statement
> and resets it in case an error is thrown?
>
> if (cancel_func)
> SVN_ERR_RESET_STMT(cancel_func(cancel_baton), stmt);