You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ju...@apache.org on 2009/12/22 15:25:51 UTC
svn commit: r893179 - in /subversion/trunk/subversion/libsvn_fs_base: dag.c
revs-txns.c
Author: julianfoad
Date: Tue Dec 22 14:25:51 2009
New Revision: 893179
URL: http://svn.apache.org/viewvc?rev=893179&view=rev
Log:
For obliterate in BDB, change the way we update the "copies" table.
Instead of updating it at oblit-begin time, update it at oblit-commit time,
which is not necessarily correct but at least ensures no user-visible
changes are made until oblit-commit time, which is a step closer to
correctness.
* subversion/libsvn_fs_base/revs-txns.c
(copy_dup): Removed.
(txn_body_begin_obliteration_txn): Instead of updating the "copies" table,
just refer to the existing rows in it, even though they do not refer
back to this txn.
* subversion/libsvn_fs_base/dag.c
(svn_fs_base__dag_commit_obliteration_txn): Update the "copies" table to
refer to the new txn.
Modified:
subversion/trunk/subversion/libsvn_fs_base/dag.c
subversion/trunk/subversion/libsvn_fs_base/revs-txns.c
Modified: subversion/trunk/subversion/libsvn_fs_base/dag.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_base/dag.c?rev=893179&r1=893178&r2=893179&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_base/dag.c (original)
+++ subversion/trunk/subversion/libsvn_fs_base/dag.c Tue Dec 22 14:25:51 2009
@@ -1631,8 +1631,61 @@
trail_t *trail,
apr_pool_t *pool)
{
+ transaction_t *txn_obj;
revision_t revision;
+ /* Read the txn so we can access its "copies" list */
+ SVN_ERR(svn_fs_bdb__get_txn(&txn_obj, trail->fs, txn->id, trail,
+ trail->pool));
+
+ /* Finish updating the "copies" table, which was started in
+ * svn_fs_base__begin_obliteration_txn(). Change the keys of all the
+ * "copies" table rows that we created back to their original keys.
+ *
+ * This assumes that the old 'replacing_rev' transaction is now obsolete,
+ * so that the old "copies" table rows are no longer referenced.
+ *
+ * ### TODO: See txn_body_begin_obliteration_txn().
+ * ### TODO: Guarantee the old txn is obsolete.
+ */
+ if (txn_obj->copies)
+ {
+ int i;
+
+ for (i = 0; i < txn_obj->copies->nelts; i++)
+ {
+ const char *txn_copy_id = APR_ARRAY_IDX(txn_obj->copies, i,
+ const char *);
+ const char *final_copy_id;
+ copy_t *copy;
+ const char *id_node_id, *id_copy_id, *id_txn_id;
+
+ /* Get the old copy */
+ SVN_ERR(svn_fs_bdb__get_copy(©, trail->fs, txn_copy_id, trail,
+ pool));
+
+ /* Modify it: change dst_noderev_id's txn_id to the new txn's id */
+ id_node_id = svn_fs_base__id_node_id(copy->dst_noderev_id);
+ id_copy_id = svn_fs_base__id_copy_id(copy->dst_noderev_id);
+ id_txn_id = svn_fs_base__id_txn_id(copy->dst_noderev_id);
+ /* SVN_ERR_ASSERT(svn_fs_base__key_compare(id_copy_id, old_copy_id)
+ == 0); */
+ /* SVN_ERR_ASSERT(svn_fs_base__key_compare(id_txn_id, old_txn_id)
+ == 0); */
+ copy->dst_noderev_id = svn_fs_base__id_create(id_node_id,
+ id_copy_id,
+ txn->id,
+ pool);
+
+ /* Save the new copy */
+ final_copy_id = id_copy_id;
+ SVN_ERR(svn_fs_bdb__create_copy(trail->fs, final_copy_id,
+ copy->src_path, copy->src_txn_id,
+ copy->dst_noderev_id, copy->kind,
+ trail, pool));
+ }
+ }
+
/* Replace the revision entry in the `revisions' table. */
revision.txn_id = txn->id;
SVN_ERR(svn_fs_bdb__put_rev(&replacing_rev, txn->fs, &revision, trail, pool));
Modified: subversion/trunk/subversion/libsvn_fs_base/revs-txns.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_base/revs-txns.c?rev=893179&r1=893178&r2=893179&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_base/revs-txns.c (original)
+++ subversion/trunk/subversion/libsvn_fs_base/revs-txns.c Tue Dec 22 14:25:51 2009
@@ -434,45 +434,6 @@
}
-/* Create a new row in the "copies" table that is a deep copy of the row
- * keyed by OLD_COPY_ID. Assume that the txn-id component of its
- * 'dst_noderev_id' field is OLD_TXN_ID, and change that to NEW_TXN_ID.
- * Set *NEW_COPY_ID to the key of the new row.
- * Work in TRAIL and allocate *NEW_COPY_ID in TRAIL->POOL. */
-static svn_error_t *
-copy_dup(const char **new_copy_id,
- const char *old_copy_id,
- const char *new_txn_id,
- const char *old_txn_id,
- trail_t *trail,
- apr_pool_t *scratch_pool)
-{
- copy_t *copy;
- const char *node_id, *copy_id, *txn_id;
-
- /* Get the old copy */
- SVN_ERR(svn_fs_bdb__get_copy(©, trail->fs, old_copy_id, trail,
- scratch_pool));
-
- /* Modify it: change dst_noderev_id's txn_id to NEW_TXN_ID */
- node_id = svn_fs_base__id_node_id(copy->dst_noderev_id);
- copy_id = svn_fs_base__id_copy_id(copy->dst_noderev_id);
- txn_id = svn_fs_base__id_txn_id(copy->dst_noderev_id);
- SVN_ERR_ASSERT(svn_fs_base__key_compare(copy_id, old_copy_id) == 0);
- SVN_ERR_ASSERT(svn_fs_base__key_compare(txn_id, old_txn_id) == 0);
- copy->dst_noderev_id = svn_fs_base__id_create(node_id, copy_id, new_txn_id,
- scratch_pool);
-
- /* Save the new copy */
- SVN_ERR(svn_fs_bdb__reserve_copy_id(new_copy_id, trail->fs, trail,
- trail->pool));
- SVN_ERR(svn_fs_bdb__create_copy(trail->fs, *new_copy_id, copy->src_path,
- copy->src_txn_id, copy->dst_noderev_id,
- copy->kind, trail, scratch_pool));
- return SVN_NO_ERROR;
-}
-
-
/* Duplicate all entries in the "changes" table that are keyed by OLD_TXN_ID,
* creating new entries that are keyed by NEW_TXN_ID.
*
@@ -863,41 +824,25 @@
/* Dup txn->proplist */
new_txn->proplist = old_txn->proplist;
- /* ### TODO: Update "copies" table entries referenced by txn->copies.
- * This is hard, because I don't want to change the copy_ids, because they
- * pervade node-ids throughout history. But what actually uses them, and
- * does anything use them during txn construction? */
-
- /* Dup txn->copies
+ /* Prepare to update the "copies" table.
*
- * ### PROBLEM:
- * This code makes new rows in the 'copies' table, keyed by a NEW COPY-ID
- * that is not the copy-id of the node-rev it refers to. WRONG!
+ * As part of obliteration, we need to update all of the "copies" table
+ * rows that are referenced by this txn, to refer to the new txn's
+ * node-rev ids instead. At txn begin time, just keep the references to
+ * the old rows. At commit time, we will update those rows to refer to
+ * this txn's node-rev ids.
*
- * For the purpose of the txn keeping track of which "copies" table rows
- * it allocated, this is fine. It is no good if something needs to look
- * up copy info based on copy-id during txn construction.
+ * We cannot simply create new "copies" table rows now and make the old
+ * ones obsolete at commit time, because the rows are keyed by copy-id,
+ * and we don't want to change the copy_ids because they pervade node-ids
+ * throughout history.
*
- * If no look-ups are required until after the txn is committed, maybe we
- * could overwrite the old "copies" table entries with the new ones at
- * commit time.
- */
+ * ### What actually uses the "copies" table? Does anything use it during
+ * txn construction? Does it need to be keyed by copy-id or could we
+ * change the schema to use arbitrary keys? */
if (old_txn->copies)
{
- int i;
-
- new_txn->copies = apr_array_make(trail->pool, old_txn->copies->nelts,
- sizeof(const char *));
- for (i = 0; i < old_txn->copies->nelts; i++)
- {
- const char *old_copy_id = APR_ARRAY_IDX(old_txn->copies, i,
- const char *);
- const char *new_copy_id;
-
- SVN_ERR(copy_dup(&new_copy_id, old_copy_id, new_txn_id, old_txn_id,
- trail, trail->pool));
- APR_ARRAY_PUSH(new_txn->copies, const char *) = new_copy_id;
- }
+ new_txn->copies = apr_array_copy(trail->pool, old_txn->copies);
}
/* Dup the "changes" that are keyed by the txn_id. */
Re: svn commit: r893179 - in
/subversion/trunk/subversion/libsvn_fs_base: dag.c revs-txns.c
Posted by Julian Foad <ju...@wandisco.com>.
I (Julian Foad) wrote:
> Philip Martin wrote:
> >
> > Is this loop big enough to warrent an iteration pool?
>
> It could be large in some revisions, so yes.
[...]
> > Do those comparisons fail or what?
>
> Oh, no, I commented them out because the function didn't have the
> old_copy_id and old_txn_id to compare with. Now it does have the
> old_txn_id, and the old copy_id is called txn_copy_id.
[...]
> Thank you for reviewing, Philip. I'll fix pool usage and re-enable the
> asserts.
In r893507 I moved that loop out to a separate function and gave it an
iterpool and re-enabled the assertions. I also changed the outer
function (svn_fs_base__dag_commit_obliteration_txn)'s pool parameter to
"scratch_pool" and made use of it.
Please let me know any more improvements you spot.
- Julian
Re: svn commit: r893179 - in
/subversion/trunk/subversion/libsvn_fs_base: dag.c revs-txns.c
Posted by Julian Foad <ju...@wandisco.com>.
Philip Martin wrote:
> node_origins_update() also has a loop that doesn't use an iteration
> pool:
>
> SVN_ERR(svn_fs_bdb__changes_fetch_raw(&changes, trail->fs, old_txn_id, trail,
> scratch_pool));
> for (i = 0; i < changes->nelts; i++)
> {
>
> I suppose fetch_raw is already allocating memory that is proportional
> to the number of changes, so perhaps that's sufficient justification
> not to use an iteration pool in this case.
Still worth doing. Done in r893509.
Thanks.
- Julian
Re: svn commit: r893179 - in /subversion/trunk/subversion/libsvn_fs_base: dag.c revs-txns.c
Posted by Philip Martin <ph...@wandisco.com>.
Julian Foad <ju...@wandisco.com> writes:
> Philip Martin wrote:
>> julianfoad@apache.org writes:
>>
>> > + if (txn_obj->copies)
>> > + {
>> > + int i;
>> > +
>> > + for (i = 0; i < txn_obj->copies->nelts; i++)
>> > + {
>>
>> Is this loop big enough to warrent an iteration pool?
>
> It could be large in some revisions, so yes.
node_origins_update() also has a loop that doesn't use an iteration
pool:
SVN_ERR(svn_fs_bdb__changes_fetch_raw(&changes, trail->fs, old_txn_id, trail,
scratch_pool));
for (i = 0; i < changes->nelts; i++)
{
I suppose fetch_raw is already allocating memory that is proportional
to the number of changes, so perhaps that's sufficient justification
not to use an iteration pool in this case.
--
Philip
Re: svn commit: r893179 - in
/subversion/trunk/subversion/libsvn_fs_base: dag.c revs-txns.c
Posted by Julian Foad <ju...@wandisco.com>.
Philip Martin wrote:
> julianfoad@apache.org writes:
>
> > + if (txn_obj->copies)
> > + {
> > + int i;
> > +
> > + for (i = 0; i < txn_obj->copies->nelts; i++)
> > + {
>
> Is this loop big enough to warrent an iteration pool?
It could be large in some revisions, so yes.
> > + const char *txn_copy_id = APR_ARRAY_IDX(txn_obj->copies, i,
> > + const char *);
> > + const char *final_copy_id;
> > + copy_t *copy;
> > + const char *id_node_id, *id_copy_id, *id_txn_id;
> > +
> > + /* Get the old copy */
> > + SVN_ERR(svn_fs_bdb__get_copy(©, trail->fs, txn_copy_id, trail,
> > + pool));
> > +
> > + /* Modify it: change dst_noderev_id's txn_id to the new txn's id */
> > + id_node_id = svn_fs_base__id_node_id(copy->dst_noderev_id);
> > + id_copy_id = svn_fs_base__id_copy_id(copy->dst_noderev_id);
> > + id_txn_id = svn_fs_base__id_txn_id(copy->dst_noderev_id);
> > + /* SVN_ERR_ASSERT(svn_fs_base__key_compare(id_copy_id, old_copy_id)
> > + == 0); */
> > + /* SVN_ERR_ASSERT(svn_fs_base__key_compare(id_txn_id, old_txn_id)
> > + == 0); */
>
> Do those comparisons fail or what?
Oh, no, I commented them out because the function didn't have the
old_copy_id and old_txn_id to compare with. Now it does have the
old_txn_id, and the old copy_id is called txn_copy_id.
> > + copy->dst_noderev_id = svn_fs_base__id_create(id_node_id,
> > + id_copy_id,
> > + txn->id,
> > + pool);
> > +
> > + /* Save the new copy */
> > + final_copy_id = id_copy_id;
> > + SVN_ERR(svn_fs_bdb__create_copy(trail->fs, final_copy_id,
> > + copy->src_path, copy->src_txn_id,
> > + copy->dst_noderev_id, copy->kind,
> > + trail, pool));
> > + }
> > + }
Thank you for reviewing, Philip. I'll fix pool usage and re-enable the
asserts.
- Julian
Re: svn commit: r893179 - in /subversion/trunk/subversion/libsvn_fs_base: dag.c revs-txns.c
Posted by Philip Martin <ph...@wandisco.com>.
julianfoad@apache.org writes:
> + if (txn_obj->copies)
> + {
> + int i;
> +
> + for (i = 0; i < txn_obj->copies->nelts; i++)
> + {
Is this loop big enough to warrent an iteration pool?
> + const char *txn_copy_id = APR_ARRAY_IDX(txn_obj->copies, i,
> + const char *);
> + const char *final_copy_id;
> + copy_t *copy;
> + const char *id_node_id, *id_copy_id, *id_txn_id;
> +
> + /* Get the old copy */
> + SVN_ERR(svn_fs_bdb__get_copy(©, trail->fs, txn_copy_id, trail,
> + pool));
> +
> + /* Modify it: change dst_noderev_id's txn_id to the new txn's id */
> + id_node_id = svn_fs_base__id_node_id(copy->dst_noderev_id);
> + id_copy_id = svn_fs_base__id_copy_id(copy->dst_noderev_id);
> + id_txn_id = svn_fs_base__id_txn_id(copy->dst_noderev_id);
> + /* SVN_ERR_ASSERT(svn_fs_base__key_compare(id_copy_id, old_copy_id)
> + == 0); */
> + /* SVN_ERR_ASSERT(svn_fs_base__key_compare(id_txn_id, old_txn_id)
> + == 0); */
Do those comparisons fail or what?
> + copy->dst_noderev_id = svn_fs_base__id_create(id_node_id,
> + id_copy_id,
> + txn->id,
> + pool);
> +
> + /* Save the new copy */
> + final_copy_id = id_copy_id;
> + SVN_ERR(svn_fs_bdb__create_copy(trail->fs, final_copy_id,
> + copy->src_path, copy->src_txn_id,
> + copy->dst_noderev_id, copy->kind,
> + trail, pool));
> + }
> + }
--
Philip