You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@gmail.com> on 2011/07/14 02:54:56 UTC
Re: svn commit: r1146528 - /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
On Wed, Jul 13, 2011 at 20:32, <da...@apache.org> wrote:
> Author: danielsh
> Date: Thu Jul 14 00:32:05 2011
> New Revision: 1146528
>
> URL: http://svn.apache.org/viewvc?rev=1146528&view=rev
> Log:
> * subversion/libsvn_fs_fs/fs_fs.c
> (write_config): Document that 'svnadmin verify' will access (and thus create)
> rep-cache.db regardless of whether rep-sharing (of new reps) is enabled
> in fsfs.conf.
If rep-sharing is turned off, then it seems wrong to spontaneously
create a .db that isn't begin used.
Cheers,
-g
Re: svn commit: r1146528 -
/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Posted by Julian Foad <ju...@wandisco.com>.
Daniel Shahaf wrote:
> One alternative solution is having svn_fs_fs__verify() stat() the DB
> file for existence before attempting to operate on it.
>
> I don't think it's the most elegant thing in the world, but it avoids
> spuriously creating the DB file without complicating the common case
> logic.
That sounds Good Enough.
- Julian
> I'm also open to alternative suggestions if anyone has them.
>
>
> Julian Foad wrote on Tue, Jul 19, 2011 at 19:12:33 +0100:
> > On Thu, 2011-07-14, Daniel Shahaf wrote:
> > > I've looked again at the code. Right now it opens the db, with
> > > svn_sqlite__mode_rwcreate, within an svn_atomic_init_once() block.
> > >
> > > Adding the special case "For <this> caller use svn_sqlite__mode_readwrite"
> > > means we can't use svn_atomic_init_once() any more. (since it's no
> > > longer "once", maybe the first call failed due to being non-rwcreate)
> > > And have to add some error handling logic around the open() call in that
> > > function.
> > >
> > > How about going for the less elegant but more readable solution,
> > >
> > > [[[
> > > Index: subversion/libsvn_fs_fs/rep-cache.c
> > > ===================================================================
> > > @@ -182,6 +187,11 @@ svn_fs_fs__walk_rep_reference(svn_fs_t *fs,
> > > }
> > > +
> > > + if (iterations == 0)
> > > + SVN_ERR(svn_io_remove_file2(path_rep_cache_db(fs->path, iterpool),
> > > + TRUE /* ignore_enoent */,
> > > + iterpool));
> > >
> > > SVN_ERR(svn_sqlite__reset(stmt));
> > >
> > > ]]]
> > >
> > > ?
> >
> > I've just looked at the code to see what you're suggesting here. You're
> > suggesting to make svn_fs_fs__walk_rep_reference() delete the rep cache
> > DB file if it's empty. Relying on the fact that svn_fs_fs__verify() is
> > currently the only caller of that function. That seems too ugly.
> >
> > - Julian
> >
> >
> > > > > We could make the code pass readwrite (rather than rwcreate) for that
> > > > > one caller, and then mask the error when the DB doesn't exist.
> > > >
> > > > +1.
> > > >
> > > > > (We do
> > > > > want to verify the DB if it's exists but fsfs.conf says "Don't use it",
> > > > > since fsfs.conf may get changed.)
> > > >
> > > > +1.
> > > >
> > > > > I'm not sure that non-creating an
> > > > > unused db justifies this additional code.
> > > >
> > > > I'm with Greg - it would be good for the users.
> > > >
> > > > - Julian
> >
> >
Re: svn commit: r1146528 -
/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
One alternative solution is having svn_fs_fs__verify() stat() the DB
file for existence before attempting to operate on it.
I don't think it's the most elegant thing in the world, but it avoids
spuriously creating the DB file without complicating the common case
logic.
I'm also open to alternative suggestions if anyone has them.
Julian Foad wrote on Tue, Jul 19, 2011 at 19:12:33 +0100:
> On Thu, 2011-07-14, Daniel Shahaf wrote:
> > I've looked again at the code. Right now it opens the db, with
> > svn_sqlite__mode_rwcreate, within an svn_atomic_init_once() block.
> >
> > Adding the special case "For <this> caller use svn_sqlite__mode_readwrite"
> > means we can't use svn_atomic_init_once() any more. (since it's no
> > longer "once", maybe the first call failed due to being non-rwcreate)
> > And have to add some error handling logic around the open() call in that
> > function.
> >
> > How about going for the less elegant but more readable solution,
> >
> > [[[
> > Index: subversion/libsvn_fs_fs/rep-cache.c
> > ===================================================================
> > @@ -182,6 +187,11 @@ svn_fs_fs__walk_rep_reference(svn_fs_t *fs,
> > }
> > +
> > + if (iterations == 0)
> > + SVN_ERR(svn_io_remove_file2(path_rep_cache_db(fs->path, iterpool),
> > + TRUE /* ignore_enoent */,
> > + iterpool));
> >
> > SVN_ERR(svn_sqlite__reset(stmt));
> >
> > ]]]
> >
> > ?
>
> I've just looked at the code to see what you're suggesting here. You're
> suggesting to make svn_fs_fs__walk_rep_reference() delete the rep cache
> DB file if it's empty. Relying on the fact that svn_fs_fs__verify() is
> currently the only caller of that function. That seems too ugly.
>
> - Julian
>
>
> > > > We could make the code pass readwrite (rather than rwcreate) for that
> > > > one caller, and then mask the error when the DB doesn't exist.
> > >
> > > +1.
> > >
> > > > (We do
> > > > want to verify the DB if it's exists but fsfs.conf says "Don't use it",
> > > > since fsfs.conf may get changed.)
> > >
> > > +1.
> > >
> > > > I'm not sure that non-creating an
> > > > unused db justifies this additional code.
> > >
> > > I'm with Greg - it would be good for the users.
> > >
> > > - Julian
>
>
Re: svn commit: r1146528 -
/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Posted by Julian Foad <ju...@wandisco.com>.
On Thu, 2011-07-14, Daniel Shahaf wrote:
> I've looked again at the code. Right now it opens the db, with
> svn_sqlite__mode_rwcreate, within an svn_atomic_init_once() block.
>
> Adding the special case "For <this> caller use svn_sqlite__mode_readwrite"
> means we can't use svn_atomic_init_once() any more. (since it's no
> longer "once", maybe the first call failed due to being non-rwcreate)
> And have to add some error handling logic around the open() call in that
> function.
>
> How about going for the less elegant but more readable solution,
>
> [[[
> Index: subversion/libsvn_fs_fs/rep-cache.c
> ===================================================================
> @@ -182,6 +187,11 @@ svn_fs_fs__walk_rep_reference(svn_fs_t *fs,
> }
> +
> + if (iterations == 0)
> + SVN_ERR(svn_io_remove_file2(path_rep_cache_db(fs->path, iterpool),
> + TRUE /* ignore_enoent */,
> + iterpool));
>
> SVN_ERR(svn_sqlite__reset(stmt));
>
> ]]]
>
> ?
I've just looked at the code to see what you're suggesting here. You're
suggesting to make svn_fs_fs__walk_rep_reference() delete the rep cache
DB file if it's empty. Relying on the fact that svn_fs_fs__verify() is
currently the only caller of that function. That seems too ugly.
- Julian
> > > We could make the code pass readwrite (rather than rwcreate) for that
> > > one caller, and then mask the error when the DB doesn't exist.
> >
> > +1.
> >
> > > (We do
> > > want to verify the DB if it's exists but fsfs.conf says "Don't use it",
> > > since fsfs.conf may get changed.)
> >
> > +1.
> >
> > > I'm not sure that non-creating an
> > > unused db justifies this additional code.
> >
> > I'm with Greg - it would be good for the users.
> >
> > - Julian
Re: svn commit: r1146528 -
/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
I've looked again at the code. Right now it opens the db, with
svn_sqlite__mode_rwcreate, within an svn_atomic_init_once() block.
Adding the special case "For <this> caller use svn_sqlite__mode_readwrite"
means we can't use svn_atomic_init_once() any more. (since it's no
longer "once", maybe the first call failed due to being non-rwcreate)
And have to add some error handling logic around the open() call in that
function.
How about going for the less elegant but more readable solution,
[[[
Index: subversion/libsvn_fs_fs/rep-cache.c
===================================================================
--- subversion/libsvn_fs_fs/rep-cache.c (revision 1146757)
+++ subversion/libsvn_fs_fs/rep-cache.c (working copy)
@@ -43,8 +43,13 @@ REP_CACHE_DB_SQL_DECLARE_STATEMENTS(statements);
/** Helper functions. **/
+static APR_INLINE const char *
+path_rep_cache_db(const char *fs_path,
+ apr_pool_t *result_pool)
+{
+ return svn_dirent_join(fs_path, REP_CACHE_DB_NAME, result_pool);
+}
-
/* Check that REP refers to a revision that exists in FS. */
static svn_error_t *
rep_has_been_born(representation_t *rep,
@@ -91,7 +96,7 @@ open_rep_cache(void *baton,
/* Open (or create) the sqlite database. It will be automatically
closed when fs->pool is destoyed. */
- db_path = svn_dirent_join(fs->path, REP_CACHE_DB_NAME, pool);
+ db_path = path_rep_cache_db(fs->path, pool);
SVN_ERR(svn_sqlite__open(&ffd->rep_cache_db, db_path,
svn_sqlite__mode_rwcreate, statements,
0, NULL,
@@ -182,6 +187,11 @@ svn_fs_fs__walk_rep_reference(svn_fs_t *fs,
SVN_ERR(svn_sqlite__step(&have_row, stmt));
}
+
+ if (iterations == 0)
+ SVN_ERR(svn_io_remove_file2(path_rep_cache_db(fs->path, iterpool),
+ TRUE /* ignore_enoent */,
+ iterpool));
SVN_ERR(svn_sqlite__reset(stmt));
]]]
?
Julian Foad wrote on Thu, Jul 14, 2011 at 12:00:09 +0100:
> On Thu, 2011-07-14 at 04:10 +0300, Daniel Shahaf wrote:
> > Greg Stein wrote on Wed, Jul 13, 2011 at 20:54:56 -0400:
> > > On Wed, Jul 13, 2011 at 20:32, <da...@apache.org> wrote:
> > > > Author: danielsh
> > > > Date: Thu Jul 14 00:32:05 2011
> > > > New Revision: 1146528
> > > >
> > > > URL: http://svn.apache.org/viewvc?rev=1146528&view=rev
> > > > Log:
> > > > * subversion/libsvn_fs_fs/fs_fs.c
> > > > (write_config): Document that 'svnadmin verify' will access (and thus create)
> > > > rep-cache.db regardless of whether rep-sharing (of new reps) is enabled
> > > > in fsfs.conf.
> > >
> > > If rep-sharing is turned off, then it seems wrong to spontaneously
> > > create a .db that isn't begin used.
> >
> > It's harmless (it isn't being used).
>
> Harmless in functional terms only.
>
> > We could make the code pass readwrite (rather than rwcreate) for that
> > one caller, and then mask the error when the DB doesn't exist.
>
> +1.
>
> > (We do
> > want to verify the DB if it's exists but fsfs.conf says "Don't use it",
> > since fsfs.conf may get changed.)
>
> +1.
>
> > I'm not sure that non-creating an
> > unused db justifies this additional code.
>
> I'm with Greg - it would be good for the users.
>
> - Julian
>
>
Re: svn commit: r1146528 -
/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Posted by Julian Foad <ju...@wandisco.com>.
On Thu, 2011-07-14 at 04:10 +0300, Daniel Shahaf wrote:
> Greg Stein wrote on Wed, Jul 13, 2011 at 20:54:56 -0400:
> > On Wed, Jul 13, 2011 at 20:32, <da...@apache.org> wrote:
> > > Author: danielsh
> > > Date: Thu Jul 14 00:32:05 2011
> > > New Revision: 1146528
> > >
> > > URL: http://svn.apache.org/viewvc?rev=1146528&view=rev
> > > Log:
> > > * subversion/libsvn_fs_fs/fs_fs.c
> > > (write_config): Document that 'svnadmin verify' will access (and thus create)
> > > rep-cache.db regardless of whether rep-sharing (of new reps) is enabled
> > > in fsfs.conf.
> >
> > If rep-sharing is turned off, then it seems wrong to spontaneously
> > create a .db that isn't begin used.
>
> It's harmless (it isn't being used).
Harmless in functional terms only.
> We could make the code pass readwrite (rather than rwcreate) for that
> one caller, and then mask the error when the DB doesn't exist.
+1.
> (We do
> want to verify the DB if it's exists but fsfs.conf says "Don't use it",
> since fsfs.conf may get changed.)
+1.
> I'm not sure that non-creating an
> unused db justifies this additional code.
I'm with Greg - it would be good for the users.
- Julian
Re: svn commit: r1146528 - /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Jul 13, 2011 at 21:10, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> Greg Stein wrote on Wed, Jul 13, 2011 at 20:54:56 -0400:
>> On Wed, Jul 13, 2011 at 20:32, <da...@apache.org> wrote:
>> > Author: danielsh
>> > Date: Thu Jul 14 00:32:05 2011
>> > New Revision: 1146528
>> >
>> > URL: http://svn.apache.org/viewvc?rev=1146528&view=rev
>> > Log:
>> > * subversion/libsvn_fs_fs/fs_fs.c
>> > (write_config): Document that 'svnadmin verify' will access (and thus create)
>> > rep-cache.db regardless of whether rep-sharing (of new reps) is enabled
>> > in fsfs.conf.
>>
>> If rep-sharing is turned off, then it seems wrong to spontaneously
>> create a .db that isn't begin used.
>
> It's harmless (it isn't being used).
>
> We could make the code pass readwrite (rather than rwcreate) for that
> one caller, and then mask the error when the DB doesn't exist. (We do
> want to verify the DB if it's exists but fsfs.conf says "Don't use it",
> since fsfs.conf may get changed.) I'm not sure that non-creating an
> unused db justifies this additional code.
I'm thinking of the sysadmin running into it, and thinking "wha...?! I
turned that off. <goes to check>". Could be confusing.
Cheers,
-g
Re: svn commit: r1146528 -
/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Greg Stein wrote on Wed, Jul 13, 2011 at 20:54:56 -0400:
> On Wed, Jul 13, 2011 at 20:32, <da...@apache.org> wrote:
> > Author: danielsh
> > Date: Thu Jul 14 00:32:05 2011
> > New Revision: 1146528
> >
> > URL: http://svn.apache.org/viewvc?rev=1146528&view=rev
> > Log:
> > * subversion/libsvn_fs_fs/fs_fs.c
> > (write_config): Document that 'svnadmin verify' will access (and thus create)
> > rep-cache.db regardless of whether rep-sharing (of new reps) is enabled
> > in fsfs.conf.
>
> If rep-sharing is turned off, then it seems wrong to spontaneously
> create a .db that isn't begin used.
It's harmless (it isn't being used).
We could make the code pass readwrite (rather than rwcreate) for that
one caller, and then mask the error when the DB doesn't exist. (We do
want to verify the DB if it's exists but fsfs.conf says "Don't use it",
since fsfs.conf may get changed.) I'm not sure that non-creating an
unused db justifies this additional code.
>
> Cheers,
> -g