You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by rh...@apache.org on 2011/04/15 13:03:48 UTC
svn commit: r1092660 - in /subversion/trunk/subversion/libsvn_wc: adm_ops.c
crop.c wc_db.c wc_db.h
Author: rhuijben
Date: Fri Apr 15 11:03:48 2011
New Revision: 1092660
URL: http://svn.apache.org/viewvc?rev=1092660&view=rev
Log:
Instead of installing a wq operation to remove records from the database
in the commit processing, just perform the operation directly.
Working queue operations are for synchronizing the working copy with the
database. In this case the working copy doesn't change at all.
To make this work, change an existing wc_db temp operation recursive.
(It assumed that it was a leave operation before or it would break the DB)
* subversion/libsvn_wc/adm_ops.c
(process_committed_leaf): Determine if we should really install a not present
node and then call svn_wc__db_op_remove_node() instead of installing a
wq operation that does the same thing.
(svn_wc__internal_remove_from_revision_control): Don't perform expensive
compares if we are not going to use the result. Update caller.
* subversion/libsvn_wc/crop.c
(crop_children): Update caller.
* subversion/libsvn_wc/wc_db.c
(remove_node_baton): New struct.
(remove_node_txn): New function.
(svn_wc__db_temp_op_remove_entry): Rename to ...
(svn_wc__db_op_remove_node): ... this to reflect that the function is really
needed and now recursive.
* subversion/libsvn_wc/wc_db.h
(svn_wc__db_temp_op_remove_entry): Rename to ...
(svn_wc__db_op_remove_node): ... this to reflect that the function is really
needed and now recursive.
Modified:
subversion/trunk/subversion/libsvn_wc/adm_ops.c
subversion/trunk/subversion/libsvn_wc/crop.c
subversion/trunk/subversion/libsvn_wc/wc_db.c
subversion/trunk/subversion/libsvn_wc/wc_db.h
Modified: subversion/trunk/subversion/libsvn_wc/adm_ops.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/adm_ops.c?rev=1092660&r1=1092659&r2=1092660&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/adm_ops.c (original)
+++ subversion/trunk/subversion/libsvn_wc/adm_ops.c Fri Apr 15 11:03:48 2011
@@ -127,8 +127,8 @@ process_committed_leaf(svn_wc__db_t *db,
svn_wc__db_status_t status;
svn_wc__db_kind_t kind;
const svn_checksum_t *copied_checksum;
- const char *adm_abspath;
svn_revnum_t new_changed_rev = new_revnum;
+ svn_boolean_t have_base;
SVN_ERR_ASSERT(svn_dirent_is_absolute(local_abspath));
@@ -137,21 +137,28 @@ process_committed_leaf(svn_wc__db_t *db,
NULL, NULL, NULL,
NULL, NULL, &copied_checksum,
NULL, NULL, NULL,
- NULL, NULL, NULL, NULL,
NULL, NULL, NULL, NULL, NULL,
+ &have_base, NULL, NULL, NULL,
db, local_abspath,
scratch_pool, scratch_pool));
- if (kind == svn_wc__db_kind_dir)
- adm_abspath = local_abspath;
- else
- adm_abspath = svn_dirent_dirname(local_abspath, scratch_pool);
- SVN_ERR(svn_wc__write_check(db, adm_abspath, scratch_pool));
+ {
+ const char *adm_abspath;
+
+ if (kind == svn_wc__db_kind_dir)
+ adm_abspath = local_abspath;
+ else
+ adm_abspath = svn_dirent_dirname(local_abspath, scratch_pool);
+ SVN_ERR(svn_wc__write_check(db, adm_abspath, scratch_pool));
+ }
if (status == svn_wc__db_status_deleted)
{
- return svn_error_return(svn_wc__wq_add_deletion_postcommit(
- db, local_abspath, new_revnum, no_unlock,
+ return svn_error_return(
+ svn_wc__db_op_remove_node(
+ db, local_abspath,
+ have_base ? new_revnum : SVN_INVALID_REVNUM,
+ kind,
scratch_pool));
}
@@ -1789,28 +1796,35 @@ svn_wc__internal_remove_from_revision_co
if (kind == svn_wc__db_kind_file || kind == svn_wc__db_kind_symlink)
{
svn_node_kind_t on_disk;
- svn_boolean_t wc_special, local_special;
svn_boolean_t text_modified_p;
const svn_checksum_t *base_sha1_checksum, *working_sha1_checksum;
- /* Only check if the file was modified when it wasn't overwritten with a
- special file */
- SVN_ERR(svn_wc__get_translate_info(NULL, NULL, NULL,
- &wc_special, db, local_abspath, NULL,
- scratch_pool, scratch_pool));
- SVN_ERR(svn_io_check_special_path(local_abspath, &on_disk,
- &local_special, scratch_pool));
- if (wc_special || ! local_special)
+ if (instant_error || destroy_wf)
{
- /* Check for local mods. before removing entry */
- SVN_ERR(svn_wc__internal_file_modified_p(&text_modified_p, NULL,
- NULL, db,
- local_abspath, FALSE,
- TRUE, scratch_pool));
- if (text_modified_p && instant_error)
- return svn_error_createf(SVN_ERR_WC_LEFT_LOCAL_MOD, NULL,
- _("File '%s' has local modifications"),
- svn_dirent_local_style(local_abspath, scratch_pool));
+ svn_boolean_t wc_special, local_special;
+ /* Only check if the file was modified when it wasn't overwritten
+ with a special file */
+
+ SVN_ERR(svn_wc__get_translate_info(NULL, NULL, NULL,
+ &wc_special, db, local_abspath, NULL,
+ scratch_pool, scratch_pool));
+ SVN_ERR(svn_io_check_special_path(local_abspath, &on_disk,
+ &local_special, scratch_pool));
+ if (wc_special || ! local_special)
+ {
+ /* Check for local mods. before removing entry */
+ SVN_ERR(svn_wc__internal_file_modified_p(&text_modified_p, NULL,
+ NULL, db,
+ local_abspath, FALSE,
+ TRUE, scratch_pool));
+ if (text_modified_p && instant_error)
+ return svn_error_createf(SVN_ERR_WC_LEFT_LOCAL_MOD, NULL,
+ _("File '%s' has local modifications"),
+ svn_dirent_local_style(local_abspath, scratch_pool));
+ }
+
+ if (! wc_special && local_special)
+ text_modified_p = TRUE;
}
/* Find the checksum(s) of the node's one or two pristine texts. Note
@@ -1844,8 +1858,10 @@ svn_wc__internal_remove_from_revision_co
SVN_ERR(err);
/* Remove NAME from PATH's entries file: */
- SVN_ERR(svn_wc__db_temp_op_remove_entry(db, local_abspath,
- scratch_pool));
+ SVN_ERR(svn_wc__db_op_remove_node(db, local_abspath,
+ SVN_INVALID_REVNUM,
+ svn_wc__db_kind_unknown,
+ scratch_pool));
/* Having removed the checksums that reference the pristine texts,
remove the pristine texts (if now totally unreferenced) from the
@@ -1869,7 +1885,7 @@ svn_wc__internal_remove_from_revision_co
if (destroy_wf)
{
/* Don't kill local mods. */
- if ((! wc_special && local_special) || text_modified_p)
+ if (text_modified_p)
return svn_error_create(SVN_ERR_WC_LEFT_LOCAL_MOD, NULL, NULL);
else /* The working file is still present; remove it. */
SVN_ERR(svn_io_remove_file2(local_abspath, TRUE, scratch_pool));
@@ -1907,8 +1923,10 @@ svn_wc__internal_remove_from_revision_co
iterpool));
if (hidden)
{
- SVN_ERR(svn_wc__db_temp_op_remove_entry(db, entry_abspath,
- iterpool));
+ SVN_ERR(svn_wc__db_op_remove_node(db, entry_abspath,
+ SVN_INVALID_REVNUM,
+ svn_wc__db_kind_unknown,
+ iterpool));
continue;
}
@@ -1948,8 +1966,10 @@ svn_wc__internal_remove_from_revision_co
full_path. We need to remove that entry: */
if (! is_root)
{
- SVN_ERR(svn_wc__db_temp_op_remove_entry(db, local_abspath,
- iterpool));
+ SVN_ERR(svn_wc__db_op_remove_node(db, local_abspath,
+ SVN_INVALID_REVNUM,
+ svn_wc__db_kind_unknown,
+ iterpool));
}
}
Modified: subversion/trunk/subversion/libsvn_wc/crop.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/crop.c?rev=1092660&r1=1092659&r2=1092660&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/crop.c (original)
+++ subversion/trunk/subversion/libsvn_wc/crop.c Fri Apr 15 11:03:48 2011
@@ -125,8 +125,10 @@ crop_children(svn_wc__db_t *db,
? svn_depth_immediates
: svn_depth_files;
if (depth < remove_below)
- SVN_ERR(svn_wc__db_temp_op_remove_entry(db, local_abspath,
- iterpool));
+ SVN_ERR(svn_wc__db_op_remove_node(db, local_abspath, FALSE,
+ SVN_INVALID_REVNUM,
+ svn_wc__db_kind_unknown,
+ iterpool));
continue;
}
Modified: subversion/trunk/subversion/libsvn_wc/wc_db.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_db.c?rev=1092660&r1=1092659&r2=1092660&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/wc_db.c (original)
+++ subversion/trunk/subversion/libsvn_wc/wc_db.c Fri Apr 15 11:03:48 2011
@@ -4359,14 +4359,82 @@ svn_wc__db_op_read_tree_conflict(
return SVN_NO_ERROR;
}
+/* Baton for remove_post_commit_txn */
+struct remove_node_baton
+{
+ svn_revnum_t not_present_rev;
+ svn_wc__db_kind_t not_present_kind;
+};
+
+/* Implements svn_wc__db_txn_callback_t for svn_wc__db_op_remove_node */
+static svn_error_t *
+remove_node_txn(void *baton,
+ svn_wc__db_wcroot_t *wcroot,
+ const char *local_relpath,
+ apr_pool_t *scratch_pool)
+{
+ struct remove_node_baton *rnb = baton;
+ svn_sqlite__stmt_t *stmt;
+
+ apr_int64_t repos_id;
+ const char *repos_relpath;
+
+ const char *like_arg = construct_like_arg(local_relpath, scratch_pool);
+
+ SVN_ERR_ASSERT(*local_relpath != '\0'); /* Never on a wcroot */
+
+ /* Need info for not_present node? */
+ if (SVN_IS_VALID_REVNUM(rnb->not_present_rev))
+ SVN_ERR(scan_upwards_for_repos(&repos_id, &repos_relpath, wcroot,
+ local_relpath, scratch_pool, scratch_pool));
+
+ SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
+ STMT_DELETE_NODES_RECURSIVE));
+
+ /* Remove all nodes at or below local_relpath where op_depth >= 0 */
+ SVN_ERR(svn_sqlite__bindf(stmt, "issi", wcroot->wc_id,
+ local_relpath,
+ like_arg,
+ (apr_int64_t)0));
+ SVN_ERR(svn_sqlite__step_done(stmt));
+
+ SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
+ STMT_DELETE_ACTUAL_NODE_RECURSIVE));
+
+ /* Delete all actual nodes at or below local_relpath */
+ SVN_ERR(svn_sqlite__bindf(stmt, "iss", wcroot->wc_id,
+ local_relpath,
+ like_arg));
+ SVN_ERR(svn_sqlite__step_done(stmt));
+
+ /* Should we leave a not-present node? */
+ if (SVN_IS_VALID_REVNUM(rnb->not_present_rev))
+ {
+ insert_base_baton_t ibb;
+ blank_ibb(&ibb);
+
+ ibb.repos_id = repos_id;
+ ibb.status = svn_wc__db_status_not_present;
+ ibb.kind = rnb->not_present_kind;
+
+ ibb.repos_relpath = repos_relpath;
+ ibb.revision = rnb->not_present_rev;
+
+ SVN_ERR(insert_base_node(&ibb, wcroot, local_relpath, scratch_pool));
+ }
+
+ return SVN_NO_ERROR;
+}
svn_error_t *
-svn_wc__db_temp_op_remove_entry(svn_wc__db_t *db,
- const char *local_abspath,
- apr_pool_t *scratch_pool)
+svn_wc__db_op_remove_node(svn_wc__db_t *db,
+ const char *local_abspath,
+ svn_revnum_t not_present_revision,
+ svn_wc__db_kind_t not_present_kind,
+ apr_pool_t *scratch_pool)
{
svn_wc__db_wcroot_t *wcroot;
- svn_sqlite__stmt_t *stmt;
+ struct remove_node_baton rnb;
const char *local_relpath;
SVN_ERR_ASSERT(svn_dirent_is_absolute(local_abspath));
@@ -4375,17 +4443,15 @@ svn_wc__db_temp_op_remove_entry(svn_wc__
local_abspath, scratch_pool, scratch_pool));
VERIFY_USABLE_WCROOT(wcroot);
- SVN_ERR(flush_entries(wcroot, local_abspath, scratch_pool));
-
- SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb, STMT_DELETE_NODES));
- SVN_ERR(svn_sqlite__bindf(stmt, "is", wcroot->wc_id, local_relpath));
- SVN_ERR(svn_sqlite__step_done(stmt));
+ rnb.not_present_rev = not_present_revision;
+ rnb.not_present_kind = not_present_kind;
- SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
- STMT_DELETE_ACTUAL_NODE_WITHOUT_CONFLICT));
- SVN_ERR(svn_sqlite__bindf(stmt, "is", wcroot->wc_id, local_relpath));
+ SVN_ERR(svn_wc__db_with_txn(wcroot, local_relpath, remove_node_txn,
+ &rnb, scratch_pool));
- SVN_ERR(svn_sqlite__step_done(stmt));
+ /* ### Flush everything below this node in all ways */
+ SVN_ERR(flush_entries(wcroot, local_abspath, scratch_pool));
+ SVN_ERR(svn_wc__db_temp_forget_directory(db, local_abspath, scratch_pool));
return SVN_NO_ERROR;
}
Modified: subversion/trunk/subversion/libsvn_wc/wc_db.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_db.h?rev=1092660&r1=1092659&r2=1092660&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/wc_db.h (original)
+++ subversion/trunk/subversion/libsvn_wc/wc_db.h Fri Apr 15 11:03:48 2011
@@ -2424,12 +2424,21 @@ svn_wc__db_temp_forget_directory(svn_wc_
const char *local_dir_abspath,
apr_pool_t *scratch_pool);
-/* Removes all references of LOCAL_ABSPATH from its working copy
- using DB. */
+/* Removes all references to LOCAL_ABSPATH from DB, while optionally leaving
+ tree conflicts and/or a not present node.
+
+ This operation always recursively removes all nodes at and below
+ LOCAL_ABSPATH from NODES and ACTUAL.
+
+ If NOT_PRESENT_REVISION specifies a valid revision, leave a not_present
+ BASE node at local_abspath. (Requires an existing BASE node before removing)
+ */
svn_error_t *
-svn_wc__db_temp_op_remove_entry(svn_wc__db_t *db,
- const char *local_abspath,
- apr_pool_t *scratch_pool);
+svn_wc__db_op_remove_node(svn_wc__db_t *db,
+ const char *local_abspath,
+ svn_revnum_t not_present_revision,
+ svn_wc__db_kind_t not_present_kind,
+ apr_pool_t *scratch_pool);
/* Remove the WORKING_NODE row of LOCAL_ABSPATH in DB. */
svn_error_t *
RE: svn commit: r1092660 - in /subversion/trunk/subversion/libsvn_wc: adm_ops.c crop.c wc_db.c wc_db.h
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Greg Stein [mailto:gstein@gmail.com]
> Sent: zaterdag 16 april 2011 0:12
> To: Bert Huijben
> Cc: dev@subversion.apache.org
> Subject: Re: svn commit: r1092660 - in
> /subversion/trunk/subversion/libsvn_wc: adm_ops.c crop.c wc_db.c
> wc_db.h
>
> On Fri, Apr 15, 2011 at 16:20, Bert Huijben <be...@qqmail.nl> wrote:
> >> -----Original Message-----
> >> From: Greg Stein [mailto:gstein@gmail.com]
> >> Sent: vrijdag 15 april 2011 22:00
> >> To: Bert Huijben
> >> Cc: dev@subversion.apache.org
> >> Subject: Re: svn commit: r1092660 - in
> >> /subversion/trunk/subversion/libsvn_wc: adm_ops.c crop.c wc_db.c
> >> wc_db.h
> >
> >> But I still don't think this change is right. You immediately remove
> >> nodes, or you queue up a commit operation (in order to batch the
> >> entire commit). What am I missing here?
> >
> > The rest of the batch of commits ;-)
>
> hehe :-) ... it is just that things like this can also be viewed in
> isolation, and didn't seem right.
>
> > I started by moving that remove deleted operation which had to be loggy
in
> > the entries+access baton world, but is 100% database now. (We don't
leave
> > directories with data, to be removed later).
> >
> > This is essentialy the db-only side of
> svn_wc_remove_from_revision_control2,
> > which is just a recursive delete
> >
> > So this handles committing svn_wc__db_status_deleted.
> >
> > And I replaced the queuing of this operation with performing this
operation
> > itself. (We explicitly ran the wq to avoid recursing over deleted nodes.
So
> > this was a safe change)
>
> Gotcha. It was that queue-run that we did which makes these equivalent.
>
> That said... we don't do the queue-run any more (you took out those,
> leaving one final run, iirc), so it would seem that we still want to
> (eventually) reach one big set of wq operations before *any* db or
> filesystem changes are made. Right?
>
> > Then I handled the specialized case where we shadow nodes. (The old
> > 'replaced' and the new cases where we can have more than 2 layers). I
> moved
> > this into the db_op, as it is a very easy recursive sql operation:
Delete
> > all nodes at shadowed op_depths.
> > This is a safe operation once the op_root defining the old top level
> > operation is committed (read: moved to BASE / op_depth 0). And that
> happens
> > in the same transaction.
>
> Alrighty.
>
> > And then we had just two commit operations left:
> > svn_wc__db_status_added and svn_wc__db_status_normal.
> >
> > And you already implemented the support for these two operations in
> wc_db
> > :-)
>
> heh.
>
> > Before my patches, the commit for these statee was handled in the
> > post_commit wq operation:
> >
> > - This fetched old information from the db
> > - Changed the DB
> > - Applied differences to the working copy.
> >
> > This order of events made it a not restartable wq operation. (Once the
db is
> > changed, the differences may be lost)
>
> Right. It is effectively the old loggy operation, just cut and paste
> into the workqueue. The things you're doing now were "to be done at
> some future point".
>
> > In r1092708, I changed this into a wq operation with only these steps:
> > - Fetch information
> > - Change the DB *and* install wq operation.
> > And a separate wq operation for applying.
>
> Yup, looks great. I will also note that OP_FILE_COMMIT "should"
> possibly go away in favor of a simple OP_FILE_INSTALL. IOW, is there
> something specific that makes OP_FILE_COMMIT different, and if so...
> why? Logically, it would seem INSTALL is all that is necessary.
In some ways it should go away, and in some ways it shouldn't.
It should be safe to completely replace the op with the install, but that
doesn't look at two things the original commit processing pre wc-ng did
* What if somebody changes the file while we perform the commit?
(The to be committed file is already in the pristine store (used to be in
.svn/tmp); so this doesn't affect the commit itself)
* Can we help our user by not changing the timestamp of their working copy
file?
I'm not sure how relevant the first part is, but I certainly like to keep
the second part.
Take our own working copy:
I edit subversion/includes/svn_types.h
Run a full testrun, etc. etc.
And then commit.
The current code notices that the in-wc actual file is the same as the
committed file and leaves this in the working copy with the current
timestamp (and records the timestamp+size as the unmodified state). So
running make just sees that the file isn't changed, and you can compile a
simple change in a .c file in a few seconds.
When I would perform OP_FILE_INSTALL, the file would be rewritten at commit
time.
And when the next 'make' is started everything is recompiled.
(But this support can certainly be added to the same WQ operation)
<snip>
> > (And then Mark Phippard ran into a few problems in svn_client_commit,
> which
> > I tried to solve in the last hour before rushing to the daycare for my
> > children...)
>
> *nod*
>
> This stuff is really looking great. We'll finally have some sanity to
> the commit process, and the code looks like it is getting much simpler
> (and thus, understandable!) ... the commit code pre-wc-ng had so many
> corner cases, it was really hard to follow.
Thanks :)
(And thanks for all your work to make this possible)
> I do believe that the endpoint is when
> svn_wc_process_committed_queue2() queues up all of the individual
> commit processing, and then runs the whole batch. That way, we can
> perform the entire commit... or nothing. The current code can still
> leave the working copy in a partially-committed state, which isn't
> really all that cool. My original concern with this commit (r1092660)
> is that moving from workqueue-to-not, but that is effectively where we
> already were (ie. partial-commits). But this commit certainly helps to
> improve the flow and understandability. Thus, it is certainly better
> than before!
>
> Thanks for taking the time, and all the detailed explanation on this
> stuff. It has really helped.
Thanks for asking!
These questions and the answers will certainly help others :)
Bert
Re: svn commit: r1092660 - in /subversion/trunk/subversion/libsvn_wc:
adm_ops.c crop.c wc_db.c wc_db.h
Posted by Greg Stein <gs...@gmail.com>.
On Fri, Apr 15, 2011 at 16:20, Bert Huijben <be...@qqmail.nl> wrote:
>> -----Original Message-----
>> From: Greg Stein [mailto:gstein@gmail.com]
>> Sent: vrijdag 15 april 2011 22:00
>> To: Bert Huijben
>> Cc: dev@subversion.apache.org
>> Subject: Re: svn commit: r1092660 - in
>> /subversion/trunk/subversion/libsvn_wc: adm_ops.c crop.c wc_db.c
>> wc_db.h
>
>> But I still don't think this change is right. You immediately remove
>> nodes, or you queue up a commit operation (in order to batch the
>> entire commit). What am I missing here?
>
> The rest of the batch of commits ;-)
hehe :-) ... it is just that things like this can also be viewed in
isolation, and didn't seem right.
> I started by moving that remove deleted operation which had to be loggy in
> the entries+access baton world, but is 100% database now. (We don't leave
> directories with data, to be removed later).
>
> This is essentialy the db-only side of svn_wc_remove_from_revision_control2,
> which is just a recursive delete
>
> So this handles committing svn_wc__db_status_deleted.
>
> And I replaced the queuing of this operation with performing this operation
> itself. (We explicitly ran the wq to avoid recursing over deleted nodes. So
> this was a safe change)
Gotcha. It was that queue-run that we did which makes these equivalent.
That said... we don't do the queue-run any more (you took out those,
leaving one final run, iirc), so it would seem that we still want to
(eventually) reach one big set of wq operations before *any* db or
filesystem changes are made. Right?
> Then I handled the specialized case where we shadow nodes. (The old
> 'replaced' and the new cases where we can have more than 2 layers). I moved
> this into the db_op, as it is a very easy recursive sql operation: Delete
> all nodes at shadowed op_depths.
> This is a safe operation once the op_root defining the old top level
> operation is committed (read: moved to BASE / op_depth 0). And that happens
> in the same transaction.
Alrighty.
> And then we had just two commit operations left:
> svn_wc__db_status_added and svn_wc__db_status_normal.
>
> And you already implemented the support for these two operations in wc_db
> :-)
heh.
> Before my patches, the commit for these statee was handled in the
> post_commit wq operation:
>
> - This fetched old information from the db
> - Changed the DB
> - Applied differences to the working copy.
>
> This order of events made it a not restartable wq operation. (Once the db is
> changed, the differences may be lost)
Right. It is effectively the old loggy operation, just cut and paste
into the workqueue. The things you're doing now were "to be done at
some future point".
> In r1092708, I changed this into a wq operation with only these steps:
> - Fetch information
> - Change the DB *and* install wq operation.
> And a separate wq operation for applying.
Yup, looks great. I will also note that OP_FILE_COMMIT "should"
possibly go away in favor of a simple OP_FILE_INSTALL. IOW, is there
something specific that makes OP_FILE_COMMIT different, and if so...
why? Logically, it would seem INSTALL is all that is necessary.
> But with that change it was also possible to do the fetch, change and
> install outside the wq.
>
> So that is the change I applied in r1092712.
Ah. Very nice. I just updated to r1092711, and yeah...
log_do_committed is a simple shell around the db operation. So this is
a pretty straightforward shift.
> (And then Mark Phippard ran into a few problems in svn_client_commit, which
> I tried to solve in the last hour before rushing to the daycare for my
> children...)
*nod*
This stuff is really looking great. We'll finally have some sanity to
the commit process, and the code looks like it is getting much simpler
(and thus, understandable!) ... the commit code pre-wc-ng had so many
corner cases, it was really hard to follow.
I do believe that the endpoint is when
svn_wc_process_committed_queue2() queues up all of the individual
commit processing, and then runs the whole batch. That way, we can
perform the entire commit... or nothing. The current code can still
leave the working copy in a partially-committed state, which isn't
really all that cool. My original concern with this commit (r1092660)
is that moving from workqueue-to-not, but that is effectively where we
already were (ie. partial-commits). But this commit certainly helps to
improve the flow and understandability. Thus, it is certainly better
than before!
Thanks for taking the time, and all the detailed explanation on this
stuff. It has really helped.
Cheers,
-g
RE: svn commit: r1092660 - in /subversion/trunk/subversion/libsvn_wc: adm_ops.c crop.c wc_db.c wc_db.h
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Greg Stein [mailto:gstein@gmail.com]
> Sent: vrijdag 15 april 2011 22:00
> To: Bert Huijben
> Cc: dev@subversion.apache.org
> Subject: Re: svn commit: r1092660 - in
> /subversion/trunk/subversion/libsvn_wc: adm_ops.c crop.c wc_db.c
> wc_db.h
> But I still don't think this change is right. You immediately remove
> nodes, or you queue up a commit operation (in order to batch the
> entire commit). What am I missing here?
The rest of the batch of commits ;-)
I started by moving that remove deleted operation which had to be loggy in
the entries+access baton world, but is 100% database now. (We don't leave
directories with data, to be removed later).
This is essentialy the db-only side of svn_wc_remove_from_revision_control2,
which is just a recursive delete
So this handles committing svn_wc__db_status_deleted.
And I replaced the queuing of this operation with performing this operation
itself. (We explicitly ran the wq to avoid recursing over deleted nodes. So
this was a safe change)
Then I handled the specialized case where we shadow nodes. (The old
'replaced' and the new cases where we can have more than 2 layers). I moved
this into the db_op, as it is a very easy recursive sql operation: Delete
all nodes at shadowed op_depths.
This is a safe operation once the op_root defining the old top level
operation is committed (read: moved to BASE / op_depth 0). And that happens
in the same transaction.
And then we had just two commit operations left:
svn_wc__db_status_added and svn_wc__db_status_normal.
And you already implemented the support for these two operations in wc_db
:-)
Before my patches, the commit for these statee was handled in the
post_commit wq operation:
- This fetched old information from the db
- Changed the DB
- Applied differences to the working copy.
This order of events made it a not restartable wq operation. (Once the db is
changed, the differences may be lost)
In r1092708, I changed this into a wq operation with only these steps:
- Fetch information
- Change the DB *and* install wq operation.
And a separate wq operation for applying.
But with that change it was also possible to do the fetch, change and
install outside the wq.
So that is the change I applied in r1092712.
(And then Mark Phippard ran into a few problems in svn_client_commit, which
I tried to solve in the last hour before rushing to the daycare for my
children...)
Bert
Re: svn commit: r1092660 - in /subversion/trunk/subversion/libsvn_wc:
adm_ops.c crop.c wc_db.c wc_db.h
Posted by Greg Stein <gs...@gmail.com>.
On Fri, Apr 15, 2011 at 15:46, Bert Huijben <be...@qqmail.nl> wrote:
>> -----Original Message-----
>> From: Greg Stein [mailto:gstein@gmail.com]
>> Sent: vrijdag 15 april 2011 20:51
>> To: dev@subversion.apache.org
>> Subject: Re: svn commit: r1092660 - in
>> /subversion/trunk/subversion/libsvn_wc: adm_ops.c crop.c wc_db.c
>> wc_db.h
>>
>> On Fri, Apr 15, 2011 at 07:03, <rh...@apache.org> wrote:
>> >...
>> > Instead of installing a wq operation to remove records from the database
>> > in the commit processing, just perform the operation directly.
>> >
>> > Working queue operations are for synchronizing the working copy with the
>> > database. In this case the working copy doesn't change at all.
>>
>> They are also used to ensure that N separate database operations are
>> all completed. You don't want the deletes to happen without the rest
>> of the commit processing, too.
>>
>> Given that... does this change still make sense? I'm not really sure
>> of the node removal within the larger context of the commit.
>
> Yes. It removes only the shadowed operations, which is safe
> Once the commit for this node has been processed the shadowed operations
> below this layer are invalid by itself, as they rely on their op_roots,
> which are being removed by the processing. (And this happens in the same
> transaction as the removal of that op_root). So it actually avoids db
> instability.
>
> Before this patch series we did:
> Queue one operation
> Queue one operation
> Run operations
> (db is in an unstable situation here)
> Queue one operation
> Run one operation,
> (etc)
Oh, I totally agree with the delay of the queue-run. That was an
artifact of multi-db operation (and the old loggy files).
>
> (and some of these operations were not restartable yet)
>
>
> But why queue db-only changes when we can just perform them directly with
> the same atomic guarantees?
> (The SQLite transaction guarantees that the db_op_commit_node is completely
> evaluated or not at all; same guarantee as queuing a single wq operation to
> perform that operation)
Well.. it looked like the op_remove_node was called from
process_committed_leaf. That is outside any guarantees.
On the other hand, db_global_commit is called from within the
workqueue. Thus, "all" commits should be getting processed during the
queue-run.
> The code shifting now even allows moving all the processing (and the queuing
> of the wq operations) in a single sqlite transaction which would improve
> stability and performance even further.
I agree with the sentiment, and I do agree that the commit processing
is not our ideal of "db changes and queue items in one txn". It is
kind of backwards now as a result of the old loggy style, and before I
had really settled on how wc_db and wq should interact (the transacted
insertion of items, for example).
But I still don't think this change is right. You immediately remove
nodes, or you queue up a commit operation (in order to batch the
entire commit). What am I missing here?
Cheers,
-g
RE: svn commit: r1092660 - in /subversion/trunk/subversion/libsvn_wc: adm_ops.c crop.c wc_db.c wc_db.h
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Greg Stein [mailto:gstein@gmail.com]
> Sent: vrijdag 15 april 2011 20:51
> To: dev@subversion.apache.org
> Subject: Re: svn commit: r1092660 - in
> /subversion/trunk/subversion/libsvn_wc: adm_ops.c crop.c wc_db.c
> wc_db.h
>
> On Fri, Apr 15, 2011 at 07:03, <rh...@apache.org> wrote:
> >...
> > Instead of installing a wq operation to remove records from the database
> > in the commit processing, just perform the operation directly.
> >
> > Working queue operations are for synchronizing the working copy with the
> > database. In this case the working copy doesn't change at all.
>
> They are also used to ensure that N separate database operations are
> all completed. You don't want the deletes to happen without the rest
> of the commit processing, too.
>
> Given that... does this change still make sense? I'm not really sure
> of the node removal within the larger context of the commit.
Yes. It removes only the shadowed operations, which is safe
Once the commit for this node has been processed the shadowed operations
below this layer are invalid by itself, as they rely on their op_roots,
which are being removed by the processing. (And this happens in the same
transaction as the removal of that op_root). So it actually avoids db
instability.
Before this patch series we did:
Queue one operation
Queue one operation
Run operations
(db is in an unstable situation here)
Queue one operation
Run one operation,
(etc)
(and some of these operations were not restartable yet)
But why queue db-only changes when we can just perform them directly with
the same atomic guarantees?
(The SQLite transaction guarantees that the db_op_commit_node is completely
evaluated or not at all; same guarantee as queuing a single wq operation to
perform that operation)
The code shifting now even allows moving all the processing (and the queuing
of the wq operations) in a single sqlite transaction which would improve
stability and performance even further.
Bert
>
> Cheers,
> -g
Re: svn commit: r1092660 - in /subversion/trunk/subversion/libsvn_wc:
adm_ops.c crop.c wc_db.c wc_db.h
Posted by Greg Stein <gs...@gmail.com>.
On Fri, Apr 15, 2011 at 07:03, <rh...@apache.org> wrote:
>...
> Instead of installing a wq operation to remove records from the database
> in the commit processing, just perform the operation directly.
>
> Working queue operations are for synchronizing the working copy with the
> database. In this case the working copy doesn't change at all.
They are also used to ensure that N separate database operations are
all completed. You don't want the deletes to happen without the rest
of the commit processing, too.
Given that... does this change still make sense? I'm not really sure
of the node removal within the larger context of the commit.
Cheers,
-g