On Mon, 2010-10-25, Philip Martin wrote:
> I've been looking at a delete bug: delete can remove a directory NODES
> row and leave rows for children, thus creating an invalid metadata
> state. The code is structured as
>
> svn_wc_delete4(path):
> various checks on path
> for each child of path:
> various checks on child
> svn_wc_delete4(child)
> svn_wc__db_temp_op_delete(path)
> notify
> cleanup
>
> The recursion happens in svn_wc_delete4 while svn_wc__db_temp_op_delete
> is where the database is modifed (non-recursively). I can fix the bug
> by modify the checks in svn_wc_delete4 but I think this is the wrong way
> to do it. I wrote svn_wc__db_temp_op_delete and the _temp_ part is
> simply because I didn't really know how it should work.
>
> Our metadata API should be responsible for maintaining the implied
> database invariants. Thus the svn_wc__db_op_delete needs to do the
> checks to ensure that children don't become orphans. It either needs to
> reject such a change, or it needs to be recursive and remove the
> children.
>
> Making the checks in svn_wc__db_op_delete also solves a race. At
> present the checks made by svn_wc_delete4 are only protected by a
> wclock, putting them in svn_wc__db_op_delete means they can go inside
> the same SQLite transaction that modifies the database.
>
> Once svn_wc__db_op_delete is making the checks there is very little
> point in svn_wc_delete4 making the same checks, and a good performance
> reason for only making the checks once. It's probably more efficient to
> move the recursion into svn_wc__db_op_delete as well and doing so also
> solves the problem that currently op_depth needs to be rewritten as each
> parent gets deleted.
>
> That all seems to be good until we come to notification and workqueues.
> svn_wc__db_op_delete can't really do notification as it would be making
> callbacks from within an SQLite transaction. So svn_wc_delete4 must
> continue to notify, and as it can't reliably identify which paths were
> deleted svn_wc__db_op_delete must return a list of affected paths.
>
> Delete doesn't currently use a workqueue. It could, and perhaps it
> should, although I'm not sure it's strictly necessary. If it were to
> use a workqueue then the function that modifies the metadata,
> svn_wc__db_op_delete, should install any wq items (one per path or one
> per tree).
>
> So the delete code ends up looking like
>
> svn_wc_delete4(path):
> deleted = svn_wc__db_op_delete(path)
> for name in deleted:
> notify(name)
> run workqueue
> cleanup
>
> Does that sound sensible?
>
> Finally, while this mail refers to delete there are similar problems in
> other functions: revert, copy, etc.
+1 to the basic idea that the DB API is responsible for making a whole
logical change, and the outside code shouldn't have to duplicate the
logic.
I've started to use a part of this pattern in the "copy" operation,
except without the notification output. I wrote the entire recursive
metadata copy as a single DB statement, and therefore I did not have
access to a list of changed paths.
The "copy" operation doesn't historically notify every changed path, so
this isn't yet a problem, but when in future we do want to produce
individual notifications, I think the best way to do so may be to crawl
the target tree after creating it. Not sure.
If we were to produce the list within the DB op, then the DB op would
have to recurse outside the SQLite engine, which is fair enough when we
want notifications but wasteful for times when we don't want them.
- Julian
Received on 2010-10-28 18:02:43 CEST