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.
