This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9770

upgrade: don't create store backup if `--no-backup` is passed
ClosedPublic

Authored by pulkit on Jan 14 2021, 7:54 AM.

Details

Reviewers
mharbison72
Group Reviewers
hg-reviewers
Commits
rHG2e8a844d0ae0: upgrade: don't create store backup if `--no-backup` is passed
Summary

If the user explicitly mentioned that they don't need backup, then let's not
create it.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pulkit created this revision.Jan 14 2021, 7:54 AM
mharbison72 accepted this revision.Jan 18 2021, 9:15 PM
This revision is now accepted and ready to land.Jan 18 2021, 9:15 PM
baymax updated this revision to Diff 25198.Jan 21 2021, 9:28 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

indygreg added a subscriber: indygreg.Feb 6 2021, 2:15 PM
indygreg added inline comments.
mercurial/upgrade_utils/engine.py
418

This change makes the repository inconsistent for longer than before.

With the old code, we performed 2 atomic directory renames, which likely completely ~instantaneously, leaving the repository inconsistent for a short window.

With this new code path, we perform the equivalent of rm -rf on store/ then perform an atomic rename of the new store to the proper path.

The problem with the new code path is that the recursive delete can take seconds to perform, leaving the repository inconsistent for that duration.

A better solution would be to rename the old store directory, swap in the new store (the old code path), then perform the rmtree() after the new store is installed.

pulkit added inline comments.Feb 6 2021, 2:26 PM
mercurial/upgrade_utils/engine.py
418

TBH, I am on edge that whether having store inconsistent for less time is helping us or preventing faster upgrades. Not talking about this specific patch but an attempt at a different thing that received a similar objection: https://phab.mercurial-scm.org/D9677

As far as this patch is concerned, actually, we used the better solution before and I undid it in this patch. Feel free to revert it as I don't feel strongly about it.

Revision Contents
Changeset List

Diff 25255

mercurial/upgrade.py

