> > 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
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, 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. Patch by: Denis Kovalchuk <denis.kovalc...@visualsvn.com> Index: subversion/libsvn_fs_fs/rep-cache-db.sql =================================================================== --- subversion/libsvn_fs_fs/rep-cache-db.sql (revision 1875655) +++ subversion/libsvn_fs_fs/rep-cache-db.sql (working copy) @@ -61,7 +61,7 @@ WHERE hash = ?1 -- STMT_SET_REP /* Works for both V1 and V2 schemas. */ -INSERT OR FAIL INTO rep_cache (hash, revision, offset, size, expanded_size) +INSERT OR IGNORE INTO rep_cache (hash, revision, offset, size, expanded_size) VALUES (?1, ?2, ?3, ?4, ?5) -- STMT_GET_REPS_FOR_RANGE Index: subversion/libsvn_fs_fs/rep-cache.c =================================================================== --- subversion/libsvn_fs_fs/rep-cache.c (revision 1875655) +++ subversion/libsvn_fs_fs/rep-cache.c (working copy) @@ -328,7 +328,6 @@ svn_fs_fs__set_rep_reference(svn_fs_t *fs, { fs_fs_data_t *ffd = fs->fsap_data; svn_sqlite__stmt_t *stmt; - svn_error_t *err; svn_checksum_t checksum; checksum.kind = svn_checksum_sha1; checksum.digest = rep->sha1_digest; @@ -351,29 +350,8 @@ svn_fs_fs__set_rep_reference(svn_fs_t *fs, (apr_int64_t) rep->size, (apr_int64_t) rep->expanded_size)); - err = svn_sqlite__insert(NULL, stmt); - if (err) - { - representation_t *old_rep; + SVN_ERR(svn_sqlite__insert(NULL, stmt)); - if (err->apr_err != SVN_ERR_SQLITE_CONSTRAINT) - return svn_error_trace(err); - - svn_error_clear(err); - - /* Constraint failed so the mapping for SHA1_CHECKSUM->REP - should exist. If so that's cool -- just do nothing. If not, - that's a red flag! */ - SVN_ERR(svn_fs_fs__get_rep_reference(&old_rep, fs, &checksum, pool)); - - if (!old_rep) - { - /* Something really odd at this point, we failed to insert the - checksum AND failed to read an existing checksum. Do we need - to flag this? */ - } - } - return SVN_NO_ERROR; }
