[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] svnadmin build-repcache command

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Fri, 20 Mar 2020 21:49:04 +0000

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

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.