You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "C. Michael Pilato" <cm...@collab.net> on 2011/03/14 15:53:48 UTC
Re: wc_db performance
> On 12.03.2011 13:47, Stefan Sperling wrote:
>> For others who want to jump in and help, here is a list of places
>> where the node walker is still being used. I'm not sure if we can
>> eliminate it everywhere before release, but each of these should
>> be looked at to see whether we can use an alternative approach to
>> increase performance:
>>
>> subversion/libsvn_client/changelist.c
>> subversion/libsvn_client/commit_util.c
>> subversion/libsvn_client/info.c
>> subversion/libsvn_client/merge.c
>> subversion/libsvn_client/mergeinfo.c
>> subversion/libsvn_client/prop_commands.c
>> (This should be propget and propset. Proplist is already using
>> queries involving temporary tables. Rewriting propget on top
>> of the proplist code would be easy. Propset needs more work.)
>> subversion/libsvn_client/ra.c
>> subversion/libsvn_wc/update_editor.c
I want to jump in and help. But (as is common for me, it seems) the first
place I go to jump in stirs up a question. What is the motif we seek to
apply where we need have per-path notification events that need to happen?
For example, I'm familiar with the changelist code, so I started looking at
making this conversion there. But we do per-path notification in that code.
How do we approach this? Do we query old state, make the change, query new
state, diff the states and notify on the diffs? That seems ... wrong. Is
there any pattern established upon which I can model these changes?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: wc_db performance
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Mar 15, 2011 at 12:52:42PM +0000, Philip Martin wrote:
> Yes, by the time we invoke the callbacks we have a read-lock. If we
> simply ran a single transaction and made callbacks within the
> transaction then it would have the same effect. I don't see what the
> temporary table achieves.
The temp tables live in a separate database.
Without them, lock contention is on a single database.
With them, the contention situation is different -- the callback cannot
read from or write to the temp table.
At least that is how I understand this text:
http://sqlite.org/lang_createtable.html: If the "TEMP" or "TEMPORARY" keyword
occurs between the "CREATE" and "TABLE" then the new table is created in the
temp database. It is an error to specify both a <database-name> and the TEMP or
TEMPORARY keyword, unless the <database-name> is "temp". If no database name is
specified and the TEMP keyword is not present then the table is created in the
main database.
So we use the temp table to create a separate database that contains
the data we need to present to the callback, and which is unaffected
by whatever else the callback might be doing.
Does that make sense?
Re: wc_db performance
Posted by Philip Martin <ph...@wandisco.com>.
Stefan Sperling <st...@elego.de> writes:
> On Tue, Mar 15, 2011 at 10:26:15AM +0000, Philip Martin wrote:
>> Branko Čibej <br...@e-reka.si> writes:
>>
>> > The restriction that you may not invoke a callback from within a sqlite
>> > transaction remains. That's what the temporary results tables are for --
>> > they live outside transactions on the main wc-db.
>>
>> I don't understand the temporary table approach. Taking the
>> temp__node_props_cache table as an example: it gets populated by a
>> transaction and lives beyond that transaction. The table then gets
>> queried by a second transaction and the callbacks are made while that
>> second transaction is in progress. So callbacks are still being invoked
>> from within a transaction.
>
> It works around the infinite loop problem that happens when the
> callback inserts rows that end up being picked up by the proplist query.
> Which is silly for this callback to do, but then again we somehow agreed
> that the callbacks are free to do virtually anything. (I still think
> putting newly documented restrictions on them now is fine and would make
> our life easier... *shrug*).
I'm still confused. I thought the infinite loop problem affected the
old multiple transaction code. If we simply ran a single transaction
and made callbacks then that problem would not exist. We would have the
same limitations that apply to the temporary table approach: the
callback cannot write to the database because a transaction is in
progress.
>> As far as I can see using a temporary table has made the overall "read
>> properties" operation into one that modifies the database, to create the
>> temporary table. I guess that by the time the callbacks are made the
>> database write-lock has been dropped, but there would have been no
>> write-lock if the temporary table were not used.
>
> I think the temporary table gets put into a separate database file
> (or memory, depending on the temp_store pragma or whatsitcalled).
> If so, a read lock on the primary db should suffice, shouldn't it?
Yes, by the time we invoke the callbacks we have a read-lock. If we
simply ran a single transaction and made callbacks within the
transaction then it would have the same effect. I don't see what the
temporary table achieves.
--
Philip
Re: wc_db performance
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Mar 15, 2011 at 10:26:15AM +0000, Philip Martin wrote:
> Branko Čibej <br...@e-reka.si> writes:
>
> > The restriction that you may not invoke a callback from within a sqlite
> > transaction remains. That's what the temporary results tables are for --
> > they live outside transactions on the main wc-db.
>
> I don't understand the temporary table approach. Taking the
> temp__node_props_cache table as an example: it gets populated by a
> transaction and lives beyond that transaction. The table then gets
> queried by a second transaction and the callbacks are made while that
> second transaction is in progress. So callbacks are still being invoked
> from within a transaction.
It works around the infinite loop problem that happens when the
callback inserts rows that end up being picked up by the proplist query.
Which is silly for this callback to do, but then again we somehow agreed
that the callbacks are free to do virtually anything. (I still think
putting newly documented restrictions on them now is fine and would make
our life easier... *shrug*).
> As far as I can see using a temporary table has made the overall "read
> properties" operation into one that modifies the database, to create the
> temporary table. I guess that by the time the callbacks are made the
> database write-lock has been dropped, but there would have been no
> write-lock if the temporary table were not used.
I think the temporary table gets put into a separate database file
(or memory, depending on the temp_store pragma or whatsitcalled).
If so, a read lock on the primary db should suffice, shouldn't it?
Re: wc_db performance
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
> Why can't we simply put restrictions on those callbacks? I'm unclear
> on this part.
We seem to be unsure if such a restriction is a good idea.
Suppose a client calls proplist, what can it do when it sees a property
of interest? Can it delete the property? Modify the value? Run diff
on the file? Run proplist on some other path?
--
Philip
Re: wc_db performance
Posted by Greg Stein <gs...@gmail.com>.
On Tue, Mar 15, 2011 at 10:34, Philip Martin <ph...@wandisco.com> wrote:
>...
> So with the temporary table approach the callback really has to use a
> separate database connection to read/write the database from within the
> callback.
>
> However I think that is also the case if we were to avoid the table and
> simply lock the main database; if just one connection was reused it
> might be attempt to reuse an SQLite statement.
And note that we work really hard to use a single SDB connection. We
re-use that as much as we can. I don't even know how we'd start to
bring in more connections. Maybe under the covers, wc_db could open
specialized connections.
But, really?
Why can't we simply put restrictions on those callbacks? I'm unclear
on this part.
Cheers,
-g
Re: wc_db performance
Posted by Philip Martin <ph...@wandisco.com>.
Branko Čibej <br...@e-reka.si> writes:
> On 15.03.2011 15:34, Philip Martin wrote:
>> So with the temporary table approach the callback really has to use a
>> separate database connection to read/write the database from within the
>> callback.
>>
>> However I think that is also the case if we were to avoid the table and
>> simply lock the main database; if just one connection was reused it
>> might be attempt to reuse an SQLite statement.
>
> There's a trick we could use if that's a problem, namely: instead of
> simply creating temporary tables by using the connection's default temp
> database; create another temp *database* per query. It's anonymous, so
> only the code that holds its handle knows about it, and there's no way
> for the callback to access it.
>
> This would require a bit more code, however, the concept is a reusable
> pattern (and the code for the temp database handling would be reusable,
> too.)
Sounds plausible.
Is it valid for an application to call proplist and then in the callback
call proplist again, say on a different part of the working copy? To
support it I think we need to avoid using a fixed name for the temporary
database, perhaps we could use a sequence number in the database
connection structure?
I thought we would have this problem with the name of the temporary
table, but we get a database locked error first.
--
Philip
Re: wc_db performance
Posted by Branko Čibej <br...@e-reka.si>.
On 15.03.2011 15:34, Philip Martin wrote:
> Philip Martin <ph...@wandisco.com> writes:
>
>> Branko Čibej <br...@e-reka.si> writes:
>>
>>> The temporary table is:
>>>
>>> * in a different database, so the read lock on it does not affect
>>> the main wc-db;
>>> * per-connection, so even the same process using a different
>>> database connection will not even see that temporary table.
>> OK, so the process is:
>>
>> - read lock on main database
>> - write lock on temporary database
>> - populate temporary table
>> - release write lock
>> - release read lock
>> - read lock on temporary database
>> - make callbacks
>> - release read lock
>>
>> without the temporary table it could be:
>>
>> - read lock on main database
>> - make callbacks
>> - release lock
>>
>> What we gain is that the callback can modify the main database, because
>> there is no read lock. It still can't modify it using any functions
>> that require write access to the temporary database.
>>
>> What we lose is that the callback cannot call any "read-only" functions
>> that use temporary tables because the step that requires a write lock
>> will fail.
> So with the temporary table approach the callback really has to use a
> separate database connection to read/write the database from within the
> callback.
>
> However I think that is also the case if we were to avoid the table and
> simply lock the main database; if just one connection was reused it
> might be attempt to reuse an SQLite statement.
There's a trick we could use if that's a problem, namely: instead of
simply creating temporary tables by using the connection's default temp
database; create another temp *database* per query. It's anonymous, so
only the code that holds its handle knows about it, and there's no way
for the callback to access it.
This would require a bit more code, however, the concept is a reusable
pattern (and the code for the temp database handling would be reusable,
too.)
-- Brane
Re: wc_db performance
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> Branko Čibej <br...@e-reka.si> writes:
>
>> The temporary table is:
>>
>> * in a different database, so the read lock on it does not affect
>> the main wc-db;
>> * per-connection, so even the same process using a different
>> database connection will not even see that temporary table.
>
> OK, so the process is:
>
> - read lock on main database
> - write lock on temporary database
> - populate temporary table
> - release write lock
> - release read lock
> - read lock on temporary database
> - make callbacks
> - release read lock
>
> without the temporary table it could be:
>
> - read lock on main database
> - make callbacks
> - release lock
>
> What we gain is that the callback can modify the main database, because
> there is no read lock. It still can't modify it using any functions
> that require write access to the temporary database.
>
> What we lose is that the callback cannot call any "read-only" functions
> that use temporary tables because the step that requires a write lock
> will fail.
So with the temporary table approach the callback really has to use a
separate database connection to read/write the database from within the
callback.
However I think that is also the case if we were to avoid the table and
simply lock the main database; if just one connection was reused it
might be attempt to reuse an SQLite statement.
--
Philip
Re: wc_db performance
Posted by Philip Martin <ph...@wandisco.com>.
Branko Čibej <br...@e-reka.si> writes:
> The temporary table is:
>
> * in a different database, so the read lock on it does not affect
> the main wc-db;
> * per-connection, so even the same process using a different
> database connection will not even see that temporary table.
OK, so the process is:
- read lock on main database
- write lock on temporary database
- populate temporary table
- release write lock
- release read lock
- read lock on temporary database
- make callbacks
- release read lock
without the temporary table it could be:
- read lock on main database
- make callbacks
- release lock
What we gain is that the callback can modify the main database, because
there is no read lock. It still can't modify it using any functions
that require write access to the temporary database.
What we lose is that the callback cannot call any "read-only" functions
that use temporary tables because the step that requires a write lock
will fail.
--
Philip
Re: wc_db performance
Posted by Branko Čibej <br...@e-reka.si>.
On 15.03.2011 11:26, Philip Martin wrote:
> Branko Čibej <br...@e-reka.si> writes:
>
>> The restriction that you may not invoke a callback from within a sqlite
>> transaction remains. That's what the temporary results tables are for --
>> they live outside transactions on the main wc-db.
> I don't understand the temporary table approach. Taking the
> temp__node_props_cache table as an example: it gets populated by a
> transaction and lives beyond that transaction. The table then gets
> queried by a second transaction and the callbacks are made while that
> second transaction is in progress. So callbacks are still being invoked
> from within a transaction.
>
> As far as I can see using a temporary table has made the overall "read
> properties" operation into one that modifies the database, to create the
> temporary table. I guess that by the time the callbacks are made the
> database write-lock has been dropped, but there would have been no
> write-lock if the temporary table were not used.
The temporary table is:
* in a different database, so the read lock on it does not affect
the main wc-db;
* per-connection, so even the same process using a different
database connection will not even see that temporary table.
-- Brane
Re: wc_db performance
Posted by Philip Martin <ph...@wandisco.com>.
Branko Čibej <br...@e-reka.si> writes:
> The restriction that you may not invoke a callback from within a sqlite
> transaction remains. That's what the temporary results tables are for --
> they live outside transactions on the main wc-db.
I don't understand the temporary table approach. Taking the
temp__node_props_cache table as an example: it gets populated by a
transaction and lives beyond that transaction. The table then gets
queried by a second transaction and the callbacks are made while that
second transaction is in progress. So callbacks are still being invoked
from within a transaction.
As far as I can see using a temporary table has made the overall "read
properties" operation into one that modifies the database, to create the
temporary table. I guess that by the time the callbacks are made the
database write-lock has been dropped, but there would have been no
write-lock if the temporary table were not used.
--
Philip
Re: wc_db performance
Posted by Branko Čibej <br...@e-reka.si>.
On 14.03.2011 17:13, Stefan Sperling wrote:
> On Mon, Mar 14, 2011 at 05:05:39PM +0100, Stefan Sperling wrote:
>> The concern seems to be that invoking a notification callback could
>> attempt to modify DB state, correct? Which would deadlock if you invoke
>> the callback after starting a transaction to tweak changelist information
>> but before this transaction has been committed (i.e. the write lock has
>> been released)?
> Of course, the same problem happens when the notification callback
> does a *read* operation on the DB (because writers block readers --
> somehow this restriction is kinda hard to getting used to...)
The restriction that you may not invoke a callback from within a sqlite
transaction remains. That's what the temporary results tables are for --
they live outside transactions on the main wc-db.
Still, wherever possible, we shouldn't allow wc-db modifications from
within callbacks, except in a controlled manner through the wc-db API
itself (e.g., modifications could also be queued to a temporary table
whilst holding a read lock on wc-db; block writers, but lets other
readers continue, and writers need exclusive locks to the WC anyway).
-- Brane
Re: wc_db performance
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Mar 14, 2011 at 05:05:39PM +0100, Stefan Sperling wrote:
> The concern seems to be that invoking a notification callback could
> attempt to modify DB state, correct? Which would deadlock if you invoke
> the callback after starting a transaction to tweak changelist information
> but before this transaction has been committed (i.e. the write lock has
> been released)?
Of course, the same problem happens when the notification callback
does a *read* operation on the DB (because writers block readers --
somehow this restriction is kinda hard to getting used to...)
Re: wc_db performance
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Mar 14, 2011 at 10:53:48AM -0400, C. Michael Pilato wrote:
> > On 12.03.2011 13:47, Stefan Sperling wrote:
> >> For others who want to jump in and help, here is a list of places
> >> where the node walker is still being used. I'm not sure if we can
> >> eliminate it everywhere before release, but each of these should
> >> be looked at to see whether we can use an alternative approach to
> >> increase performance:
> >>
> >> subversion/libsvn_client/changelist.c
> >> subversion/libsvn_client/commit_util.c
> >> subversion/libsvn_client/info.c
> >> subversion/libsvn_client/merge.c
> >> subversion/libsvn_client/mergeinfo.c
> >> subversion/libsvn_client/prop_commands.c
> >> (This should be propget and propset. Proplist is already using
> >> queries involving temporary tables. Rewriting propget on top
> >> of the proplist code would be easy. Propset needs more work.)
> >> subversion/libsvn_client/ra.c
> >> subversion/libsvn_wc/update_editor.c
>
> I want to jump in and help. But (as is common for me, it seems) the first
> place I go to jump in stirs up a question. What is the motif we seek to
> apply where we need have per-path notification events that need to happen?
> For example, I'm familiar with the changelist code, so I started looking at
> making this conversion there. But we do per-path notification in that code.
> How do we approach this? Do we query old state, make the change, query new
> state, diff the states and notify on the diffs? That seems ... wrong. Is
> there any pattern established upon which I can model these changes?
The temporary table trick we're using with proplist works well if all
you're doing is reading data from the DB. It sounds like you want to make
modifications, too.
The concern seems to be that invoking a notification callback could
attempt to modify DB state, correct? Which would deadlock if you invoke
the callback after starting a transaction to tweak changelist information
but before this transaction has been committed (i.e. the write lock has
been released)?
I guess you could:
- Read all information you need to determine how the changelist needs
to be changed, using at most a handful of queries.
- Perform sanity checks using this information, and error out if you can
already determine that something won't work out for a specific path.
- Make all the changes in a transaction for all affected paths.
Try to commit the transaction.
- If the commit succeeded, notify for all paths. Else, print some error.
Would that work? We're notifying after the fact in case of success.
However, we can assume that the DB operations finish quickly, so for
the user everything will be nearly instantaneous anyway.