Patchwork D5308: store: don't pass 'atomictemp=True' while appending to fncache

login
register
mail settings
Submitter phabricator
Date Nov. 27, 2018, 1:22 p.m.
Message ID <differential-rev-PHID-DREV-iexcp73yyksfjy6nhdc5-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/36817/
State New
Headers show

Comments

phabricator - Nov. 27, 2018, 1:22 p.m.
pulkit created this revision.
Herald added subscribers: mercurial-devel, mjpieters.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Appending to an atomictemp file means that the entire file is copied first.
  Thanks to Yuya for suggesting.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5308

AFFECTED FILES
  mercurial/store.py

CHANGE DETAILS




To: pulkit, #hg-reviewers
Cc: mjpieters, mercurial-devel
Yuya Nishihara - Nov. 27, 2018, 1:34 p.m.
> --- a/mercurial/store.py
> +++ b/mercurial/store.py
> @@ -486,7 +486,7 @@
>          if self.addls:
>              # if we have just new entries, let's append them to the fncache
>              tr.addbackup('fncache')
> -            fp = self.vfs('fncache', mode='ab', atomictemp=True)
> +            fp = self.vfs('fncache', mode='ab')

Ah, no. addbackup() creates hardlink, which means fncache can't be updated in
place. Also, the reader wouldn't handle partially-written fncache well.
phabricator - Nov. 27, 2018, 1:37 p.m.
yuja added a comment.


  >   - a/mercurial/store.py +++ b/mercurial/store.py @@ -486,7 +486,7 @@ if self.addls:
  >     1. if we have just new entries, let's append them to the fncache tr.addbackup('fncache')
  > - fp = self.vfs('fncache', mode='ab', atomictemp=True) +            fp = self.vfs('fncache', mode='ab')
  
  Ah, no. addbackup() creates hardlink, which means fncache can't be updated in
  place. Also, the reader wouldn't handle partially-written fncache well.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5308

To: pulkit, #hg-reviewers
Cc: yuja, mjpieters, mercurial-devel
phabricator - Nov. 27, 2018, 1:53 p.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D5308#78985, @yuja wrote:
  
  > >   - a/mercurial/store.py +++ b/mercurial/store.py @@ -486,7 +486,7 @@ if self.addls:
  > >     1. if we have just new entries, let's append them to the fncache tr.addbackup('fncache')
  > > - fp = self.vfs('fncache', mode='ab', atomictemp=True) +            fp = self.vfs('fncache', mode='ab')
  >
  > Ah, no. addbackup() creates hardlink, which means fncache can't be updated in
  >  place. Also, the reader wouldn't handle partially-written fncache well.
  
  
  do we need to call `addbackup()` if we are just appending?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5308

To: pulkit, #hg-reviewers
Cc: yuja, mjpieters, mercurial-devel
Yuya Nishihara - Nov. 27, 2018, 2:08 p.m.
On Tue, 27 Nov 2018 13:53:07 +0000, pulkit (Pulkit Goyal) wrote:
>   do we need to call `addbackup()` if we are just appending?

We need a mechanism to restore the original content on rollback. IIUC,
it's `tr.add()` for append-only files.

As I said, we'll also need to somehow hide partial data from another
reader processes. There's no read lock.
phabricator - Nov. 28, 2018, 5:06 p.m.
pulkit added a comment.


  Pasting Yuja's reply to that from mailing list:
  
  > In https://phab.mercurial-scm.org/D5308#78989, @pulkit wrote:
  >
  > > In https://phab.mercurial-scm.org/D5308#78985, @yuja wrote:
  > >
  > > > >   - a/mercurial/store.py +++ b/mercurial/store.py @@ -486,7 +486,7 @@ if self.addls:
  > > > >     1. if we have just new entries, let's append them to the fncache tr.addbackup('fncache')
  > > > > - fp = self.vfs('fncache', mode='ab', atomictemp=True) +            fp = self.vfs('fncache', mode='ab')
  > > >
  > > > Ah, no. addbackup() creates hardlink, which means fncache can't be updated in
  > > >  place. Also, the reader wouldn't handle partially-written fncache well.
  > >
  > >
  > > do we need to call `addbackup()` if we are just appending?
  >
  >
  > We need a mechanism to restore the original content on rollback. IIUC,
  >  it's `tr.add()` for append-only files.
  >
  > As I said, we'll also need to somehow hide partial data from another
  >  reader processes. There's no read lock.
  
  
  Read locks! was there a previous attempt on implementing them? I will try to take a shot at implementing them. I am unable to find any good detail except mentions in wiki or mailing list archive.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5308

To: pulkit, #hg-reviewers
Cc: yuja, mjpieters, mercurial-devel
Yuya Nishihara - Nov. 29, 2018, 11:57 a.m.
>   > As I said, we'll also need to somehow hide partial data from another
>   >  reader processes. There's no read lock.
>   
>   
>   Read locks! was there a previous attempt on implementing them? I will try to take a shot at implementing them. I am unable to find any good detail except mentions in wiki or mailing list archive.

Another option is to ignore trailing data after `\n`. That's how the revlog
parser gets around partially-written entry.

In either way, writing fncache non-atomically wouldn't be strictly compatible
with older hg clients. I think that's okayish, but I should note that.
phabricator - Nov. 29, 2018, 11:59 a.m.
yuja added a comment.


  >   > As I said, we'll also need to somehow hide partial data from another
  >   >  reader processes. There's no read lock.
  >   
  >   
  >   Read locks! was there a previous attempt on implementing them? I will try to take a shot at implementing them. I am unable to find any good detail except mentions in wiki or mailing list archive.
  
  Another option is to ignore trailing data after `\n`. That's how the revlog
  parser gets around partially-written entry.
  
  In either way, writing fncache non-atomically wouldn't be strictly compatible
  with older hg clients. I think that's okayish, but I should note that.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5308

To: pulkit, #hg-reviewers
Cc: yuja, mjpieters, mercurial-devel

Patch

diff --git a/mercurial/store.py b/mercurial/store.py
--- a/mercurial/store.py
+++ b/mercurial/store.py
@@ -486,7 +486,7 @@ 
         if self.addls:
             # if we have just new entries, let's append them to the fncache
             tr.addbackup('fncache')
-            fp = self.vfs('fncache', mode='ab', atomictemp=True)
+            fp = self.vfs('fncache', mode='ab')
             if self.addls:
                 fp.write(encodedir('\n'.join(self.addls) + '\n'))
             fp.close()