You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ph...@apache.org on 2010/04/14 18:35:11 UTC
svn commit: r934008 - /subversion/trunk/subversion/libsvn_subr/sqlite.c
Author: philip
Date: Wed Apr 14 16:35:11 2010
New Revision: 934008
URL: http://svn.apache.org/viewvc?rev=934008&view=rev
Log:
* subversion/libsvn_subr/sqlite.c
(svn_sqlite__hotcopy): Use the SQLite backup interface if available.
Modified:
subversion/trunk/subversion/libsvn_subr/sqlite.c
Modified: subversion/trunk/subversion/libsvn_subr/sqlite.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/sqlite.c?rev=934008&r1=934007&r2=934008&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/sqlite.c (original)
+++ subversion/trunk/subversion/libsvn_subr/sqlite.c Wed Apr 14 16:35:11 2010
@@ -975,23 +975,55 @@ svn_sqlite__hotcopy(const char *src_path
const char *dst_path,
apr_pool_t *scratch_pool)
{
- svn_sqlite__db_t *db;
- svn_sqlite__stmt_t *stmt;
+ svn_sqlite__db_t *src_db;
- /* The SELECT takes a shared lock in the source database which
- blocks writers and so ensures that the database won't change
- during the copy.
-
- We could use the SQLite backup interface (from 3.6.11 and still
- experimental) and the copy would be done in chunks with the lock
- released between chunks. */
- SVN_ERR(svn_sqlite__open(&db, src_path, svn_sqlite__mode_readonly,
+ SVN_ERR(svn_sqlite__open(&src_db, src_path, svn_sqlite__mode_readonly,
internal_statements, 0, NULL,
scratch_pool, scratch_pool));
- SVN_ERR(svn_sqlite__get_statement(&stmt, db, STMT_DUMMY_SELECT_FOR_BACKUP));
- SVN_ERR(svn_sqlite__step_row(stmt));
- SVN_ERR(svn_io_copy_file(src_path, dst_path, TRUE, scratch_pool));
- SVN_ERR(svn_sqlite__close(db));
+
+#if SQLITE_VERSION_AT_LEAST(3,6,11)
+ {
+ svn_sqlite__db_t *dst_db;
+ sqlite3_backup *backup;
+ int rc1, rc2;
+
+ SVN_ERR(svn_sqlite__open(&dst_db, dst_path, svn_sqlite__mode_rwcreate,
+ NULL, 0, NULL, scratch_pool, scratch_pool));
+ backup = sqlite3_backup_init(dst_db->db3, "main", src_db->db3, "main");
+ if (!backup)
+ return SVN_NO_ERROR;
+ do
+ {
+ /* Pages are usually 1024 byte (SQLite docs). On my laptop
+ copying gets faster as the number of pages is increased up
+ to about 64, beyond that speed levels off. Lets put the
+ number of pages an order of magnitude higher, this is still
+ likely to be a fraction of large databases. */
+ rc1 = sqlite3_backup_step(backup, 1024);
+ if (rc1 == SQLITE_BUSY || rc1 == SQLITE_LOCKED)
+ sqlite3_sleep(25);
+ }
+ while (rc1 == SQLITE_OK || rc1 == SQLITE_BUSY || rc1 == SQLITE_LOCKED);
+ rc2 = sqlite3_backup_finish(backup);
+ if (rc1 != SQLITE_DONE)
+ SQLITE_ERR(rc1, dst_db);
+ SQLITE_ERR(rc2, dst_db);
+ SVN_ERR(svn_sqlite__close(dst_db));
+ }
+#else
+ {
+ svn_sqlite__stmt_t *stmt;
+ /* The SELECT takes a shared lock in the source database which
+ blocks writers and so ensures that the database won't change
+ during the copy. */
+ SVN_ERR(svn_sqlite__get_statement(&stmt, src_db,
+ STMT_DUMMY_SELECT_FOR_BACKUP));
+ SVN_ERR(svn_sqlite__step_row(stmt));
+ SVN_ERR(svn_io_copy_file(src_path, dst_path, TRUE, scratch_pool));
+ }
+#endif
+
+ SVN_ERR(svn_sqlite__close(src_db));
return SVN_NO_ERROR;
}
Re: svn commit: r934008 - /subversion/trunk/subversion/libsvn_subr/sqlite.c
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
> philip@apache.org wrote on Wed, 14 Apr 2010 at 16:35 -0000:
>> - /* We could use the SQLite backup interface (from 3.6.11 and still
>> - experimental) and the copy would be done in chunks with the lock
>> - released between chunks. */
>> +#if SQLITE_VERSION_AT_LEAST(3,6,11)
>> + {
>
> Given that the API is experimental, perhaps we shouldn't automatically
> allow all future versions of sqlite here?
SQLite say that it is rare to change experimental interfaces:
http://www.sqlite.org/c3ref/experimental.html
--
Philip
Re: svn commit: r934008 -
/subversion/trunk/subversion/libsvn_subr/sqlite.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
philip@apache.org wrote on Wed, 14 Apr 2010 at 16:35 -0000:
> - /* We could use the SQLite backup interface (from 3.6.11 and still
> - experimental) and the copy would be done in chunks with the lock
> - released between chunks. */
> +#if SQLITE_VERSION_AT_LEAST(3,6,11)
> + {
Given that the API is experimental, perhaps we shouldn't automatically
allow all future versions of sqlite here?
We can allow everything up to whatever is currently latest, but forbid
new versions (since the API is "is subject to change without notice").
[[[
Index: sqlite.c
===================================================================
--- sqlite.c (revision 934083)
+++ sqlite.c (working copy)
@@ -981,7 +981,9 @@ svn_sqlite__hotcopy(const char *src_path,
internal_statements, 0, NULL,
scratch_pool, scratch_pool));
-#if SQLITE_VERSION_AT_LEAST(3,6,11)
+#if SQLITE_VERSION_AT_LEAST(3,6,24)
+# error "sqlite3_backup_*() is experimental and may have changed semantics"
+#elif SQLITE_VERSION_AT_LEAST(3,6,11)
{
svn_sqlite__db_t *dst_db;
sqlite3_backup *backup;
]]]
?
Daniel
> + svn_sqlite__db_t *dst_db;
> + sqlite3_backup *backup;
> + int rc1, rc2;
> +
> + SVN_ERR(svn_sqlite__open(&dst_db, dst_path, svn_sqlite__mode_rwcreate,
> + NULL, 0, NULL, scratch_pool, scratch_pool));
> + backup = sqlite3_backup_init(dst_db->db3, "main", src_db->db3, "main");
> + if (!backup)
> + return SVN_NO_ERROR;
> + do
> + {
> + /* Pages are usually 1024 byte (SQLite docs). On my laptop
> + copying gets faster as the number of pages is increased up
> + to about 64, beyond that speed levels off. Lets put the
> + number of pages an order of magnitude higher, this is still
> + likely to be a fraction of large databases. */
> + rc1 = sqlite3_backup_step(backup, 1024);
> + if (rc1 == SQLITE_BUSY || rc1 == SQLITE_LOCKED)
> + sqlite3_sleep(25);
> + }
> + while (rc1 == SQLITE_OK || rc1 == SQLITE_BUSY || rc1 == SQLITE_LOCKED);
> + rc2 = sqlite3_backup_finish(backup);
> + if (rc1 != SQLITE_DONE)
> + SQLITE_ERR(rc1, dst_db);
> + SQLITE_ERR(rc2, dst_db);
> + SVN_ERR(svn_sqlite__close(dst_db));
> + }
> +#else
Re: svn commit: r934008 - /subversion/trunk/subversion/libsvn_subr/sqlite.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
Philip Martin wrote:
> philip@apache.org writes:
>
>> Author: philip
>> Date: Wed Apr 14 16:35:11 2010
>> New Revision: 934008
>>
>> URL: http://svn.apache.org/viewvc?rev=934008&view=rev
>> Log:
>> * subversion/libsvn_subr/sqlite.c
>> (svn_sqlite__hotcopy): Use the SQLite backup interface if available.
>>
>> Modified:
>> subversion/trunk/subversion/libsvn_subr/sqlite.c
>
> The backup method works but is measurably slower in my tests than than
> the plain copy under a lock; the benefit should be better concurrency.
>
> I don't know which is better: the faster hotcopy that blocks writers
> or the slower hotcopy that allows writers to progress? At the moment
> it's a compile time decision based on the version of SQLite but that's
> not a good way to decide whether we should be using this feature. I
> think it's too obscure to make it configurable on a per-repository
> basis so I don't really know what we should do.
The nature of a "hot copy" is generally such that the copied thing isn't
"unavailable" by most definitions thereof. Not sure that write-locking the
repository really counts as "hot". :-)
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn commit: r934008 - /subversion/trunk/subversion/libsvn_subr/sqlite.c
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Apr 14, 2010 at 12:43, Philip Martin <ph...@wandisco.com> wrote:
> philip@apache.org writes:
>
>> Author: philip
>> Date: Wed Apr 14 16:35:11 2010
>> New Revision: 934008
>>
>> URL: http://svn.apache.org/viewvc?rev=934008&view=rev
>> Log:
>> * subversion/libsvn_subr/sqlite.c
>> (svn_sqlite__hotcopy): Use the SQLite backup interface if available.
>>
>> Modified:
>> subversion/trunk/subversion/libsvn_subr/sqlite.c
>
> The backup method works but is measurably slower in my tests than than
> the plain copy under a lock; the benefit should be better concurrency.
>
> I don't know which is better: the faster hotcopy that blocks writers
> or the slower hotcopy that allows writers to progress? At the moment
> it's a compile time decision based on the version of SQLite but that's
> not a good way to decide whether we should be using this feature. I
> think it's too obscure to make it configurable on a per-repository
> basis so I don't really know what we should do.
To me, "hot copy" means that you are trying to continue operation.
Thus, the concurrency is needed.
I don't see that a hot copy needs to be blazing fast. Just concurrent.
Cheers,
-g
Re: svn commit: r934008 - /subversion/trunk/subversion/libsvn_subr/sqlite.c
Posted by Philip Martin <ph...@wandisco.com>.
philip@apache.org writes:
> Author: philip
> Date: Wed Apr 14 16:35:11 2010
> New Revision: 934008
>
> URL: http://svn.apache.org/viewvc?rev=934008&view=rev
> Log:
> * subversion/libsvn_subr/sqlite.c
> (svn_sqlite__hotcopy): Use the SQLite backup interface if available.
>
> Modified:
> subversion/trunk/subversion/libsvn_subr/sqlite.c
The backup method works but is measurably slower in my tests than than
the plain copy under a lock; the benefit should be better concurrency.
I don't know which is better: the faster hotcopy that blocks writers
or the slower hotcopy that allows writers to progress? At the moment
it's a compile time decision based on the version of SQLite but that's
not a good way to decide whether we should be using this feature. I
think it's too obscure to make it configurable on a per-repository
basis so I don't really know what we should do.
--
Philip
Re: svn commit: r934008 -
/subversion/trunk/subversion/libsvn_subr/sqlite.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
philip@apache.org wrote on Wed, 14 Apr 2010 at 16:35 -0000:
> - /* We could use the SQLite backup interface (from 3.6.11 and still
> - experimental) and the copy would be done in chunks with the lock
> - released between chunks. */
> +#if SQLITE_VERSION_AT_LEAST(3,6,11)
> + {
Given that the API is experimental, perhaps we shouldn't automatically
allow all future versions of sqlite here?
We can allow everything up to whatever is currently latest, but forbid
new versions (since the API is "is subject to change without notice").
[[[
Index: sqlite.c
===================================================================
--- sqlite.c (revision 934083)
+++ sqlite.c (working copy)
@@ -981,7 +981,9 @@ svn_sqlite__hotcopy(const char *src_path,
internal_statements, 0, NULL,
scratch_pool, scratch_pool));
-#if SQLITE_VERSION_AT_LEAST(3,6,11)
+#if SQLITE_VERSION_AT_LEAST(3,6,24)
+# error "sqlite3_backup_*() is experimental and may have changed semantics"
+#elif SQLITE_VERSION_AT_LEAST(3,6,11)
{
svn_sqlite__db_t *dst_db;
sqlite3_backup *backup;
]]]
?
Daniel
> + svn_sqlite__db_t *dst_db;
> + sqlite3_backup *backup;
> + int rc1, rc2;
> +
> + SVN_ERR(svn_sqlite__open(&dst_db, dst_path, svn_sqlite__mode_rwcreate,
> + NULL, 0, NULL, scratch_pool, scratch_pool));
> + backup = sqlite3_backup_init(dst_db->db3, "main", src_db->db3, "main");
> + if (!backup)
> + return SVN_NO_ERROR;
> + do
> + {
> + /* Pages are usually 1024 byte (SQLite docs). On my laptop
> + copying gets faster as the number of pages is increased up
> + to about 64, beyond that speed levels off. Lets put the
> + number of pages an order of magnitude higher, this is still
> + likely to be a fraction of large databases. */
> + rc1 = sqlite3_backup_step(backup, 1024);
> + if (rc1 == SQLITE_BUSY || rc1 == SQLITE_LOCKED)
> + sqlite3_sleep(25);
> + }
> + while (rc1 == SQLITE_OK || rc1 == SQLITE_BUSY || rc1 == SQLITE_LOCKED);
> + rc2 = sqlite3_backup_finish(backup);
> + if (rc1 != SQLITE_DONE)
> + SQLITE_ERR(rc1, dst_db);
> + SQLITE_ERR(rc2, dst_db);
> + SVN_ERR(svn_sqlite__close(dst_db));
> + }
> +#else
Re: svn commit: r934008 - /subversion/trunk/subversion/libsvn_subr/sqlite.c
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
>> + SVN_ERR(svn_sqlite__open(&dst_db, dst_path, svn_sqlite__mode_rwcreate,
>> + NULL, 0, NULL, scratch_pool, scratch_pool));
>> + backup = sqlite3_backup_init(dst_db->db3, "main", src_db->db3, "main");
>> + if (!backup)
>> + return SVN_NO_ERROR;
>
> Shouldn't the databases be closed? If not, then why not? (ie. add a comment)
Oops! I should be returning an error as well. That was supposed to
be temporary to try it out.
--
Philip
Re: svn commit: r934008 - /subversion/trunk/subversion/libsvn_subr/sqlite.c
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Apr 14, 2010 at 12:35, <ph...@apache.org> wrote:
>...
> +++ subversion/trunk/subversion/libsvn_subr/sqlite.c Wed Apr 14 16:35:11 2010
> @@ -975,23 +975,55 @@ svn_sqlite__hotcopy(const char *src_path
> const char *dst_path,
> apr_pool_t *scratch_pool)
> {
> - svn_sqlite__db_t *db;
> - svn_sqlite__stmt_t *stmt;
> + svn_sqlite__db_t *src_db;
>
> - /* The SELECT takes a shared lock in the source database which
> - blocks writers and so ensures that the database won't change
> - during the copy.
> -
> - We could use the SQLite backup interface (from 3.6.11 and still
> - experimental) and the copy would be done in chunks with the lock
> - released between chunks. */
> - SVN_ERR(svn_sqlite__open(&db, src_path, svn_sqlite__mode_readonly,
> + SVN_ERR(svn_sqlite__open(&src_db, src_path, svn_sqlite__mode_readonly,
> internal_statements, 0, NULL,
> scratch_pool, scratch_pool));
> - SVN_ERR(svn_sqlite__get_statement(&stmt, db, STMT_DUMMY_SELECT_FOR_BACKUP));
> - SVN_ERR(svn_sqlite__step_row(stmt));
> - SVN_ERR(svn_io_copy_file(src_path, dst_path, TRUE, scratch_pool));
> - SVN_ERR(svn_sqlite__close(db));
> +
> +#if SQLITE_VERSION_AT_LEAST(3,6,11)
> + {
> + svn_sqlite__db_t *dst_db;
> + sqlite3_backup *backup;
> + int rc1, rc2;
> +
> + SVN_ERR(svn_sqlite__open(&dst_db, dst_path, svn_sqlite__mode_rwcreate,
> + NULL, 0, NULL, scratch_pool, scratch_pool));
> + backup = sqlite3_backup_init(dst_db->db3, "main", src_db->db3, "main");
> + if (!backup)
> + return SVN_NO_ERROR;
Shouldn't the databases be closed? If not, then why not? (ie. add a comment)
>...
Cheers,
-g
Re: svn commit: r934008 - /subversion/trunk/subversion/libsvn_subr/sqlite.c
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Apr 14, 2010 at 12:35, <ph...@apache.org> wrote:
>...
> +++ subversion/trunk/subversion/libsvn_subr/sqlite.c Wed Apr 14 16:35:11 2010
> @@ -975,23 +975,55 @@ svn_sqlite__hotcopy(const char *src_path
> const char *dst_path,
> apr_pool_t *scratch_pool)
> {
> - svn_sqlite__db_t *db;
> - svn_sqlite__stmt_t *stmt;
> + svn_sqlite__db_t *src_db;
>
> - /* The SELECT takes a shared lock in the source database which
> - blocks writers and so ensures that the database won't change
> - during the copy.
> -
> - We could use the SQLite backup interface (from 3.6.11 and still
> - experimental) and the copy would be done in chunks with the lock
> - released between chunks. */
> - SVN_ERR(svn_sqlite__open(&db, src_path, svn_sqlite__mode_readonly,
> + SVN_ERR(svn_sqlite__open(&src_db, src_path, svn_sqlite__mode_readonly,
> internal_statements, 0, NULL,
> scratch_pool, scratch_pool));
> - SVN_ERR(svn_sqlite__get_statement(&stmt, db, STMT_DUMMY_SELECT_FOR_BACKUP));
> - SVN_ERR(svn_sqlite__step_row(stmt));
> - SVN_ERR(svn_io_copy_file(src_path, dst_path, TRUE, scratch_pool));
> - SVN_ERR(svn_sqlite__close(db));
> +
> +#if SQLITE_VERSION_AT_LEAST(3,6,11)
> + {
> + svn_sqlite__db_t *dst_db;
> + sqlite3_backup *backup;
> + int rc1, rc2;
> +
> + SVN_ERR(svn_sqlite__open(&dst_db, dst_path, svn_sqlite__mode_rwcreate,
> + NULL, 0, NULL, scratch_pool, scratch_pool));
> + backup = sqlite3_backup_init(dst_db->db3, "main", src_db->db3, "main");
> + if (!backup)
> + return SVN_NO_ERROR;
Shouldn't the databases be closed? If not, then why not? (ie. add a comment)
>...
Cheers,
-g