You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Rall <dl...@collab.net> on 2006/12/29 08:56:01 UTC
Re: PATCH][merge-tracking]Create mergeinfo.db with proper schema in case it does not exist.
Kamesh wrote:
> Create mergeinfo.db with the necessary schema if the db does not
> exist with one.
I'm in favor of this concept.
> * subversion/libsvn_fs_util/merge-info-sqlite-index.c
> (CREATE_MERGEINFO_SCHEMA): New macro.
If the DB doesn't exist, is this be sufficient to create the DB
(empty), then also create the schema in it?
> (svn_fs_merge_info__update_index):
^
Please start the change log text on the same line as the symbol name.
> Create the default merge-tracking sqlite schema on first failure of sqlite
> operation.
> (svn_fs_merge_info__get_merge_info):
> Create the default merge-tracking sqlite schema on first failure of sqlite
> operation.
Since you say the same thing here, you can condense the change log by
listing both symbol names inside the same set of parenthesis,
separated by commas. Example:
(svn_fs_merge_info__update_index, svn_fs_merge_info__get_merge_info): ...
In this particular case, your change logs should actually have been a
little different, since you introduced a behavioral difference in the
get_merge_info() function.
Index: subversion/libsvn_fs_util/merge-info-sqlite-index.c
===================================================================
--- subversion/libsvn_fs_util/merge-info-sqlite-index.c (revision 22777)
+++ subversion/libsvn_fs_util/merge-info-sqlite-index.c (working copy)
@@ -96,6 +96,21 @@
"create index mi_c_revision_idx on mergeinfo_changed (revision);"
APR_EOL_STR;
This doc string could be cleaned up a bit. A few suggestions below,
but it the entire thing could flow a little better.
+/* Create the mergeinfo database with the proper schema if one already does
+ not exist. Sqlite was not friendlier enough to explicitly say that Table
"is not friendly enough" "table"
+ does not exist.
+ As per sqlite docs SQLITE_ERROR === "SQL error or missing database".
"the sqlite docs, SQLITE_ERROR corresponds to "
+ Our queries will never cause SQL error, so we can assume SQLITE_ERROR to be
You hope. What if a well-meaning sysadmin modified our table
structure, or fiddled around in some other fashion? I wouldn't make
such an absolute statement.
+ missing schema. Most likely caused by the newly upgraded repository created
+ originally by merge-tracking unaware svnadmin. */
Nice last comment.
+#define CREATE_MERGEINFO_SCHEMA(x, db) do \
"err" would be a more meaningful name than "x".
+{ \
+ if ((x) == SQLITE_ERROR) \
+ SVN_ERR(util_sqlite_exec(db, SVN_MTD_CREATE_SQL, NULL, NULL)); \
+ else \
+ SQLITE_ERR((x), db); \
+} while (0)
+
/* Create an sqlite DB for our merge info index under PATH. Use POOL
for temporary allocations. */
svn_error_t *
@@ -242,7 +257,7 @@
deletestring = apr_psprintf(pool,
"delete from mergeinfo_changed where revision = %ld;",
new_rev);
- SVN_ERR(util_sqlite_exec(db, deletestring, NULL, NULL));
+ CREATE_MERGEINFO_SCHEMA(sqlite3_exec(db, deletestring, NULL, NULL, NULL), db);
I guess we don't need this SQL statement to succeed, if the schema
hasn't been created yet.
deletestring = apr_psprintf(pool,
"delete from mergeinfo where revision = %ld;",
new_rev);
@@ -502,6 +517,9 @@
sqlite3_trace (db, sqlite_tracer, db);
#endif
+ CREATE_MERGEINFO_SCHEMA(sqlite3_exec(db, "select 1 from mergeinfo",
+ NULL, NULL, NULL),
+ db);
-1 on issuing a "select 1" on this often-invoked code path.
*mergeinfo = apr_hash_make (pool);
for (i = 0; i < paths->nelts; i++)
{
Re: PATCH][merge-tracking]Create mergeinfo.db with proper schema in case it does not exist.
Posted by Daniel Rall <dl...@collab.net>.
On Fri, 29 Dec 2006, Kamesh Jayachandran wrote:
...
> >In this particular case, your change logs should actually have been a
> >little different, since you introduced a behavioral difference in the
> >get_merge_info() function.
>
> Do you mean 'select 1 from mergeinfo' as a behavioural change?
Yup!
...
> >+ Our queries will never cause SQL error, so we can assume SQLITE_ERROR
> >to be
> >
> >You hope. What if a well-meaning sysadmin modified our table
> >structure, or fiddled around in some other fashion? I wouldn't make
> >such an absolute statement.
>
> sqlite operations to the db is limited to
> 'subversion/libsvn_fs_util/merge-info-sqlite-index.c' where as of today
> I see any no place we drop tables. And I could not envisage a situation
> where the constructed query could go wrong(May be I am wrong here, would
> need to see the queries much closely.)
I'm referring to unexpected differences, rather than to differences
that we can predict based on Subversion's code.
> Anyways CREATE_MERGEINFO_SCHEMA is used only in 2 places(delete from
> mergeinfo_changed where revision=some_rev and select 1 from mergeinfo).
> In both such places we never get SQLITE_ERROR except when those table
> does not exist.
...
> >+#define CREATE_MERGEINFO_SCHEMA(x, db) do
> >\
> >
> >"err" would be a more meaningful name than "x".
>
> I feel retval could be more meaningful.
That sounds good.
...
> >@@ -242,7 +257,7 @@
> > deletestring = apr_psprintf(pool,
> > "delete from mergeinfo_changed where
> > revision = %ld;",
> > new_rev);
> >- SVN_ERR(util_sqlite_exec(db, deletestring, NULL, NULL));
> >+ CREATE_MERGEINFO_SCHEMA(sqlite3_exec(db, deletestring, NULL, NULL,
> >NULL), db);
> >
> >I guess we don't need this SQL statement to succeed, if the schema
> >hasn't been created yet.
>
> Yes we don't need this to succeed in normal cases(Where we proper schema
> in place).
> When schema not in place I feel this is the right candidate to check for
> schema existence as it does not need any re-execution.
Yup!
...
> >@@ -502,6 +517,9 @@
> > sqlite3_trace (db, sqlite_tracer, db);
> > #endif
> >
> >+ CREATE_MERGEINFO_SCHEMA(sqlite3_exec(db, "select 1 from mergeinfo",
> >+ NULL, NULL, NULL),
> >+ db);
> >
> >-1 on issuing a "select 1" on this often-invoked code path.
>
> How often?
> 1)svn url1 url2
> 2)svn merge
> In both these cases we are running it only once(This query should be no
> cost query)
(At least) 'merge', 'copy', and 'move' should call get_merge_info().
While I have not measured this code, and I concur that a "select 1"
*should* be inconsequential, performance-wise, my concern about an
"often-invoked code path" is more that this is happening on the
repository-side, meaning that in a highly-concurrent environment this
effectively no-op set of instructions could be getting run quite a
bit.
I'd prefer that instead of using a no-op statement like "select 1",
that we do what we typically do across the Subversion code base, and
attempt the actual operation we want to perform. If that operation
fails, detect a missing schema, create the schema, and re-try the
original SQL. This does somewhat throw a wrench in your macro-based
implementation, but I'm sure you'll come up with an adjusted
implementation which fits the bill.
- Dan
Re: PATCH][merge-tracking]Create mergeinfo.db with proper schema
in case it does not exist.
Posted by Kamesh Jayachandran <ka...@collab.net>.
>> * subversion/libsvn_fs_util/merge-info-sqlite-index.c
>> (CREATE_MERGEINFO_SCHEMA): New macro.
>>
>
> If the DB doesn't exist, is this be sufficient to create the DB
> (empty), then also create the schema in it?
>
The sqlite3_open opens a db if exists, else creates an empty db and
opens it.
As we call sqlite3_open prior to possible execution of
CREATE_MERGEINFO_SCHEMA.(see it accepts 'db' as an argument.)
>
>> (svn_fs_merge_info__update_index):
>>
> ^
> Please start the change log text on the same line as the symbol name.
>
Fine.
>
>> Create the default merge-tracking sqlite schema on first failure of sqlite
>> operation.
>> (svn_fs_merge_info__get_merge_info):
>> Create the default merge-tracking sqlite schema on first failure of sqlite
>> operation.
>>
>
> Since you say the same thing here, you can condense the change log by
> listing both symbol names inside the same set of parenthesis,
> separated by commas. Example:
>
> (svn_fs_merge_info__update_index, svn_fs_merge_info__get_merge_info): ...
>
> In this particular case, your change logs should actually have been a
> little different, since you introduced a behavioral difference in the
> get_merge_info() function.
>
Do you mean 'select 1 from mergeinfo' as a behavioural change?
>
>
> +/* Create the mergeinfo database with the proper schema if one already does
> + not exist. Sqlite was not friendlier enough to explicitly say that Table
>
> "is not friendly enough" "table"
>
Accepted.
> + does not exist.
> + As per sqlite docs SQLITE_ERROR === "SQL error or missing database".
>
> "the sqlite docs, SQLITE_ERROR corresponds to "
>
Accepted.
> + Our queries will never cause SQL error, so we can assume SQLITE_ERROR to be
>
> You hope. What if a well-meaning sysadmin modified our table
> structure, or fiddled around in some other fashion? I wouldn't make
> such an absolute statement.
>
sqlite operations to the db is limited to
'subversion/libsvn_fs_util/merge-info-sqlite-index.c' where as of today
I see any no place we drop tables. And I could not envisage a situation
where the constructed query could go wrong(May be I am wrong here, would
need to see the queries much closely.)
Anyways CREATE_MERGEINFO_SCHEMA is used only in 2 places(delete from
mergeinfo_changed where revision=some_rev and select 1 from mergeinfo).
In both such places we never get SQLITE_ERROR except when those table
does not exist.
> + missing schema. Most likely caused by the newly upgraded repository created
> + originally by merge-tracking unaware svnadmin. */
>
> Nice last comment.
>
> +#define CREATE_MERGEINFO_SCHEMA(x, db) do \
>
> "err" would be a more meaningful name than "x".
>
I feel retval could be more meaningful.
> +{ \
> + if ((x) == SQLITE_ERROR) \
> + SVN_ERR(util_sqlite_exec(db, SVN_MTD_CREATE_SQL, NULL, NULL)); \
> + else \
> + SQLITE_ERR((x), db); \
> +} while (0)
> +
> /* Create an sqlite DB for our merge info index under PATH. Use POOL
> for temporary allocations. */
> svn_error_t *
> @@ -242,7 +257,7 @@
> deletestring = apr_psprintf(pool,
> "delete from mergeinfo_changed where revision = %ld;",
> new_rev);
> - SVN_ERR(util_sqlite_exec(db, deletestring, NULL, NULL));
> + CREATE_MERGEINFO_SCHEMA(sqlite3_exec(db, deletestring, NULL, NULL, NULL), db);
>
> I guess we don't need this SQL statement to succeed, if the schema
> hasn't been created yet.
>
Yes we don't need this to succeed in normal cases(Where we proper schema
in place).
When schema not in place I feel this is the right candidate to check for
schema existence as it does not need any re-execution.
> deletestring = apr_psprintf(pool,
> "delete from mergeinfo where revision = %ld;",
> new_rev);
> @@ -502,6 +517,9 @@
> sqlite3_trace (db, sqlite_tracer, db);
> #endif
>
> + CREATE_MERGEINFO_SCHEMA(sqlite3_exec(db, "select 1 from mergeinfo",
> + NULL, NULL, NULL),
> + db);
>
> -1 on issuing a "select 1" on this often-invoked code path.
>
How often?
1)svn url1 url2
2)svn merge
In both these cases we are running it only once(This query should be no
cost query)
Thanks
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org