You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Denis Kovalchuk <de...@visualsvn.com> on 2020/03/18 14:32:43 UTC
[PATCH] svnadmin build-repcache command
Hello.
I would like to suggest an implementation of ‘svnadmin build-repcache’
command
that allows to process data in the specified revision range and add missing
entries
to the representation cache. It may be useful for admins who had disabled
the
rep-sharing option or deleted the rep-cache.db file [1]. An idea of having
such
command has been previously discussed in [2].
The implementation iterates over revisions in the specified range and
recursively
processes the changed nodes, starting from the corresponding revision roots.
For each changed node, it ensures that its data and property representations
exist in the rep-cache. The nodes are processed in the same order as when
committing a transaction, so that the rep-cache.db files are fully
consistent.
If a repository does not support the rep-sharing or rep-sharing is disabled,
then do nothing.
I have attached a patch with the implementation of the command.
[1] https://svn.haxx.se/users/archive-2014-01/0128.shtml
[2] https://svn.haxx.se/dev/archive-2018-08/0050.shtml
Regards,
Denis Kovalchuk
Re: [PATCH] svnadmin build-repcache command
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Apr 01, 2020 at 10:26:40AM +0000, Daniel Shahaf wrote:
> If you haven't already, re-run that builder using subversion-bot to
> check if the bug is reproducible.
Can you please do this yourself when you find time for it?
You provide a lot of good ideas, while at the same time asking others
to implement them. That's fine, of course, it never hurts to ask.
I guess you simply do not have the time to carry out your idea and
then come back to the list and report the result.
But I don't have enough time on my hands either to carry out most of
your ideas, so please don't be offended when I refuse.
I've already done a code review to try to pin-poin the bug. I've added
the docstrings you asked for in the other thread. I'm doing the RM work.
All of this happens on the side to other stuff I have to do.
Re: [PATCH] svnadmin build-repcache command
Posted by Nathan Hartman <ha...@gmail.com>.
On Wed, Apr 1, 2020 at 3:54 PM Stefan Sperling <st...@elego.de> wrote:
> On Wed, Apr 01, 2020 at 10:50:12PM +0300, Denis Kovalchuk wrote:
> > Because the root cause of the problem was unclear to me I investigated
> it.
> >
> > svn_fs_fs__fixup_expanded_size() function exists due to issue SVN-4554
> [1].
> > In this case, fsfs-v6 repository created with older binaries contains a
> > representation with "expanded size" field written on disk as 0. The fsfs
> code
> > assumes that this field has resolved value after the fixup, but because
> of the
> > missing calls it did not and contained 0. It caused the read
> representation
> > contents to be truncated to size of 0. In this case it is a directory
> > representation
> > and so the call to svn_fs_fs__rep_contents_dir() resulted in error. The
> error is
> > a checksum mismatch error where the actual checksum is a checksum of the
> > (truncated) empty string and the expected checksum is the one recorded in
> > the revision file.
> >
> > Thanks for your help and the fix!
> >
> > [1] https://issues.apache.org/jira/browse/SVN-4554
> >
>
> Great. Thank you, too, for following up and explaining the problem!
>
> I will again wait a while to see if voting in STATUS happens over the
> next few days. My opinion is that, ideally, we should get all current
> entries in STATUS reviewed and merged, and then roll the -rc2 release.
This is great news! Thank you and kudos to stsp for fixing the issue and to
Denis for tracking it down and explaining it.
Just FYI, I intend to vote soon, after I do a little bit more testing.
Thanks again to all,
Nathan
Re: [PATCH] svnadmin build-repcache command
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Apr 01, 2020 at 10:50:12PM +0300, Denis Kovalchuk wrote:
> > > This seems to fix it.
> > >
> > > I don't understand why this is necessary but the regular code path for
> > > access to node revision data in fsfs also seems to apply this always.
> > >
> > > Can an fsfs expert confirm?
> >
> > Since this fix looks reasonable to me and we're trying to move the release
> > process forward, I have decided to commit the change and have added it to
> > the backport nomination as well.
>
> I had the same diff in my working copy and can confirm it fixes the problem.
>
> Because the root cause of the problem was unclear to me I investigated it.
>
> svn_fs_fs__fixup_expanded_size() function exists due to issue SVN-4554 [1].
> In this case, fsfs-v6 repository created with older binaries contains a
> representation with "expanded size" field written on disk as 0. The fsfs code
> assumes that this field has resolved value after the fixup, but because of the
> missing calls it did not and contained 0. It caused the read representation
> contents to be truncated to size of 0. In this case it is a directory
> representation
> and so the call to svn_fs_fs__rep_contents_dir() resulted in error. The error is
> a checksum mismatch error where the actual checksum is a checksum of the
> (truncated) empty string and the expected checksum is the one recorded in
> the revision file.
>
> Thanks for your help and the fix!
>
> [1] https://issues.apache.org/jira/browse/SVN-4554
>
Great. Thank you, too, for following up and explaining the problem!
I will again wait a while to see if voting in STATUS happens over the
next few days. My opinion is that, ideally, we should get all current
entries in STATUS reviewed and merged, and then roll the -rc2 release.
Re: [PATCH] svnadmin build-repcache command
Posted by Denis Kovalchuk <de...@visualsvn.com>.
> > This seems to fix it.
> >
> > I don't understand why this is necessary but the regular code path for
> > access to node revision data in fsfs also seems to apply this always.
> >
> > Can an fsfs expert confirm?
>
> Since this fix looks reasonable to me and we're trying to move the release
> process forward, I have decided to commit the change and have added it to
> the backport nomination as well.
I had the same diff in my working copy and can confirm it fixes the problem.
Because the root cause of the problem was unclear to me I investigated it.
svn_fs_fs__fixup_expanded_size() function exists due to issue SVN-4554 [1].
In this case, fsfs-v6 repository created with older binaries contains a
representation with "expanded size" field written on disk as 0. The fsfs code
assumes that this field has resolved value after the fixup, but because of the
missing calls it did not and contained 0. It caused the read representation
contents to be truncated to size of 0. In this case it is a directory
representation
and so the call to svn_fs_fs__rep_contents_dir() resulted in error. The error is
a checksum mismatch error where the actual checksum is a checksum of the
(truncated) empty string and the expected checksum is the one recorded in
the revision file.
Thanks for your help and the fix!
[1] https://issues.apache.org/jira/browse/SVN-4554
Re: [PATCH] svnadmin build-repcache command
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Apr 01, 2020 at 06:40:27PM +0200, Stefan Sperling wrote:
> This seems to fix it.
>
> I don't understand why this is necessary but the regular code path for
> access to node revision data in fsfs also seems to apply this always.
>
> Can an fsfs expert confirm?
Since this fix looks reasonable to me and we're trying to move the release
process forward, I have decided to commit the change and have added it to
the backport nomination as well.
> Index: subversion/libsvn_fs_fs/fs_fs.c
> ===================================================================
> --- subversion/libsvn_fs_fs/fs_fs.c (revision 1875921)
> +++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
> @@ -2393,6 +2393,10 @@ reindex_node(svn_fs_t *fs,
> SVN_ERR(svn_fs_fs__read_noderev(&noderev, rev_file->stream,
> pool, pool));
>
> + /* Make sure EXPANDED_SIZE has the correct value for every rep. */
> + SVN_ERR(svn_fs_fs__fixup_expanded_size(fs, noderev->data_rep, pool));
> + SVN_ERR(svn_fs_fs__fixup_expanded_size(fs, noderev->prop_rep, pool));
> +
> /* First reindex sub-directory to match write_final_rev() behavior. */
> if (noderev->kind == svn_node_dir)
> {
>
>
Re: [PATCH] svnadmin build-repcache command
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Apr 01, 2020 at 06:25:12PM +0200, Stefan Sperling wrote:
> On Wed, Apr 01, 2020 at 11:45:04AM -0400, Nathan Hartman wrote:
> > On Wed, Apr 1, 2020 at 8:05 AM Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > > The failure is 100% reproducible with «./svnadmin_tests.py
> > > --fs-type=fsfs --fsfs-version=6 74» on Linux.
> >
> > I'm experimenting with this also. So far I have nothing definitive but
> > I can confirm that it is 100% reproducible on Linux with the above
> > invocation.
>
> Also on OpenBSD. Thanks Daniel for identifying a way to reproduce it :)
This seems to fix it.
I don't understand why this is necessary but the regular code path for
access to node revision data in fsfs also seems to apply this always.
Can an fsfs expert confirm?
Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c (revision 1875921)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -2393,6 +2393,10 @@ reindex_node(svn_fs_t *fs,
SVN_ERR(svn_fs_fs__read_noderev(&noderev, rev_file->stream,
pool, pool));
+ /* Make sure EXPANDED_SIZE has the correct value for every rep. */
+ SVN_ERR(svn_fs_fs__fixup_expanded_size(fs, noderev->data_rep, pool));
+ SVN_ERR(svn_fs_fs__fixup_expanded_size(fs, noderev->prop_rep, pool));
+
/* First reindex sub-directory to match write_final_rev() behavior. */
if (noderev->kind == svn_node_dir)
{
Re: [PATCH] svnadmin build-repcache command
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Apr 01, 2020 at 11:45:04AM -0400, Nathan Hartman wrote:
> On Wed, Apr 1, 2020 at 8:05 AM Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > The failure is 100% reproducible with «./svnadmin_tests.py
> > --fs-type=fsfs --fsfs-version=6 74» on Linux.
>
> I'm experimenting with this also. So far I have nothing definitive but
> I can confirm that it is 100% reproducible on Linux with the above
> invocation.
Also on OpenBSD. Thanks Daniel for identifying a way to reproduce it :)
Re: [PATCH] svnadmin build-repcache command
Posted by Nathan Hartman <ha...@gmail.com>.
On Wed, Apr 1, 2020 at 8:05 AM Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> The failure is 100% reproducible with «./svnadmin_tests.py
> --fs-type=fsfs --fsfs-version=6 74» on Linux.
I'm experimenting with this also. So far I have nothing definitive but
I can confirm that it is 100% reproducible on Linux with the above
invocation. I tried removing r1875918 just to see whether the "INSERT
OR IGNORE" optimization was contributing something, but the result is
the same with or without that. I find it odd that this error occurs
while building the rep cache (as opposed to an error where the built
rep cache is different than the original one).
I have to do some other things now but I'll continue looking at this
some more later.
Thanks,
Nathan
Re: [PATCH] svnadmin build-repcache command
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Wed, 01 Apr 2020 10:26 +0000:
> Stefan Sperling wrote on Wed, 01 Apr 2020 12:18 +0200:
> > On Wed, Apr 01, 2020 at 09:59:41AM +0000, Daniel Shahaf wrote:
> > > Stefan Sperling wrote on Wed, 01 Apr 2020 11:13 +0200:
> > > > https://ci.apache.org/builders/svn-x64-macosx-fsfs-v6/builds/2941/steps/Test%20ra_local%2Bfsfs-v6/logs/faillog
> > > >
> > > > The svnadmin build-repcache test runs into a checksum mismatch.
> > > >
> > > > Any ideas?
> > >
> > > Note the "actual" checksum is the checksum of the empty string.
> > >
> >
> > Oh, interesting point :)
> >
> > I cannot see what's wrong. An empty stream is returned from
> > svn_fs_fs__get_contents() if the passed in rep pointer is NULL,
> > but I don't think that's where the bug is.
> >
> > Perhaps the failing test really read empty rep content for some reason?
> > I'm now running this test in a loop, but I could not reproduce the
> > failure yet.
>
> Are you running your local tests using the same configuration the
> builder uses? (Even without looking up the build script, the builder's
> name suggests it uses FSFS f6, i.e., --server-minor-version.)
>
The failure is 100% reproducible with «./svnadmin_tests.py
--fs-type=fsfs --fsfs-version=6 74» on Linux.
Let's wait to hear from Denis.
> If you haven't already, re-run that builder using subversion-bot to
> check if the bug is reproducible.
>
> The error trace goes through cache.c; SVN_X_DOES_NOT_MARK_THE_SPOT in
> notes/knobs can be used to rule out svn_cache__t as the cause.
Re: [PATCH] svnadmin build-repcache command
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Wed, 01 Apr 2020 12:18 +0200:
> On Wed, Apr 01, 2020 at 09:59:41AM +0000, Daniel Shahaf wrote:
> > Stefan Sperling wrote on Wed, 01 Apr 2020 11:13 +0200:
> > > https://ci.apache.org/builders/svn-x64-macosx-fsfs-v6/builds/2941/steps/Test%20ra_local%2Bfsfs-v6/logs/faillog
> > >
> > > The svnadmin build-repcache test runs into a checksum mismatch.
> > >
> > > Any ideas?
> >
> > Note the "actual" checksum is the checksum of the empty string.
> >
>
> Oh, interesting point :)
>
> I cannot see what's wrong. An empty stream is returned from
> svn_fs_fs__get_contents() if the passed in rep pointer is NULL,
> but I don't think that's where the bug is.
>
> Perhaps the failing test really read empty rep content for some reason?
> I'm now running this test in a loop, but I could not reproduce the
> failure yet.
Are you running your local tests using the same configuration the
builder uses? (Even without looking up the build script, the builder's
name suggests it uses FSFS f6, i.e., --server-minor-version.)
If you haven't already, re-run that builder using subversion-bot to
check if the bug is reproducible.
The error trace goes through cache.c; SVN_X_DOES_NOT_MARK_THE_SPOT in
notes/knobs can be used to rule out svn_cache__t as the cause.
Re: [PATCH] svnadmin build-repcache command
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Apr 01, 2020 at 09:59:41AM +0000, Daniel Shahaf wrote:
> Stefan Sperling wrote on Wed, 01 Apr 2020 11:13 +0200:
> > https://ci.apache.org/builders/svn-x64-macosx-fsfs-v6/builds/2941/steps/Test%20ra_local%2Bfsfs-v6/logs/faillog
> >
> > The svnadmin build-repcache test runs into a checksum mismatch.
> >
> > Any ideas?
>
> Note the "actual" checksum is the checksum of the empty string.
>
Oh, interesting point :)
I cannot see what's wrong. An empty stream is returned from
svn_fs_fs__get_contents() if the passed in rep pointer is NULL,
but I don't think that's where the bug is.
Perhaps the failing test really read empty rep content for some reason?
I'm now running this test in a loop, but I could not reproduce the
failure yet.
Re: [PATCH] svnadmin build-repcache command
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Wed, 01 Apr 2020 11:13 +0200:
> https://ci.apache.org/builders/svn-x64-macosx-fsfs-v6/builds/2941/steps/Test%20ra_local%2Bfsfs-v6/logs/faillog
>
> The svnadmin build-repcache test runs into a checksum mismatch.
>
> Any ideas?
Note the "actual" checksum is the checksum of the empty string.
Re: [PATCH] svnadmin build-repcache command
Posted by Denis Kovalchuk <de...@visualsvn.com>.
> There's another test failure related to this patch.
>
> This time on Mac OS X:
> https://ci.apache.org/builders/svn-x64-macosx-fsfs-v6/builds/2941/steps/Test%20ra_local%2Bfsfs-v6/logs/faillog
>
> The svnadmin build-repcache test runs into a checksum mismatch.
>
> Any ideas?
I started to handle this problem and plan to report the result.
Regards,
Denis Kovalchuk
Re: [PATCH] svnadmin build-repcache command
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Mar 31, 2020 at 12:48:21PM +0200, Stefan Sperling wrote:
> On Tue, Mar 31, 2020 at 01:35:54PM +0300, Denis Kovalchuk wrote:
> > > This has caused a test failure on the windows buildbot:
> > >
> > > https://ci.apache.org/builders/svn-windows-local/builds/3392/steps/Test%20fsfs%2Blocal/logs/faillog
> > >
> > > Can you please check what we should do about that?
> >
> > I missed SkipUnless test decorator. The build_repcache() test should be skipped
> > if read_rep_cache() is not supported by Python-SQLite version.
> >
> > I have attached a patch that fixes this.
>
> Thank you for the quick fix. Applied in r1875925.
There's another test failure related to this patch.
This time on Mac OS X:
https://ci.apache.org/builders/svn-x64-macosx-fsfs-v6/builds/2941/steps/Test%20ra_local%2Bfsfs-v6/logs/faillog
The svnadmin build-repcache test runs into a checksum mismatch.
Any ideas?
Re: [PATCH] svnadmin build-repcache command
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Mar 31, 2020 at 01:35:54PM +0300, Denis Kovalchuk wrote:
> > This has caused a test failure on the windows buildbot:
> >
> > https://ci.apache.org/builders/svn-windows-local/builds/3392/steps/Test%20fsfs%2Blocal/logs/faillog
> >
> > Can you please check what we should do about that?
>
> I missed SkipUnless test decorator. The build_repcache() test should be skipped
> if read_rep_cache() is not supported by Python-SQLite version.
>
> I have attached a patch that fixes this.
Thank you for the quick fix. Applied in r1875925.
Re: [PATCH] svnadmin build-repcache command
Posted by Denis Kovalchuk <de...@visualsvn.com>.
> This has caused a test failure on the windows buildbot:
>
> https://ci.apache.org/builders/svn-windows-local/builds/3392/steps/Test%20fsfs%2Blocal/logs/faillog
>
> Can you please check what we should do about that?
I missed SkipUnless test decorator. The build_repcache() test should be skipped
if read_rep_cache() is not supported by Python-SQLite version.
I have attached a patch that fixes this.
Regards,
Denis Kovalchuk
Re: [PATCH] svnadmin build-repcache command
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Mar 31, 2020 at 10:55:07AM +0200, Stefan Sperling wrote:
> On Tue, Mar 24, 2020 at 07:53:21PM +0300, Denis Kovalchuk wrote:
> > Introduce 'svnadmin build-repcache' command.
>
> Committed in r1875921. Thanks!
This has caused a test failure on the windows buildbot:
https://ci.apache.org/builders/svn-windows-local/builds/3392/steps/Test%20fsfs%2Blocal/logs/faillog
Can you please check what we should do about that?
Re: [PATCH] svnadmin build-repcache command
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Mar 24, 2020 at 07:53:21PM +0300, Denis Kovalchuk wrote:
> Introduce 'svnadmin build-repcache' command.
Committed in r1875921. Thanks!
I will now nominate both patches for backport to 1.14.x.
Re: [PATCH] svnadmin build-repcache command
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
> > I'm proposing to modify fs_has_rep_sharing() to return True when running
> > on FSFS with rep-sharing enabled and to return False on other backends.
> > That's separate from your test's decorators.
> >
>
> I think that this might be unrelated to this patch, but also I may be
> misunderstanding
> what exactly you propose. If we modify fs_has_rep_sharing() to return True
> for FSFS and to return False for other backends, we will have a problem
> with FSX,
> which also supports rep-sharing.
The function, as it stands, assumes that the tests run on FSFS, and
will return wrong values when that assumption doesn't hold. The point
is to make that function return the correct value in every configuration.
If the fix I proposed is wrong under FSX, then the issue should be fixed
differently.
Re: [PATCH] svnadmin build-repcache command
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Denis Kovalchuk wrote on Mon, 30 Mar 2020 23:48 +0300:
> Speaking of the possible starvations, I would like to note, that since
> the current 'build-repcache' implementation uses one SQL transaction
> per revision, it seems to me that it should behave similarly to
> concurrent commits, where a rep-cache.db is updated in the post commit
> processing stage.
Agreed.
Cheers,
Daniel
Re: [PATCH] svnadmin build-repcache command
Posted by Denis Kovalchuk <de...@visualsvn.com>.
>
> Denis, would you like to look into this? It looks like an easy,
> localized fix, and your build-repcache patch will benefit from it, too.
Thank you for the suggestion. I'll keep it in mind, but as I already have
two outgoing
patches, I would rather try not to add one more just now, to keep things
focused.
Speaking of the possible starvations, I would like to note, that since the
current 'build-repcache'
implementation uses one SQL transaction per revision, it seems to me that
it should behave similarly
to concurrent commits, where a rep-cache.db is updated in the post commit
processing stage.
Regards,
Denis Kovalchuk
Re: [PATCH] svnadmin build-repcache command
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Sat, 28 Mar 2020 21:54 +0000:
> Stefan Sperling wrote on Sat, 28 Mar 2020 11:32 +0100:
> > On Fri, Mar 27, 2020 at 10:58:48PM +0000, Daniel Shahaf wrote:
> > > Well, I guess the documentation, once written, should point out that if
> > > any single revision build-repcache operates on adds a large number of
> > > reps, commits made in parallel to the build-repcache run will be starved
> > > out of adding their own rep-cache entries, which will manifest as
> > > "post-commit FS processing failed". (We have a bug filed for this, but
> > > I can't find its number right now, sorry.)
> >
> > Good point.
> >
> > Maybe the command's help output should mention that the repository will
> > be blocked for commits while this command is running? Even if that isn't
> > entirely true at the implementation level it would be good user-facing
> > advice.
>
> I don't think inaccuracies in the documentation are a good idea. The
> proposal might discourage people from running this when they could;
> might lead people to run this in order to trigger the side effect of
> blocking commits for a short while, only to find commits did happen
> successfully; and might get people to distrust our documentation in
> general.
>
> Therefore, I'd document the semantics accurately. If need be, we can
> refer to fuller documentation in the release notes and/or the book.
By the way, couldn't we just fix this, rather than document it?
Looking at write_reps_to_cache(), it won't be that hard to rewrite it
to INSERT into rep-cache in batches of N reps per SQLite transaction,
rather than one SQLite transaction per commit. This'll fix the bug
I mentioned.
Makes sense?
Denis, would you like to look into this? It looks like an easy,
localized fix, and your build-repcache patch will benefit from it, too.
Cheers,
Daniel
Re: [PATCH] svnadmin build-repcache command
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Sat, 28 Mar 2020 11:32 +0100:
> On Fri, Mar 27, 2020 at 10:58:48PM +0000, Daniel Shahaf wrote:
> > Stefan Sperling wrote on Fri, 27 Mar 2020 14:41 +0100:
> > > On Thu, Mar 26, 2020 at 08:32:38PM +0000, Daniel Shahaf wrote:
> > > > What I'm worried about is that we'll apply the "backwards compatibility
> > > > until 2.0" promise to something we shouldn't. As I said, issues of
> > > > this nature can't generally be found by tests; they can only be found
> > > > by code review.
> > >
> > > I'm not worried about that. After reading through the patch I don't see
> > > any reason why we would not want to commit to supporting this feature.
> > >
> >
> > I wasn't discussing the build-repcache patch specifically; I was
> > commenting in general terms about the risks of rushing patch review
> > processes lest the patches in question miss a minor release train.
> > "We'll just fix it in a patch release" is the right attitude, but
> > doesn't always apply.
>
> Ah, I see. That wasn't clear to me.
Well, I'm glad there is still one mailing list where people can sort out
disagreements without resorting to pyromania. :-)
> > Well, I guess the documentation, once written, should point out that if
> > any single revision build-repcache operates on adds a large number of
> > reps, commits made in parallel to the build-repcache run will be starved
> > out of adding their own rep-cache entries, which will manifest as
> > "post-commit FS processing failed". (We have a bug filed for this, but
> > I can't find its number right now, sorry.)
>
> Good point.
>
> Maybe the command's help output should mention that the repository will
> be blocked for commits while this command is running? Even if that isn't
> entirely true at the implementation level it would be good user-facing
> advice.
I don't think inaccuracies in the documentation are a good idea. The
proposal might discourage people from running this when they could;
might lead people to run this in order to trigger the side effect of
blocking commits for a short while, only to find commits did happen
successfully; and might get people to distrust our documentation in
general.
Therefore, I'd document the semantics accurately. If need be, we can
refer to fuller documentation in the release notes and/or the book.
> > > I know of at least two cases where rep-sharing was temporarily or perhaps
> > > even permanently disabled after SHA1 collisions were committed (one case
> > > is webkit, I learned about another case via elego's SVN support).
> > > These repositories are now growing faster than they would if rep-caching
> > > could be re-enabled easily.
> >
> > I don't follow. There's nothing stopping the admins of those
> > repositories from reënabling rep-cache as soon as they upgrade to
> > a Subversion release that has the sha1 collision fixes. That will solve
> > the growth problem going forward. Compared to this, the benefits of
> > running build-repcache on those repositories are limited.
>
> Ah, yes. You're right about that.
>
> I suspect some might have left the rep-cache disabled anyway cause
> of (unwarranted) trust issues.
>
> Apart from the above, there's also a small possibility that a rep-cache DB
> becomes corrupt or somehow unusable by sqlite. In such cases this command
> would be very useful.
+1
Come to think of it, mightn't it also be useful in plain «svnadmin load»
use-cases? Disable rep-cache, run «svnadmin load», then after it's done
go back and populate the rep-cache. Or even run «svnadmin load
--use-post-commit-hook» with a post-commit hook that forks off an
«svnadmin build-rep -r $REV» process, to have rep-cache population happen
in a separate thread.
Setting up a cross-minor-version over-the-wire interoperability
buildbot (was: Re: [PATCH] svnadmin build-repcache command)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Sat, 28 Mar 2020 11:32 +0100:
> On Fri, Mar 27, 2020 at 10:58:48PM +0000, Daniel Shahaf wrote:
> > This reminds me that we don't have good test coverage for on-disk and
> > over-the-wire interoperability with older versions (other than
> > upgrade_tests_data/, which is wc-only). This would be a good thing to
> > set up some day…
>
> Yes, I totally agree!
>
> It would be great to have better automated test coverage for this.
> Even if it was just one or more buildbots that ran our tests with
> various permutations of supported server/client versions.
Why don't we set this up, then? Your unix-build distro is exactly what we
need to get this off the ground: we should be able to use it to easily
compile and install multiple old versions to different prefixes. We'll
then be able to use «make check URL=…» to test svn 1.A and svnadmin 1.A
against svnserve 1.B or mod_dav_svn 1.B. (I wonder if svnadmin 1.B
shouldn't be in the picture, but we can worry about this later.)
Volunteers, please?
We can prototype the setup on svn-qavm.a.o, if needed, before making it
a proper buildbot. (That'll accelerate the edit-compile-fix cycle.)
> There's a note in our release preparation guidelines that recommends
> that the RM should do some testing like that before rolling a release.
> I think asking RMs to do this manually is asking too much.
+1
This is no small task, and the RM has enough on their plate as it is.
It might be reasonable to ask the RM to _orchestrate_ testing (in order
to ensure that a variety of permutations get covered), but that's not
much more work than setting up a wiki page for recording the permutations
that have been covered.
Cheers,
Daniel
Re: [PATCH] svnadmin build-repcache command
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Mar 27, 2020 at 10:58:48PM +0000, Daniel Shahaf wrote:
> Stefan Sperling wrote on Fri, 27 Mar 2020 14:41 +0100:
> > On Thu, Mar 26, 2020 at 08:32:38PM +0000, Daniel Shahaf wrote:
> > > What I'm worried about is that we'll apply the "backwards compatibility
> > > until 2.0" promise to something we shouldn't. As I said, issues of
> > > this nature can't generally be found by tests; they can only be found
> > > by code review.
> >
> > I'm not worried about that. After reading through the patch I don't see
> > any reason why we would not want to commit to supporting this feature.
> >
>
> I wasn't discussing the build-repcache patch specifically; I was
> commenting in general terms about the risks of rushing patch review
> processes lest the patches in question miss a minor release train.
> "We'll just fix it in a patch release" is the right attitude, but
> doesn't always apply.
Ah, I see. That wasn't clear to me.
> > Do you have concrete concerns? I can't see any downsides.
> > So far, I believe this new subcommand will be very useful.
> >
>
> No, I don't. What concerns I had I already raised upthread.
OK, then we're clear :) Thanks, I wasn't sure if you still had concerns
about the patch itself or not.
> Well, I guess the documentation, once written, should point out that if
> any single revision build-repcache operates on adds a large number of
> reps, commits made in parallel to the build-repcache run will be starved
> out of adding their own rep-cache entries, which will manifest as
> "post-commit FS processing failed". (We have a bug filed for this, but
> I can't find its number right now, sorry.)
Good point.
Maybe the command's help output should mention that the repository will
be blocked for commits while this command is running? Even if that isn't
entirely true at the implementation level it would be good user-facing
advice.
> > The only existing alternative is a full dump/load which is expensive
> > to perform on large repositories.
> > I've seen several cases in recent years were servers were upgraded but
> > admins decided against doing a full dump/load cycle of repositories
> > sized in the GB/TB range because the perceived benefits didn't justify
> > the extra effort.
> >
>
> *nod*
>
> This reminds me that we don't have good test coverage for on-disk and
> over-the-wire interoperability with older versions (other than
> upgrade_tests_data/, which is wc-only). This would be a good thing to
> set up some day…
Yes, I totally agree!
It would be great to have better automated test coverage for this.
Even if it was just one or more buildbots that ran our tests with
various permutations of supported server/client versions.
There's a note in our release preparation guidelines that recommends
that the RM should do some testing like that before rolling a release.
I think asking RMs to do this manually is asking too much.
> > I know of at least two cases where rep-sharing was temporarily or perhaps
> > even permanently disabled after SHA1 collisions were committed (one case
> > is webkit, I learned about another case via elego's SVN support).
> > These repositories are now growing faster than they would if rep-caching
> > could be re-enabled easily.
>
> I don't follow. There's nothing stopping the admins of those
> repositories from reënabling rep-cache as soon as they upgrade to
> a Subversion release that has the sha1 collision fixes. That will solve
> the growth problem going forward. Compared to this, the benefits of
> running build-repcache on those repositories are limited.
Ah, yes. You're right about that.
I suspect some might have left the rep-cache disabled anyway cause
of (unwarranted) trust issues.
Apart from the above, there's also a small possibility that a rep-cache DB
becomes corrupt or somehow unusable by sqlite. In such cases this command
would be very useful.
Re: [PATCH] svnadmin build-repcache command
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Fri, 27 Mar 2020 14:41 +0100:
> On Thu, Mar 26, 2020 at 08:32:38PM +0000, Daniel Shahaf wrote:
> > What I'm worried about is that we'll apply the "backwards compatibility
> > until 2.0" promise to something we shouldn't. As I said, issues of
> > this nature can't generally be found by tests; they can only be found
> > by code review.
>
> I'm not worried about that. After reading through the patch I don't see
> any reason why we would not want to commit to supporting this feature.
>
I wasn't discussing the build-repcache patch specifically; I was
commenting in general terms about the risks of rushing patch review
processes lest the patches in question miss a minor release train.
"We'll just fix it in a patch release" is the right attitude, but
doesn't always apply.
> Do you have concrete concerns? I can't see any downsides.
> So far, I believe this new subcommand will be very useful.
>
No, I don't. What concerns I had I already raised upthread.
Well, I guess the documentation, once written, should point out that if
any single revision build-repcache operates on adds a large number of
reps, commits made in parallel to the build-repcache run will be starved
out of adding their own rep-cache entries, which will manifest as
"post-commit FS processing failed". (We have a bug filed for this, but
I can't find its number right now, sorry.)
> The only existing alternative is a full dump/load which is expensive
> to perform on large repositories.
> I've seen several cases in recent years were servers were upgraded but
> admins decided against doing a full dump/load cycle of repositories
> sized in the GB/TB range because the perceived benefits didn't justify
> the extra effort.
>
*nod*
This reminds me that we don't have good test coverage for on-disk and
over-the-wire interoperability with older versions (other than
upgrade_tests_data/, which is wc-only). This would be a good thing to
set up some day…
> I know of at least two cases where rep-sharing was temporarily or perhaps
> even permanently disabled after SHA1 collisions were committed (one case
> is webkit, I learned about another case via elego's SVN support).
> These repositories are now growing faster than they would if rep-caching
> could be re-enabled easily.
I don't follow. There's nothing stopping the admins of those
repositories from reënabling rep-cache as soon as they upgrade to
a Subversion release that has the sha1 collision fixes. That will solve
the growth problem going forward. Compared to this, the benefits of
running build-repcache on those repositories are limited.
Cheers,
Daniel
Re: [PATCH] svnadmin build-repcache command
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Mar 26, 2020 at 08:32:38PM +0000, Daniel Shahaf wrote:
> What I'm worried about is that we'll apply the "backwards compatibility
> until 2.0" promise to something we shouldn't. As I said, issues of
> this nature can't generally be found by tests; they can only be found
> by code review.
I'm not worried about that. After reading through the patch I don't see
any reason why we would not want to commit to supporting this feature.
Do you have concrete concerns? I can't see any downsides.
So far, I believe this new subcommand will be very useful.
The only existing alternative is a full dump/load which is expensive
to perform on large repositories.
I've seen several cases in recent years were servers were upgraded but
admins decided against doing a full dump/load cycle of repositories
sized in the GB/TB range because the perceived benefits didn't justify
the extra effort.
I know of at least two cases where rep-sharing was temporarily or perhaps
even permanently disabled after SHA1 collisions were committed (one case
is webkit, I learned about another case via elego's SVN support).
These repositories are now growing faster than they would if rep-caching
could be re-enabled easily.
Re: [PATCH] svnadmin build-repcache command
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Thu, 26 Mar 2020 10:18 +0100:
> On Wed, Mar 25, 2020 at 09:56:23PM +0000, Daniel Shahaf wrote:
> > Nathan Hartman wrote on Wed, 25 Mar 2020 17:30 -0400:
> > > FYI, I tested 1.14.0-rc1 today, but I'm not rushing to sign (yet)
> > > because I'm waiting to see if we'll get Denis's patch (or a later
> > > version of it) into 1.14.
>
> I don't see any reason to wait with adding your signature.
> More below.
>
> > > I tested *without* Denis's patch, but I'll test again with it and keep
> > > the list posted.
> >
> > What kind of tests do you have in mind? Denis's patch has virtually
> > zero interaction with existing tests, so a 'make check' run wouldn't
> > teach us much, IMO: it wouldn't run the new code at all, other than the
> > newly-added test itself. On the other hand, running the new subcommand
> > on a repository as it's receiving commits would be interesting.
>
> Yes, that's what I had in mind when I said it's a low risk change.
>
> A full test run after every change is of course prudent but I doubt adding
> this patch will cause us any problems or regressions in the release itself.
>
> The patch will first have to pass through testing on trunk. It will be
> tested by our buildbots, and by Denis, and by developers handling the patch.
> And at this point in time the trunk code is virtually identical to 1.14.0.
>
> More likely, rushing a patch such as this one into a .0 release could mean
> that we'll find problems in the new feature after it is first released,
> rather than shipping it in a known-perfect state.
> But that's fine. It's what patch releases are for :)
I'm not worried about bugs in the new feature. If a bug slips past all
our layers of review and testing, we'll fix it in .1, as you say.
What I'm worried about is that we'll apply the "backwards compatibility
until 2.0" promise to something we shouldn't. As I said, issues of
this nature can't generally be found by tests; they can only be found
by code review.
Cheers,
Daniel
Re: [PATCH] svnadmin build-repcache command
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Mar 25, 2020 at 09:56:23PM +0000, Daniel Shahaf wrote:
> Nathan Hartman wrote on Wed, 25 Mar 2020 17:30 -0400:
> > FYI, I tested 1.14.0-rc1 today, but I'm not rushing to sign (yet)
> > because I'm waiting to see if we'll get Denis's patch (or a later
> > version of it) into 1.14.
I don't see any reason to wait with adding your signature.
More below.
> > I tested *without* Denis's patch, but I'll test again with it and keep
> > the list posted.
>
> What kind of tests do you have in mind? Denis's patch has virtually
> zero interaction with existing tests, so a 'make check' run wouldn't
> teach us much, IMO: it wouldn't run the new code at all, other than the
> newly-added test itself. On the other hand, running the new subcommand
> on a repository as it's receiving commits would be interesting.
Yes, that's what I had in mind when I said it's a low risk change.
A full test run after every change is of course prudent but I doubt adding
this patch will cause us any problems or regressions in the release itself.
The patch will first have to pass through testing on trunk. It will be
tested by our buildbots, and by Denis, and by developers handling the patch.
And at this point in time the trunk code is virtually identical to 1.14.0.
More likely, rushing a patch such as this one into a .0 release could mean
that we'll find problems in the new feature after it is first released,
rather than shipping it in a known-perfect state.
But that's fine. It's what patch releases are for :)
Re: [PATCH] svnadmin build-repcache command
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Nathan Hartman wrote on Wed, 25 Mar 2020 17:30 -0400:
> On Wed, Mar 25, 2020 at 9:17 AM Stefan Sperling <st...@elego.de> wrote:
> > I rolled 1.14.0-rc1 this morning, and I didn't notice that you had
> > posted this updated patch yesterday. Sorry about the bad timing.
> > The 1.14.0-rc1 release does not have your patch. But until .0 has been
> > released we can still decide to add new commands to 'svnadmin' for SVN 1.14.
> >
> > I don't think this is a de-stabilizing change and the feature is small
> > and useful. I would be in favour of adding it to 1.14.0, provided the
> > patch gets committed to trunk and nominated for backport to 1.14.0 soon.
> > And of course all our regression tests need to keep passing.
> >
> > Otherwise this change would have to wait throughout the entire 1.15
> > release cycle, at least according to our release policy guidelines.
> >
> > Would any of our committers be able to shepherd this patch through
> > without a lot of lag? Please let me know.
>
> FYI, I tested 1.14.0-rc1 today, but I'm not rushing to sign (yet)
> because I'm waiting to see if we'll get Denis's patch (or a later
> version of it) into 1.14.
>
> I tested *without* Denis's patch, but I'll test again with it and keep
> the list posted.
What kind of tests do you have in mind? Denis's patch has virtually
zero interaction with existing tests, so a 'make check' run wouldn't
teach us much, IMO: it wouldn't run the new code at all, other than the
newly-added test itself. On the other hand, running the new subcommand
on a repository as it's receiving commits would be interesting.
(Furthermore, if you're so inclined, you could review the 'structure'
file and then help review the code for issues that can be found by
review but not by testing.)
Cheers,
Daniel
Re: [PATCH] svnadmin build-repcache command
Posted by Nathan Hartman <ha...@gmail.com>.
On Wed, Mar 25, 2020 at 9:17 AM Stefan Sperling <st...@elego.de> wrote:
> I rolled 1.14.0-rc1 this morning, and I didn't notice that you had
> posted this updated patch yesterday. Sorry about the bad timing.
> The 1.14.0-rc1 release does not have your patch. But until .0 has been
> released we can still decide to add new commands to 'svnadmin' for SVN 1.14.
>
> I don't think this is a de-stabilizing change and the feature is small
> and useful. I would be in favour of adding it to 1.14.0, provided the
> patch gets committed to trunk and nominated for backport to 1.14.0 soon.
> And of course all our regression tests need to keep passing.
>
> Otherwise this change would have to wait throughout the entire 1.15
> release cycle, at least according to our release policy guidelines.
>
> Would any of our committers be able to shepherd this patch through
> without a lot of lag? Please let me know.
FYI, I tested 1.14.0-rc1 today, but I'm not rushing to sign (yet)
because I'm waiting to see if we'll get Denis's patch (or a later
version of it) into 1.14.
I tested *without* Denis's patch, but I'll test again with it and keep
the list posted.
Cheers,
Nathan
Re: [PATCH] svnadmin build-repcache command
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Mar 24, 2020 at 07:53:21PM +0300, Denis Kovalchuk wrote:
> + /** @since New in 1.14. */
> + SVN_ERRDEF(SVN_ERR_FS_REP_SHARING_NOT_ALLOWED,
> + SVN_ERR_FS_CATEGORY_START + 69,
> + "Rep-sharing is not allowed.")
> +
> + /** @since New in 1.14. */
> + SVN_ERRDEF(SVN_ERR_FS_REP_SHARING_NOT_SUPPORTED,
> + SVN_ERR_FS_CATEGORY_START + 70,
> + "Rep-sharing is not supported.")
> +
Hey Denis,
I rolled 1.14.0-rc1 this morning, and I didn't notice that you had
posted this updated patch yesterday. Sorry about the bad timing.
The 1.14.0-rc1 release does not have your patch. But until .0 has been
released we can still decide to add new commands to 'svnadmin' for SVN 1.14.
I don't think this is a de-stabilizing change and the feature is small
and useful. I would be in favour of adding it to 1.14.0, provided the
patch gets committed to trunk and nominated for backport to 1.14.0 soon.
And of course all our regression tests need to keep passing.
Otherwise this change would have to wait throughout the entire 1.15
release cycle, at least according to our release policy guidelines.
Would any of our committers be able to shepherd this patch through
without a lot of lag? Please let me know.
Denis, please also ask Evgeny and Ivan for help if nobody else has time to
handle your patch. We are a bit low on spare developer time these days.
Thanks!
Re: [PATCH] rep-cache.db insert optimization (was: Re: [PATCH]
svnadmin build-repcache command)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Denis Kovalchuk wrote on Mon, 30 Mar 2020 23:40 +0300:
> >
> > We do have an extra call when a duplicate rep is attempted to be
> > inserted into the rep-cache. When does that happen? I know it will
> > happen when called from build-repcache; what other use-cases trigger the
> > codepath you changed? Committing two files with equal contents
> > sequentially doesn't call svn_fs_fs__set_rep_reference() at all for the
> > second file.
> >
>
> I have spent some time examining when it happens and I see the following
> cases:
> 1) Commit multiple files with the same content in one revision.
> 2) During commit, svn_fs_fs__set_rep_reference() is called for a property
> representation of each node, regardless of whether it is a duplicate or
> not.
> 3) Commit a file with the same content as a property representation.
Case #2 sounds like it might benefit from optimization, for example,
when importing a tree with files that have had auto-props set by «svn add».
However, I wonder if we shouldn't optimize that case by making fewer
calls to svn_fs_fs__set_rep_reference() in the first place, as we do for
file content reps.
All your test cases involve a single commit process. I think there may
be other cases in which svn_fs_fs__set_rep_reference() will be called
that involve multiple svn_fs_t handles; for example:
.
% head -c 1024 /dev/urandom > foo
% svn import -mm foo $REPOS_URL/one
% svn import -mm foo $REPOS_URL/two &
% wait
.
if the two concurrent imports are served by separate svn_fs_t handles.
(Call this case 4.)
At any rate, it seems like svn_fs_fs__set_rep_reference() is far from
being the bottleneck of commit operations, performance-wise.
>
> > What tests is this behaviour covered by?
> >
>
> I think that:
> - Case 1) is for example covered by the rep_sharing_effectiveness() test.
> - Case 2) is implicitly covered by various tests for versioned property
> changes.
> - Case 3) is not currently covered by a specific test, but the situation
> seems to be an edge case.
>
Fair enough.
However, case 4 (commits through separate svn_fs_t handles) isn't
obviously covered.
>
> > What scenario does this bring a performance improvement in? What is the
> > improvement, quantitatively?
> >
>
> I have compared the approaches with ‘FAIL’ and ‘IGNORE’ conflict resolution
> algorithms using a synthetic benchmark. Inserting 10000 identical entries
> into
> a rep-cache.db which contains 250000 entries:
>
> 1) In the case of using the ‘FAIL’ algorithm: ~40000 microseconds.
>
> 2) In the case of using the ‘IGNORE’ algorithm: ~15000 microseconds.
>
So it takes half the time, but in absolute numbers the difference is
barely perceptible, and in comparison to how long it would have taken to
process 10000 filecontent deltas the difference is presumably negligible.
>
> Outside of the benchmark, there is ~15% improvement for the
> ‘build-repcache’ command.
Again, a 15% improvement in what use-case? Don't give us your
conclusions for us to take on faith; give us the data so we can reach
our own conclusions independently.
> Based on this, I assume that there are other commands that may benefit from
> this change.
>
Actually, I don't think this patch would be a big win for commands other
than build-repcache. No use-case other than build-repcache has been
identified where this patch would bring about a measurable difference to
the duration of the overall operation.
Even for build-repcache, you cited a 15% figure, but we don't know what
data pattern you see that on so we don't know whether that figure is
significant.
Thanks for sharing the benchmark data and adding them to the log
message.
Cheers,
Daniel
Re: [PATCH] rep-cache.db insert optimization (was: Re: [PATCH]
svnadmin build-repcache command)
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Mar 30, 2020 at 11:40:38PM +0300, Denis Kovalchuk wrote:
> rep-cache.db insert optimization.
Committed in r1875918. Thank you!
Re: [PATCH] rep-cache.db insert optimization (was: Re: [PATCH]
svnadmin build-repcache command)
Posted by Denis Kovalchuk <de...@visualsvn.com>.
>
> We do have an extra call when a duplicate rep is attempted to be
> inserted into the rep-cache. When does that happen? I know it will
> happen when called from build-repcache; what other use-cases trigger the
> codepath you changed? Committing two files with equal contents
> sequentially doesn't call svn_fs_fs__set_rep_reference() at all for the
> second file.
>
I have spent some time examining when it happens and I see the following
cases:
1) Commit multiple files with the same content in one revision.
2) During commit, svn_fs_fs__set_rep_reference() is called for a property
representation of each node, regardless of whether it is a duplicate or
not.
3) Commit a file with the same content as a property representation.
> What tests is this behaviour covered by?
>
I think that:
- Case 1) is for example covered by the rep_sharing_effectiveness() test.
- Case 2) is implicitly covered by various tests for versioned property
changes.
- Case 3) is not currently covered by a specific test, but the situation
seems to be
an edge case.
> What scenario does this bring a performance improvement in? What is the
> improvement, quantitatively?
>
I have compared the approaches with ‘FAIL’ and ‘IGNORE’ conflict resolution
algorithms using a synthetic benchmark. Inserting 10000 identical entries
into
a rep-cache.db which contains 250000 entries:
1) In the case of using the ‘FAIL’ algorithm: ~40000 microseconds.
2) In the case of using the ‘IGNORE’ algorithm: ~15000 microseconds.
Outside of the benchmark, there is ~15% improvement for the
‘build-repcache’ command.
Based on this, I assume that there are other commands that may benefit from
this change.
I have attached an updated patch.
Regards,
Denis Kovalchuk
Re: [PATCH] rep-cache.db insert optimization (was: Re: [PATCH]
svnadmin build-repcache command)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
> > Yes, INSERT OR IGNORE supported by all versions. I plan to start a separate
> > thread about this topic.
> >
>
> As mentioned above, I think we can optimize the insert statement for the
> case of a constraint violation. Now we use the 'FAIL' conflict
> resolution algorithm [1] and explicitly handle
> SVN_ERR_SQLITE_CONSTRAINT error in svn_fs_fs__set_rep_reference().
> Because of this, we have an extra svn_fs_fs__get_rep_reference() call.
We do have an extra call when a duplicate rep is attempted to be
inserted into the rep-cache. When does that happen? I know it will
happen when called from build-repcache; what other use-cases trigger the
codepath you changed? Committing two files with equal contents
sequentially doesn't call svn_fs_fs__set_rep_reference() at all for the
second file.
> As far as I know, svn_sqlite__step() in svn_sqlite__insert() is the
> only place where the error can occur. Based on this, I suggest to use
> the 'IGNORE' conflict resolution algorithm [1] and remove the
> SVN_ERR_SQLITE_CONSTRAINT handling, because the error should not occur
> now. It seems to me the change is quite safe, because the behavior is
> well covered by tests.
>
What tests is this behaviour covered by?
> rep-cache.db insert optimization.
>
> Use the 'IGNORE' conflict resolution algorithm [1] for STMT_SET_REP
> and remove SVN_ERR_SQLITE_CONSTRAINT handling, because the error
> should not occur now. It brings a slight performance improvement,
What scenario does this bring a performance improvement in? What is the
improvement, quantitatively?
> because we remove an extra svn_fs_fs__get_rep_reference() call.
>
> [1] https://www.sqlite.org/lang_conflict.html
>
> * subversion/libsvn_fs_fs/rep-cache-db.sql
> (STMT_SET_REP): Use the 'IGNORE' conflict resolution algorithm.
>
> * subversion/libsvn_fs_fs/rep-cache.c
> (svn_fs_fs__set_rep_reference): Remove SVN_ERR_SQLITE_CONSTRAINT handling.
Cheers,
Daniel
[PATCH] rep-cache.db insert optimization (was: Re: [PATCH] svnadmin
build-repcache command)
Posted by Denis Kovalchuk <de...@visualsvn.com>.
>
> Well, maybe. The code _appears_ to take a roundabout approach, but I'm
>> not sure that's actually a bug: there might be a difference between
>> «INSERT OR IGNORE» on the one hand, and the current behaviour («INSERT
>> OR FAIL» plus a C check) on the other.
>>
>> Is «INSERT OR IGNORE» supported by all SQLite versions we support?
>>
>
> Yes, INSERT OR IGNORE supported by all versions. I plan to start a separate
> thread about this topic.
>
As mentioned above, I think we can optimize the insert statement for the
case
of a constraint violation. Now we use the 'FAIL' conflict resolution
algorithm [1]
and explicitly handle SVN_ERR_SQLITE_CONSTRAINT error in
svn_fs_fs__set_rep_reference(). Because of this, we have an extra
svn_fs_fs__get_rep_reference() call.
As far as I know, svn_sqlite__step() in svn_sqlite__insert() is the only
place where
the error can occur. Based on this, I suggest to use the 'IGNORE' conflict
resolution
algorithm [1] and remove the SVN_ERR_SQLITE_CONSTRAINT handling, because
the error should not occur now. It seems to me the change is quite safe,
because
the behavior is well covered by tests.
I've attached a patch.
[1] https://www.sqlite.org/lang_conflict.html
Regards,
Denis Kovalchuk
Re: [PATCH] svnadmin build-repcache command
Posted by Denis Kovalchuk <de...@visualsvn.com>.
> Well, maybe. The code _appears_ to take a roundabout approach, but I'm
> not sure that's actually a bug: there might be a difference between
> «INSERT OR IGNORE» on the one hand, and the current behaviour («INSERT
> OR FAIL» plus a C check) on the other.
>
> Is «INSERT OR IGNORE» supported by all SQLite versions we support?
>
Yes, INSERT OR IGNORE supported by all versions. I plan to start a separate
thread about this topic.
> However, 'svnadmin pack' doesn't "silently succeed" when packing is
> disabled; it prints a warning. So the question now is whether a warning
> — i.e., an API notification — should be emitted when rep-sharing is
> supported by the library and the fs format but disabled in fsfs.conf.
> WDYT?
>
I think it makes sense to have such behavior here. I implemented it
with a special error code that svnadmin handles as a warning (because
the approach used by svn_fs_fs__pack() doesn't seem to be technically
applicable here).
> I'm proposing to modify fs_has_rep_sharing() to return True when running
> on FSFS with rep-sharing enabled and to return False on other backends.
> That's separate from your test's decorators.
>
I think that this might be unrelated to this patch, but also I may be
misunderstanding
what exactly you propose. If we modify fs_has_rep_sharing() to return True
for FSFS and to return False for other backends, we will have a problem
with FSX,
which also supports rep-sharing.
I've attached an updated patch.
Regards,
Denis Kovalchuk
Re: [PATCH] svnadmin build-repcache command
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Denis Kovalchuk wrote on Fri, 20 Mar 2020 22:54 +0300:
> > STMT_SET_REP uses «INSERT OR FAIL». Therefore, this call will fail if >
> any of the reps in REV are in rep-cache.db. In particular, I expect it >
> will fail in the common case that _all_ of the reps in REV are already > in
> the database (which can happen if enable-rep-sharing was toggled > during
> the filesystem's lifetime). Shouldn't it silently succeed in > that case?
> I.e., shouldn't the API be idempotent? If I am not mistaken,
> svn_fs_fs__set_rep_reference() has a special handling for the case and does
> not fail. It ignores SVN_ERR_SQLITE_CONSTRAINT error and then tries to get
> an existing representation, after which it succeeds.
Good catch.
> We already rely on the
> behavior when committing transactions (see write_reps_to_cache() function
> in libsvn_fs_fs/transaction.c). I think there is a way to have similar
> behavior using "INSERT OR IGNORE" for STMT_SET_REP. In addition, we will
> get a slight performance improvement, because we remove extra
> svn_fs_fs__get_rep_reference()
> call. I suggest to fix it
>
> with a separate patch.
Well, maybe. The code _appears_ to take a roundabout approach, but I'm
not sure that's actually a bug: there might be a difference between
«INSERT OR IGNORE» on the one hand, and the current behaviour («INSERT
OR FAIL» plus a C check) on the other.
Is «INSERT OR IGNORE» supported by all SQLite versions we support?
> > Shouldn't this case be an error? This isn't a case of "Nothing done, >
> nothing said" as above, but a case of "the precondition is unmet". What if
> it’s consistent with the svn_fs_fs__pack() behavior? If a repository does
> not support the feature, it returns an error. If the feature is explicitly
>
> disabled, it silently succeeds.
Printing an error when the fs format does not support rep-cache.db, as
the patch v2 does, is fine.
However, 'svnadmin pack' doesn't "silently succeed" when packing is
disabled; it prints a warning. So the question now is whether a warning
— i.e., an API notification — should be emitted when rep-sharing is
supported by the library and the fs format but disabled in fsfs.conf.
WDYT?
> > Shouldn't
> this function take a write lock — at least, to prevent > «svnadmin upgrade»
> from running concurrently? I am not sure that we need to take a write lock.
> We only read revisions and do not modify anything.
⋮
> Furthermore, we already have similar behavior for a transaction commit
> (see svn_fs_fs__commit() function in libsvn_fs_fs/transaction.c),
> where we add new entries to the rep-cache without a write lock.
Good point.
> > Sounds like we should have fs_has_rep_sharing() call is_fs_type_fsfs(). I
> am not sure that I understood you correctly. We cannot have only
> fs_has_rep_sharing()
> call for the test, because fsx also supports rep-sharing, but we do not
> have an implementation for it.
I'm proposing to modify fs_has_rep_sharing() to return True when running
on FSFS with rep-sharing enabled and to return False on other backends.
That's separate from your test's decorators.
>
In the future please don't wrap >-quoted lines.
Re: [PATCH] svnadmin build-repcache command
Posted by Denis Kovalchuk <de...@visualsvn.com>.
Thanks for your replies. > Add: > > /* See svn_fs_fs__build_rep_cache().
*/
Fixed.
> STMT_SET_REP uses «INSERT OR FAIL». Therefore, this call will fail if >
any of the reps in REV are in rep-cache.db. In particular, I expect it >
will fail in the common case that _all_ of the reps in REV are already > in
the database (which can happen if enable-rep-sharing was toggled > during
the filesystem's lifetime). Shouldn't it silently succeed in > that case?
I.e., shouldn't the API be idempotent? If I am not mistaken,
svn_fs_fs__set_rep_reference() has a special handling for the case and does
not fail. It ignores SVN_ERR_SQLITE_CONSTRAINT error and then tries to get
an existing representation, after which it succeeds. We already rely on the
behavior when committing transactions (see write_reps_to_cache() function
in libsvn_fs_fs/transaction.c). I think there is a way to have similar
behavior using "INSERT OR IGNORE" for STMT_SET_REP. In addition, we will
get a slight performance improvement, because we remove extra
svn_fs_fs__get_rep_reference()
call. I suggest to fix it
with a separate patch.
> Shouldn't this case be an error? This isn't a case of "Nothing done, >
nothing said" as above, but a case of "the precondition is unmet". What if
it’s consistent with the svn_fs_fs__pack() behavior? If a repository does
not support the feature, it returns an error. If the feature is explicitly
disabled, it silently succeeds. > This comment should be placed near the
«start_rev = 1;» line (snipped), > rather than here. Fixed. > Shouldn't
this function take a write lock — at least, to prevent > «svnadmin upgrade»
from running concurrently? I am not sure that we need to take a write lock.
We only read revisions and do not modify anything. As far as I know,
concurrent reading on fsfs is safe and does not require any locks. SQLite
also handles concurrency well. Furthermore, we already have similar
behavior for a transaction commit (see svn_fs_fs__commit() function in
libsvn_fs_fs/transaction.c), where we add new entries to the rep-cache
without a write lock. > Why isn't there a comma between the two string
literals, as in the other > array entries?
Fixed.
> Sounds like we should have fs_has_rep_sharing() call is_fs_type_fsfs(). I
am not sure that I understood you correctly. We cannot have only
fs_has_rep_sharing()
call for the test, because fsx also supports rep-sharing, but we do not
have an implementation for it. I have attached an updated patch. Regards, Denis
Kovalchuk
On Wed, Mar 18, 2020 at 7:22 PM Daniel Shahaf <d....@daniel.shahaf.name>
wrote:
> Denis Kovalchuk wrote on Wed, 18 Mar 2020 17:32 +0300:
> > +++ subversion/include/private/svn_fs_fs_private.h (working copy)
> > @@ -353,6 +353,16 @@
>
> Please use the -p option to diff.
>
> > +typedef struct svn_fs_fs__ioctl_build_rep_cache_input_t
> > +{
> > + svn_revnum_t start_rev;
> > + svn_revnum_t end_rev;
> > + svn_fs_progress_notify_func_t progress_func;
> > + void *progress_baton;
> > +} svn_fs_fs__ioctl_build_rep_cache_input_t;
> > +
>
> Add:
>
> /* See svn_fs_fs__build_rep_cache(). */
>
> > +SVN_FS_DECLARE_IOCTL_CODE(SVN_FS_FS__IOCTL_BUILD_REP_CACHE,
> SVN_FS_TYPE_FSFS, 1004);
>
> > +++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
> > +static svn_error_t *
> > +reindex_node(svn_fs_t *fs,
> ⋮
> > +{
> ⋮
> > + if (noderev->data_rep && noderev->data_rep->revision == rev &&
> > + noderev->kind == svn_node_file)
> > + {
> > + SVN_ERR(ensure_representation_sha1(fs, noderev->data_rep, pool));
> > + SVN_ERR(svn_fs_fs__set_rep_reference(fs, noderev->data_rep,
> pool));
>
> STMT_SET_REP uses «INSERT OR FAIL». Therefore, this call will fail if
> any of the reps in REV are in rep-cache.db. In particular, I expect it
> will fail in the common case that _all_ of the reps in REV are already
> in the database (which can happen if enable-rep-sharing was toggled
> during the filesystem's lifetime). Shouldn't it silently succeed in
> that case? I.e., shouldn't the API be idempotent?
>
> > + }
> > +
> > + if (noderev->prop_rep && noderev->prop_rep->revision == rev)
> > + {
> > + SVN_ERR(ensure_representation_sha1(fs, noderev->prop_rep, pool));
> > + SVN_ERR(svn_fs_fs__set_rep_reference(fs, noderev->prop_rep,
> pool));
>
> Ditto.
>
> > + }
> > +
> > + return SVN_NO_ERROR;
> > +}
>
> > +svn_fs_fs__build_rep_cache(svn_fs_t *fs,
> ⋮
> > +{
> > + if (!ffd->rep_sharing_allowed)
> > + {
> > + return SVN_NO_ERROR;
>
> Shouldn't this case be an error? This isn't a case of "Nothing done,
> nothing said" as above, but a case of "the precondition is unmet".
>
> If need be, rep_sharing_allowed can be exposed via `svnadmin info`.
>
> > + }
>
> > + /* Do not build rep-cache for revision zero to match
> > + * svn_fs_fs__create() behavior. */
> > + for (rev = start_rev; rev <= end_rev; rev++)
>
> This comment should be placed near the «start_rev = 1;» line (snipped),
> rather than here.
>
> > + {
> ⋮
> > + SVN_ERR(svn_sqlite__begin_transaction(ffd->rep_cache_db));
> > + err = reindex_node(fs, root_id, rev, file, cancel_func,
> cancel_baton, iterpool);
> > + SVN_ERR(svn_sqlite__finish_transaction(ffd->rep_cache_db, err));
>
> Shouldn't this function take a write lock — at least, to prevent
> «svnadmin upgrade» from running concurrently?
>
> > static const svn_opt_subcommand_desc3_t cmd_table[] =
> > {
> > + {"build-repcache", subcommand_build_repcache, {0}, {N_(
> > + "usage: svnadmin build-repcache REPOS_PATH [-r LOWER[:UPPER]]\n"
> > + "\n") N_(
> > + "Add missing entries to the representation cache for the
> repository\n"
> > + "at REPOS_PATH. Process data in revisions LOWER through UPPER.\n"
> > + "If no revision arguments are given, process all revisions. If
> only\n"
> > + "LOWER revision argument is given, process only that single
> revision.\n"
> > + )},
>
> Why isn't there a comma between the two string literals, as in the other
> array entries?
>
> > + {'r', 'q', 'M'} },
>
> > +++ subversion/tests/cmdline/svnadmin_tests.py (working copy)
> > @@ -4036,7 +4036,28 @@
> > +@SkipUnless(svntest.main.is_fs_type_fsfs)
> > +@SkipUnless(svntest.main.fs_has_rep_sharing)
>
> Sounds like we should have fs_has_rep_sharing() call is_fs_type_fsfs().
>
> Cheers,
>
> Daniel
>
Re: [PATCH] svnadmin build-repcache command
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Denis Kovalchuk wrote on Wed, 18 Mar 2020 17:32 +0300:
> +++ subversion/include/private/svn_fs_fs_private.h (working copy)
> @@ -353,6 +353,16 @@
Please use the -p option to diff.
> +typedef struct svn_fs_fs__ioctl_build_rep_cache_input_t
> +{
> + svn_revnum_t start_rev;
> + svn_revnum_t end_rev;
> + svn_fs_progress_notify_func_t progress_func;
> + void *progress_baton;
> +} svn_fs_fs__ioctl_build_rep_cache_input_t;
> +
Add:
/* See svn_fs_fs__build_rep_cache(). */
> +SVN_FS_DECLARE_IOCTL_CODE(SVN_FS_FS__IOCTL_BUILD_REP_CACHE, SVN_FS_TYPE_FSFS, 1004);
> +++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
> +static svn_error_t *
> +reindex_node(svn_fs_t *fs,
⋮
> +{
⋮
> + if (noderev->data_rep && noderev->data_rep->revision == rev &&
> + noderev->kind == svn_node_file)
> + {
> + SVN_ERR(ensure_representation_sha1(fs, noderev->data_rep, pool));
> + SVN_ERR(svn_fs_fs__set_rep_reference(fs, noderev->data_rep, pool));
STMT_SET_REP uses «INSERT OR FAIL». Therefore, this call will fail if
any of the reps in REV are in rep-cache.db. In particular, I expect it
will fail in the common case that _all_ of the reps in REV are already
in the database (which can happen if enable-rep-sharing was toggled
during the filesystem's lifetime). Shouldn't it silently succeed in
that case? I.e., shouldn't the API be idempotent?
> + }
> +
> + if (noderev->prop_rep && noderev->prop_rep->revision == rev)
> + {
> + SVN_ERR(ensure_representation_sha1(fs, noderev->prop_rep, pool));
> + SVN_ERR(svn_fs_fs__set_rep_reference(fs, noderev->prop_rep, pool));
Ditto.
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +svn_fs_fs__build_rep_cache(svn_fs_t *fs,
⋮
> +{
> + if (!ffd->rep_sharing_allowed)
> + {
> + return SVN_NO_ERROR;
Shouldn't this case be an error? This isn't a case of "Nothing done,
nothing said" as above, but a case of "the precondition is unmet".
If need be, rep_sharing_allowed can be exposed via `svnadmin info`.
> + }
> + /* Do not build rep-cache for revision zero to match
> + * svn_fs_fs__create() behavior. */
> + for (rev = start_rev; rev <= end_rev; rev++)
This comment should be placed near the «start_rev = 1;» line (snipped),
rather than here.
> + {
⋮
> + SVN_ERR(svn_sqlite__begin_transaction(ffd->rep_cache_db));
> + err = reindex_node(fs, root_id, rev, file, cancel_func, cancel_baton, iterpool);
> + SVN_ERR(svn_sqlite__finish_transaction(ffd->rep_cache_db, err));
Shouldn't this function take a write lock — at least, to prevent
«svnadmin upgrade» from running concurrently?
> static const svn_opt_subcommand_desc3_t cmd_table[] =
> {
> + {"build-repcache", subcommand_build_repcache, {0}, {N_(
> + "usage: svnadmin build-repcache REPOS_PATH [-r LOWER[:UPPER]]\n"
> + "\n") N_(
> + "Add missing entries to the representation cache for the repository\n"
> + "at REPOS_PATH. Process data in revisions LOWER through UPPER.\n"
> + "If no revision arguments are given, process all revisions. If only\n"
> + "LOWER revision argument is given, process only that single revision.\n"
> + )},
Why isn't there a comma between the two string literals, as in the other
array entries?
> + {'r', 'q', 'M'} },
> +++ subversion/tests/cmdline/svnadmin_tests.py (working copy)
> @@ -4036,7 +4036,28 @@
> +@SkipUnless(svntest.main.is_fs_type_fsfs)
> +@SkipUnless(svntest.main.fs_has_rep_sharing)
Sounds like we should have fs_has_rep_sharing() call is_fs_type_fsfs().
Cheers,
Daniel
Re: [PATCH] svnadmin build-repcache command
Posted by Julian Foad <ju...@apache.org>.
Denis Kovalchuk wrote:
> I would like to suggest an implementation of ‘svnadmin build-repcache’
> command
Thank you for contributing this enhancement!
I agree this is a useful and appropriate feature.
- Julian