upgrade_op = upgrade_actions.UpgradeOperation( upgrade_op = upgrade_actions.UpgradeOperation(
ui, ui,
newreqs, newreqs,
repo.requirements, repo.requirements,
up_actions, up_actions,
removed_actions, removed_actions,
revlogs, revlogs,
backup,
) )
if not run: if not run:
fromconfig = [] fromconfig = []
onlydefault = [] onlydefault = []
for d in format_upgrades: for d in format_upgrades:
if d.fromconfig(repo): if d.fromconfig(repo):
# clone ui without using ui.copy because repo.ui is protected # clone ui without using ui.copy because repo.ui is protected
repoui = repo.ui.__class__(repo.ui) repoui = repo.ui.__class__(repo.ui)
dstrepo = hg.repository(repoui, path=tmppath, create=True) dstrepo = hg.repository(repoui, path=tmppath, create=True)
with dstrepo.wlock(), dstrepo.lock(): with dstrepo.wlock(), dstrepo.lock():
backuppath = upgrade_engine.upgrade( backuppath = upgrade_engine.upgrade(
ui, repo, dstrepo, upgrade_op ui, repo, dstrepo, upgrade_op
) )
if not backup:
ui.status(
_(b'removing old repository content %s\n') % backuppath
)
repo.vfs.rmtree(backuppath, forcibly=True)
backuppath = None
finally: finally:
ui.status(_(b'removing temporary repository %s\n') % tmppath) ui.status(_(b'removing temporary repository %s\n') % tmppath)
repo.vfs.rmtree(tmppath, forcibly=True) repo.vfs.rmtree(tmppath, forcibly=True)
if backuppath and not ui.quiet: if backuppath and not ui.quiet:
ui.warn( ui.warn(
_(b'copy of old repository backed up at %s\n') % backuppath _(b'copy of old repository backed up at %s\n') % backuppath

mercurial/upgrade_utils/actions.py

def __init__( def __init__(
self, self,
ui, ui,
new_requirements, new_requirements,
current_requirements, current_requirements,
upgrade_actions, upgrade_actions,
removed_actions, removed_actions,
revlogs_to_process, revlogs_to_process,
backup_store,
): ):
self.ui = ui self.ui = ui
self.new_requirements = new_requirements self.new_requirements = new_requirements
self.current_requirements = current_requirements self.current_requirements = current_requirements
# list of upgrade actions the operation will perform # list of upgrade actions the operation will perform
self.upgrade_actions = upgrade_actions self.upgrade_actions = upgrade_actions
self._upgrade_actions_names = set([a.name for a in upgrade_actions]) self._upgrade_actions_names = set([a.name for a in upgrade_actions])
self.removed_actions = removed_actions self.removed_actions = removed_actions
elif b're-delta-fulladd' in self._upgrade_actions_names: elif b're-delta-fulladd' in self._upgrade_actions_names:
self.delta_reuse_mode = revlog.revlog.DELTAREUSEFULLADD self.delta_reuse_mode = revlog.revlog.DELTAREUSEFULLADD
# should this operation force re-delta of both parents # should this operation force re-delta of both parents
self.force_re_delta_both_parents = ( self.force_re_delta_both_parents = (
b're-delta-multibase' in self._upgrade_actions_names b're-delta-multibase' in self._upgrade_actions_names
) )
# should this operation create a backup of the store
self.backup_store = backup_store
def _write_labeled(self, l, label): def _write_labeled(self, l, label):
""" """
Utility function to aid writing of a list under one label Utility function to aid writing of a list under one label
""" """
first = True first = True
for r in sorted(l): for r in sorted(l):
if not first: if not first:
self.ui.write(b', ') self.ui.write(b', ')

mercurial/upgrade_utils/engine.py

currentrepo: repo object of current repository currentrepo: repo object of current repository
upgradedrepo: repo object of the upgraded data upgradedrepo: repo object of the upgraded data
backupvfs: vfs object for the backup path backupvfs: vfs object for the backup path
upgrade_op: upgrade operation object upgrade_op: upgrade operation object
to be used to decide what all is upgraded to be used to decide what all is upgraded
""" """
# TODO: don't blindly rename everything in store # TODO: don't blindly rename everything in store
# There can be upgrades where store is not touched at all # There can be upgrades where store is not touched at all
if upgrade_op.backup_store:
util.rename(currentrepo.spath, backupvfs.join(b'store')) util.rename(currentrepo.spath, backupvfs.join(b'store'))
else:
currentrepo.vfs.rmtree(b'store', forcibly=True)
indygregUnsubmitted
Not Done

This change makes the repository inconsistent for longer than before.

With the old code, we performed 2 atomic directory renames, which likely completely ~instantaneously, leaving the repository inconsistent for a short window.

With this new code path, we perform the equivalent of rm -rf on store/ then perform an atomic rename of the new store to the proper path.

The problem with the new code path is that the recursive delete can take seconds to perform, leaving the repository inconsistent for that duration.

A better solution would be to rename the old store directory, swap in the new store (the old code path), then perform the rmtree() after the new store is installed.

indygreg: This change makes the repository inconsistent for longer than before. With the old code, we…
pulkitAuthorUnsubmitted
Done

TBH, I am on edge that whether having store inconsistent for less time is helping us or preventing faster upgrades. Not talking about this specific patch but an attempt at a different thing that received a similar objection: https://phab.mercurial-scm.org/D9677

As far as this patch is concerned, actually, we used the better solution before and I undid it in this patch. Feel free to revert it as I don't feel strongly about it.

pulkit: TBH, I am on edge that whether having store inconsistent for less time is helping us or…
util.rename(upgradedrepo.spath, currentrepo.spath) util.rename(upgradedrepo.spath, currentrepo.spath)
def finishdatamigration(ui, srcrepo, dstrepo, requirements): def finishdatamigration(ui, srcrepo, dstrepo, requirements):
"""Hook point for extensions to perform additional actions during upgrade. """Hook point for extensions to perform additional actions during upgrade.
This function is called after revlogs and store files have been copied but This function is called after revlogs and store files have been copied but
before the new store is swapped into the original location. before the new store is swapped into the original location.
""" """
def upgrade(ui, srcrepo, dstrepo, upgrade_op): def upgrade(ui, srcrepo, dstrepo, upgrade_op):
"""Do the low-level work of upgrading a repository. """Do the low-level work of upgrading a repository.
The upgrade is effectively performed as a copy between a source The upgrade is effectively performed as a copy between a source
repository and a temporary destination repository. repository and a temporary destination repository.
The source repository is unmodified for as long as possible so the The source repository is unmodified for as long as possible so the
upgrade can abort at any time without causing loss of service for upgrade can abort at any time without causing loss of service for
readers and without corrupting the source repository. readers and without corrupting the source repository.
""" """
assert srcrepo.currentwlock() assert srcrepo.currentwlock()
assert dstrepo.currentwlock() assert dstrepo.currentwlock()
backuppath = None
backupvfs = None
ui.status( ui.status(
_( _(
b'(it is safe to interrupt this process any time before ' b'(it is safe to interrupt this process any time before '
b'data migration completes)\n' b'data migration completes)\n'
) )
) )
src = srcrepo.store.rawvfs.join(p) src = srcrepo.store.rawvfs.join(p)
dst = dstrepo.store.rawvfs.join(p) dst = dstrepo.store.rawvfs.join(p)
util.copyfile(src, dst, copystat=True) util.copyfile(src, dst, copystat=True)
finishdatamigration(ui, srcrepo, dstrepo, requirements) finishdatamigration(ui, srcrepo, dstrepo, requirements)
ui.status(_(b'data fully upgraded in a temporary repository\n')) ui.status(_(b'data fully upgraded in a temporary repository\n'))
backuppath = pycompat.mkdtemp(prefix=b'upgradebackup.', dir=srcrepo.path) if upgrade_op.backup_store:
backuppath = pycompat.mkdtemp(
prefix=b'upgradebackup.', dir=srcrepo.path
)
backupvfs = vfsmod.vfs(backuppath) backupvfs = vfsmod.vfs(backuppath)
# Make a backup of requires file first, as it is the first to be modified. # Make a backup of requires file first, as it is the first to be modified.
util.copyfile(srcrepo.vfs.join(b'requires'), backupvfs.join(b'requires')) util.copyfile(
srcrepo.vfs.join(b'requires'), backupvfs.join(b'requires')
)
# We install an arbitrary requirement that clients must not support # We install an arbitrary requirement that clients must not support
# as a mechanism to lock out new clients during the data swap. This is # as a mechanism to lock out new clients during the data swap. This is
# better than allowing a client to continue while the repository is in # better than allowing a client to continue while the repository is in
# an inconsistent state. # an inconsistent state.
ui.status( ui.status(
_( _(
b'marking source repository as being upgraded; clients will be ' b'marking source repository as being upgraded; clients will be '
b'unable to read from repository\n' b'unable to read from repository\n'
) )
) )
scmutil.writereporequirements( scmutil.writereporequirements(
srcrepo, srcrepo.requirements | {b'upgradeinprogress'} srcrepo, srcrepo.requirements | {b'upgradeinprogress'}
) )
ui.status(_(b'starting in-place swap of repository data\n')) ui.status(_(b'starting in-place swap of repository data\n'))
if upgrade_op.backup_store:
ui.status(_(b'replaced files will be backed up at %s\n') % backuppath) ui.status(_(b'replaced files will be backed up at %s\n') % backuppath)
# Now swap in the new store directory. Doing it as a rename should make # Now swap in the new store directory. Doing it as a rename should make
# the operation nearly instantaneous and atomic (at least in well-behaved # the operation nearly instantaneous and atomic (at least in well-behaved
# environments). # environments).
ui.status(_(b'replacing store...\n')) ui.status(_(b'replacing store...\n'))
tstart = util.timer() tstart = util.timer()
_replacestores(srcrepo, dstrepo, backupvfs, upgrade_op) _replacestores(srcrepo, dstrepo, backupvfs, upgrade_op)
elapsed = util.timer() - tstart elapsed = util.timer() - tstart
ui.status( ui.status(
_( _(
b'finalizing requirements file and making repository readable ' b'finalizing requirements file and making repository readable '
b'again\n' b'again\n'
) )
) )
scmutil.writereporequirements(srcrepo, upgrade_op.new_requirements) scmutil.writereporequirements(srcrepo, upgrade_op.new_requirements)
if upgrade_op.backup_store:
# The lock file from the old store won't be removed because nothing has a # The lock file from the old store won't be removed because nothing has a
# reference to its new location. So clean it up manually. Alternatively, we # reference to its new location. So clean it up manually. Alternatively, we
# could update srcrepo.svfs and other variables to point to the new # could update srcrepo.svfs and other variables to point to the new
# location. This is simpler. # location. This is simpler.
backupvfs.unlink(b'store/lock') backupvfs.unlink(b'store/lock')
return backuppath return backuppath

tests/test-upgrade-repo.t

finished migrating 3 manifest revisions across 1 manifests; change in size: 0 bytes finished migrating 3 manifest revisions across 1 manifests; change in size: 0 bytes
migrating changelog containing 3 revisions (394 bytes in store; 199 bytes tracked data) migrating changelog containing 3 revisions (394 bytes in store; 199 bytes tracked data)
finished migrating 3 changelog revisions; change in size: 0 bytes finished migrating 3 changelog revisions; change in size: 0 bytes
finished migrating 9 total revisions; total change in store size: 0 bytes finished migrating 9 total revisions; total change in store size: 0 bytes
copying phaseroots copying phaseroots
data fully upgraded in a temporary repository data fully upgraded in a temporary repository
marking source repository as being upgraded; clients will be unable to read from repository marking source repository as being upgraded; clients will be unable to read from repository
starting in-place swap of repository data starting in-place swap of repository data
replaced files will be backed up at $TESTTMP/upgradegd/.hg/upgradebackup.* (glob)
replacing store... replacing store...
store replacement complete; repository was inconsistent for * (glob) store replacement complete; repository was inconsistent for * (glob)
finalizing requirements file and making repository readable again finalizing requirements file and making repository readable again
removing old repository content $TESTTMP/upgradegd/.hg/upgradebackup.* (glob)
removing temporary repository $TESTTMP/upgradegd/.hg/upgrade.* (glob) removing temporary repository $TESTTMP/upgradegd/.hg/upgrade.* (glob)
$ ls -1 .hg/ | grep upgradebackup $ ls -1 .hg/ | grep upgradebackup
[1] [1]
We can restrict optimization to some revlog: We can restrict optimization to some revlog:
$ hg debugupgrade --optimize re-delta-parent --run --manifest --no-backup --debug --traceback $ hg debugupgrade --optimize re-delta-parent --run --manifest --no-backup --debug --traceback
upgrade will perform the following actions: upgrade will perform the following actions:
migrating changelog containing 3 revisions (394 bytes in store; 199 bytes tracked data) migrating changelog containing 3 revisions (394 bytes in store; 199 bytes tracked data)
blindly copying 00changelog.i containing 3 revisions blindly copying 00changelog.i containing 3 revisions
finished migrating 3 changelog revisions; change in size: 0 bytes finished migrating 3 changelog revisions; change in size: 0 bytes
finished migrating 9 total revisions; total change in store size: 0 bytes finished migrating 9 total revisions; total change in store size: 0 bytes
copying phaseroots copying phaseroots
data fully upgraded in a temporary repository data fully upgraded in a temporary repository
marking source repository as being upgraded; clients will be unable to read from repository marking source repository as being upgraded; clients will be unable to read from repository
starting in-place swap of repository data starting in-place swap of repository data
replaced files will be backed up at $TESTTMP/upgradegd/.hg/upgradebackup.* (glob)
replacing store... replacing store...
store replacement complete; repository was inconsistent for *s (glob) store replacement complete; repository was inconsistent for *s (glob)
finalizing requirements file and making repository readable again finalizing requirements file and making repository readable again
removing old repository content $TESTTMP/upgradegd/.hg/upgradebackup.* (glob)
removing temporary repository $TESTTMP/upgradegd/.hg/upgrade.* (glob) removing temporary repository $TESTTMP/upgradegd/.hg/upgrade.* (glob)
Check that the repo still works fine Check that the repo still works fine
$ hg log -G --stat $ hg log -G --stat
@ changeset: 2:76d4395f5413 (no-py3 !) @ changeset: 2:76d4395f5413 (no-py3 !)
@ changeset: 2:fca376863211 (py3 !) @ changeset: 2:fca376863211 (py3 !)
| tag: tip | tag: tip
migrating changelog containing 3 revisions (394 bytes in store; 199 bytes tracked data) migrating changelog containing 3 revisions (394 bytes in store; 199 bytes tracked data)
cloning 3 revisions from 00changelog.i cloning 3 revisions from 00changelog.i
finished migrating 3 changelog revisions; change in size: 0 bytes finished migrating 3 changelog revisions; change in size: 0 bytes
finished migrating 9 total revisions; total change in store size: 0 bytes finished migrating 9 total revisions; total change in store size: 0 bytes
copying phaseroots copying phaseroots
data fully upgraded in a temporary repository data fully upgraded in a temporary repository
marking source repository as being upgraded; clients will be unable to read from repository marking source repository as being upgraded; clients will be unable to read from repository
starting in-place swap of repository data starting in-place swap of repository data
replaced files will be backed up at $TESTTMP/upgradegd/.hg/upgradebackup.* (glob)
replacing store... replacing store...
store replacement complete; repository was inconsistent for *s (glob) store replacement complete; repository was inconsistent for *s (glob)
finalizing requirements file and making repository readable again finalizing requirements file and making repository readable again
removing old repository content $TESTTMP/upgradegd/.hg/upgradebackup.* (glob)
removing temporary repository $TESTTMP/upgradegd/.hg/upgrade.* (glob) removing temporary repository $TESTTMP/upgradegd/.hg/upgrade.* (glob)
$ hg verify $ hg verify
checking changesets checking changesets
checking manifests checking manifests
crosschecking files in changesets and manifests crosschecking files in changesets and manifests
checking files checking files
checked 3 changesets with 3 changes to 3 files checked 3 changesets with 3 changes to 3 files
migrating changelog containing 3 revisions (394 bytes in store; 199 bytes tracked data) migrating changelog containing 3 revisions (394 bytes in store; 199 bytes tracked data)
cloning 3 revisions from 00changelog.i cloning 3 revisions from 00changelog.i
finished migrating 3 changelog revisions; change in size: 0 bytes finished migrating 3 changelog revisions; change in size: 0 bytes
finished migrating 9 total revisions; total change in store size: 0 bytes finished migrating 9 total revisions; total change in store size: 0 bytes
copying phaseroots copying phaseroots
data fully upgraded in a temporary repository data fully upgraded in a temporary repository
marking source repository as being upgraded; clients will be unable to read from repository marking source repository as being upgraded; clients will be unable to read from repository
starting in-place swap of repository data starting in-place swap of repository data
replaced files will be backed up at $TESTTMP/upgradegd/.hg/upgradebackup.* (glob)
replacing store... replacing store...
store replacement complete; repository was inconsistent for *s (glob) store replacement complete; repository was inconsistent for *s (glob)
finalizing requirements file and making repository readable again finalizing requirements file and making repository readable again
removing old repository content $TESTTMP/upgradegd/.hg/upgradebackup.* (glob)
removing temporary repository $TESTTMP/upgradegd/.hg/upgrade.* (glob) removing temporary repository $TESTTMP/upgradegd/.hg/upgrade.* (glob)
$ hg verify $ hg verify
checking changesets checking changesets
checking manifests checking manifests
crosschecking files in changesets and manifests crosschecking files in changesets and manifests
checking files checking files
checked 3 changesets with 3 changes to 3 files checked 3 changesets with 3 changes to 3 files
migrating changelog containing 3 revisions (394 bytes in store; 199 bytes tracked data) migrating changelog containing 3 revisions (394 bytes in store; 199 bytes tracked data)
blindly copying 00changelog.i containing 3 revisions blindly copying 00changelog.i containing 3 revisions
finished migrating 3 changelog revisions; change in size: 0 bytes finished migrating 3 changelog revisions; change in size: 0 bytes
finished migrating 9 total revisions; total change in store size: 0 bytes finished migrating 9 total revisions; total change in store size: 0 bytes
copying phaseroots copying phaseroots
data fully upgraded in a temporary repository data fully upgraded in a temporary repository
marking source repository as being upgraded; clients will be unable to read from repository marking source repository as being upgraded; clients will be unable to read from repository
starting in-place swap of repository data starting in-place swap of repository data
replaced files will be backed up at $TESTTMP/upgradegd/.hg/upgradebackup.* (glob)
replacing store... replacing store...
store replacement complete; repository was inconsistent for *s (glob) store replacement complete; repository was inconsistent for *s (glob)
finalizing requirements file and making repository readable again finalizing requirements file and making repository readable again
removing old repository content $TESTTMP/upgradegd/.hg/upgradebackup.* (glob)
removing temporary repository $TESTTMP/upgradegd/.hg/upgrade.* (glob) removing temporary repository $TESTTMP/upgradegd/.hg/upgrade.* (glob)
$ hg verify $ hg verify
checking changesets checking changesets
checking manifests checking manifests
crosschecking files in changesets and manifests crosschecking files in changesets and manifests
checking files checking files
checked 3 changesets with 3 changes to 3 files checked 3 changesets with 3 changes to 3 files
migrating changelog containing 3 revisions (394 bytes in store; 199 bytes tracked data) migrating changelog containing 3 revisions (394 bytes in store; 199 bytes tracked data)
cloning 3 revisions from 00changelog.i cloning 3 revisions from 00changelog.i
finished migrating 3 changelog revisions; change in size: 0 bytes finished migrating 3 changelog revisions; change in size: 0 bytes
finished migrating 9 total revisions; total change in store size: 0 bytes finished migrating 9 total revisions; total change in store size: 0 bytes
copying phaseroots copying phaseroots
data fully upgraded in a temporary repository data fully upgraded in a temporary repository
marking source repository as being upgraded; clients will be unable to read from repository marking source repository as being upgraded; clients will be unable to read from repository
starting in-place swap of repository data starting in-place swap of repository data
replaced files will be backed up at $TESTTMP/upgradegd/.hg/upgradebackup.* (glob)
replacing store... replacing store...
store replacement complete; repository was inconsistent for *s (glob) store replacement complete; repository was inconsistent for *s (glob)
finalizing requirements file and making repository readable again finalizing requirements file and making repository readable again
removing old repository content $TESTTMP/upgradegd/.hg/upgradebackup.* (glob)
removing temporary repository $TESTTMP/upgradegd/.hg/upgrade.* (glob) removing temporary repository $TESTTMP/upgradegd/.hg/upgrade.* (glob)
$ hg verify $ hg verify
checking changesets checking changesets
checking manifests checking manifests
crosschecking files in changesets and manifests crosschecking files in changesets and manifests
checking files checking files
checked 3 changesets with 3 changes to 3 files checked 3 changesets with 3 changes to 3 files
migrating changelog containing 3 revisions (394 bytes in store; 199 bytes tracked data) migrating changelog containing 3 revisions (394 bytes in store; 199 bytes tracked data)
cloning 3 revisions from 00changelog.i cloning 3 revisions from 00changelog.i
finished migrating 3 changelog revisions; change in size: 0 bytes finished migrating 3 changelog revisions; change in size: 0 bytes
finished migrating 9 total revisions; total change in store size: 0 bytes finished migrating 9 total revisions; total change in store size: 0 bytes
copying phaseroots copying phaseroots
data fully upgraded in a temporary repository data fully upgraded in a temporary repository
marking source repository as being upgraded; clients will be unable to read from repository marking source repository as being upgraded; clients will be unable to read from repository
starting in-place swap of repository data starting in-place swap of repository data
replaced files will be backed up at $TESTTMP/upgradegd/.hg/upgradebackup.* (glob)
replacing store... replacing store...
store replacement complete; repository was inconsistent for *s (glob) store replacement complete; repository was inconsistent for *s (glob)
finalizing requirements file and making repository readable again finalizing requirements file and making repository readable again
removing old repository content $TESTTMP/upgradegd/.hg/upgradebackup.* (glob)
removing temporary repository $TESTTMP/upgradegd/.hg/upgrade.* (glob) removing temporary repository $TESTTMP/upgradegd/.hg/upgrade.* (glob)
$ hg verify $ hg verify
checking changesets checking changesets
checking manifests checking manifests
crosschecking files in changesets and manifests crosschecking files in changesets and manifests
checking files checking files
checked 3 changesets with 3 changes to 3 files checked 3 changesets with 3 changes to 3 files