Re: svn commit: r1494298 - /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c

[Philip Martin <ph...@wandisco.com>]

stefan2@apache.org writes:

> Author: stefan2
> Date: Tue Jun 18 20:59:07 2013
> New Revision: 1494298

>  static svn_error_t *
> @@ -1501,6 +1507,29 @@ upgrade_pack_revprops(svn_fs_t *fs,
>        svn_pool_clear(iterpool);
>      }

Is there any reason for upgrade_pack_revprops and
upgrade_cleanup_pack_revprops to clear the pool at the end of the loop
rather than the standard pattern at the start of the loop?

>    /* Bump the format file. */
> -  return write_format(format_path, SVN_FS_FS__FORMAT_NUMBER, max_files_per_dir,
> -                      TRUE, pool);
> +  SVN_ERR(write_format(format_path, SVN_FS_FS__FORMAT_NUMBER,
> +                       max_files_per_dir, TRUE, pool));
> +
> +  /* Now, it is safe to remove the redundant revprop files. */
> +  if (needs_revprop_shard_cleanup)
> +    SVN_ERR(upgrade_pack_revprops(fs, pool));

That's corrected to upgrade_cleanup_pack_revprops in a later revision.

This whole change is to handle a rerun after an interrupted upgrade but
suppose the rerun is itself interupted between write_format and
upgrade_cleanup_pack_revprops.  Is the user reduced to manually removing
the revprop files?  Neither upgrade nor pack appear to do it.

Perhaps pack should cleanup old revprop files?

-- 
Philip Martin | Subversion Committer
WANdisco | Non-Stop Data
www.wandisco.com

Re: svn commit: r1494298 - /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c

[Stefan Fuhrmann <st...@wandisco.com>]

On Wed, Jun 19, 2013 at 3:30 PM, Philip Martin
<ph...@wandisco.com>wrote:

> stefan2@apache.org writes:
>
> > Author: stefan2
> > Date: Tue Jun 18 20:59:07 2013
> > New Revision: 1494298
>
> >  static svn_error_t *
> > @@ -1501,6 +1507,29 @@ upgrade_pack_revprops(svn_fs_t *fs,
> >        svn_pool_clear(iterpool);
> >      }
>
> Is there any reason for upgrade_pack_revprops and
> upgrade_cleanup_pack_revprops to clear the pool at the end of the loop
> rather than the standard pattern at the start of the loop?
>
> >    /* Bump the format file. */
> > -  return write_format(format_path, SVN_FS_FS__FORMAT_NUMBER,
> max_files_per_dir,
> > -                      TRUE, pool);
> > +  SVN_ERR(write_format(format_path, SVN_FS_FS__FORMAT_NUMBER,
> > +                       max_files_per_dir, TRUE, pool));
> > +
> > +  /* Now, it is safe to remove the redundant revprop files. */
> > +  if (needs_revprop_shard_cleanup)
> > +    SVN_ERR(upgrade_pack_revprops(fs, pool));
>
> That's corrected to upgrade_cleanup_pack_revprops in a later revision.
>
> This whole change is to handle a rerun after an interrupted upgrade but
> suppose the rerun is itself interupted between write_format and
> upgrade_cleanup_pack_revprops.  Is the user reduced to manually removing
> the revprop files?  Neither upgrade nor pack appear to do it.
>

I consider them "leftovers" like abandoned transactions.
But I agree that we should do something about them.

Perhaps pack should cleanup old revprop files?
>

Good idea. Done in r1495566. Not foolproof but covers
99% percent of the already fringy "I cancel upgrade' case.

-- Stefan^2.