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.
Received on 2020-03-20 22:49:21 CET