Re: [PATCH] svnadmin build-repcache command

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.s_at_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
>