Hey,
Our email server is back up now. uggh
I need to sleep. But here is the response I promised.
Greg Stein wrote:
>On Wed, Mar 05, 2003 at 12:38:35PM -0600, cmpilato@tigris.org wrote:
>
>
>>...
>>* subversion/libsvn_fs/bdb/bdb-fs.c
>> New.
>> The following functions have been re-named and moved from fs.c:
>>
>>
...
>> (svn_fs_create_berkeley): was (svn_fs_create_fs)
>> (bdb_open_fs): was (svn_fs_open_berkeley)
>> (bdb_recover_fs): was (svn_fs_berkeley_recover)
>> (bdb_delete_fs): was (svn_fs_delete_berkeley)
>>
>>
>
>Woah. That's not good. Those functions are visible outside of the link unit.
>They should all be prefixed with svn_fs__
>
This is temporary. They are static in their final form. I decided to use
bdb_ rather than the current non prefixed versions (some were non
prefixed anyway) to help with debugging long term. Given they didn't
seem to conflict with any BDB library functions I felt it would be OK to
make them visible while I get a couple more patches in. One of which I
*thought* I would be doing right now. I'm trying to follow the order
CMike suggested.
>
>
>
>>* subversion/libsvn_fs/bdb/bdb-fs.h
>> New. This file contains table "open function" declarations removed
>> from their respective xxx-table.h include files. It also has
>>
>>
>
>Why just the open functions? That seems like a strange division. The other
>functions in the API are quite specific to the BDB implementation, so why
>didn't they move, too? Why not just put all of the prototypes into bdb-fs.h?
>
You know what .... I think you uncovered an artifact here. It just hit
me. Early on in my attempts to remove BDB dependancies I moved the open
declarations into bdb-fs.h so that I could include the xxx-table.h files
without having to include <db.h>
I continued to buy into the change(even after introducing a vtable)
because the xxx-table.h declarations collectively makeup the "backend"
vtable. Now I say SO WHAT!
NONE of these files are ever included above the bdb directory except in
some specifc fs tests. Hell thats only true when the backend vtable
comes in to play. For the current patch level, fs.h includes bdb-fs.h
which includes all the xxx-tables.h
So I think the right thing to do is revert *all* the changes for
xxx-table.h and xxx-table.c from this patch. That should work now and
later. I like that much better.
>
>I really don't understand the rationale for moving these. "but they'll go
>into a vtable" Okay, but wouldn't svn_fs__bdb_delete_nodes_entry have to go
>into a vtable, too? We can't have the FS calling svn_fs__bdb_* directly.
>Thus, *every* function in bdb/*.h would need to be in the vtable. Which sort
>of makes this move of the open stuff (seemingly) very arbitrary.
>
In a SQL DB you don't open tables. You open a DB or a schema/user.
It's all just different approaches to controling table access.
Soooo. Table opens are not part of the backend vtable. I moved the
open and create etc. from fs.c into bdb-fs where it can call the
specific opens without exposing the fact that it's doing it. A SQL open
function will get a connection instead. I may have to provide a cute
way to get additional connections to compensate for "ACID isolation
level" issues. Specifically to handle spying on other transactions. I
may have to add some more vtable magic to do this but I doubt I will
need to subtract anything.
It's all hooked in via the init_XXX_fs_vtab function. This may change a
little as we flesh out a more robust config system. A must have for me
by the way. I'm happy to do the work and answer to insightful posts
such as this:-)
>
>
>It would seem to be better to define the vtable, complete the discussion
>around *that*, add it to the code, and then start migrating stuff into it.
>
I tried that, months ago. I had two responses. One from you and one
from ghudson. And frankly they were very general responses. What I
posted was incomplete compared to the vtable in my current master patch
but it was quite sizable and was intended to get some conversation
going. That vtable was what I call the API or veneer layer vtable. You
had major problems with it, citing that there was quite a bit of code
that could be re-used. Reading below I see you still have problems with
it :-)
As a result, I dug into the code more and decided that you were right.
The problem is that most of the juicy stuff revolves around trails.
The trail bodies were being actively changed by CMike and others. So I
decided as a first cut to make no changes to the schema or the way it is
accessed. I would abstract the trails code and live within the current
trail bodies. IOW to simply to do what is done for BDB in SQL. This
will not be optimal for a SQL DB. I know. I don't intend to leave it
that way forever. I plan on using the API or veneer vtable to break off
pieces and optimize them as needed. Later I can merge them back into
the core implementation if possible. There is way more to it than that
of course. I want all these chunks to be dynamically loadable. I even
began work in *this* area. It's not quite ready to defend yet but it is
yet another piece of work in my bag. I *will* tell you that much of it
is centered around the use of an slightly extended svn_config_t. No
structure change, just more methods.
I hacked together a first version of the refactoring code to make sure
it could work.
Then I posted
http://www.cdrguys.com/subversion/fs_refactoring/FS_refactoring.htm.
I also posted http://www.cdrguys.com/subversion/sql_fs_docs/svn_fs_sql.htm.
Not great docs I admit. OK not even good. But they are a start.
Again, very little response. The people that did respond wanted to see
code. It made sense to me. Code is absolute. So I cleaned it up,
tested it, and posted what CMike called a Bomb blast of a patch. I met
with the collab guys *in* chicago to convince them I *am* serious about
doing this and to better explain my patch. I'm a much better verbal
communicator:-)
Doing this will help get the code I wrote last summer tied into
Subversion using real code and a real build system. Plus my test cases
were getting large enough that I wanted to avoid writing them twice.
Also, I want to make sure my abstracted SQL access code will play well
with the current access patterns. It's very DBI/JDBC like so it should
be fine.
>
>With the current approach, a lot of stuff is shifting around, but the
>reasoning isn't clear at all. And what that *really* means is that we have
>no way to evaluate the *design* of this vtable stuff.
>
>
>I'm highly suspect of the design right now. The FS is very dependent upon
>the BDB-based design. I'd like to see and understand how that strong
>coupling is going to be broken before changes like this happen. While I'm
>fine with a vtable for the database portions (the "backend"), I'm really not
>a fan of a vtable for the FS API itself.
>
Yes, I gather that:-) But it won't hurt anything unless it is abused.
It will allow people wanting to experiment with other approaches to do
so without hardwiring code all over the place. One hook in a loader and
a nifty init fucntion is all it will take. They can plug in pieces of
their code while using trunk code for the other pieces. That level
needs more work for sure. We decided to breakout 3 or 4 increments of
the existing patch and then figure what to do with the remaining API
vtable and friends. I'm simply not ready to give up on it yet. The
basic open, close, create type things don't belong in the backend table
IMNVHO :-) I'm not going down without a fight on this. There is also a
trail vtable is included as part of the API vtable. I think it needs to
be separate as well.
>I don't really see a need to change
>things at that level; the probability for changing the semantics is just too
>great, leading to problems down the line. Just how much rope do we want to
>provide?
>
I'm asking for enough to hang myself for sure. But I plan on swinging
from it. Not hanging on it.
I get this. I really do. But you are fooling yourself if you think the
semantics can't be broken in other ways. I feel like this problem can
be dealt with using stringent testing. I have suggested that (official)
alternative FSs must be tested using a common set of black box test
cases such that the results can be compared to the BDB implementation.
This should minimize semantic problems. You guys have similar issues
with the way different RA layers use the repos. What's a few more pan's
in the fire:-)
>
>
>
>>...
>> (svn_fs_set_berkeley_errcall, svn_fs_close_fs, svn_fs_recover_fs,
>> svn_fs_create_berkeley, svn_fs_open_fs, svn_fs_delete_fs): These
>> are now just wrappers around new functions in bdb-fs.c which contain
>> the old "guts" from these functions.
>>
>>
>Hunh? There is no svn_fs_close_fs().
>
Hey you noticed. So did I. It was a mistake in the log. I had a
close_fs function. I removed it in the more recent version of the
patch. Someone removed it's FS API counterpart on me. I wonder who
that was?
> rev 4451: gstein | 2003-01-19 22:29:13 -0500 (Sun, 19 Jan 2003) | 41
> lines
>
> Since the stack smasher was removed from svn_fs_close_fs(), the
> function did nothing. As a result, svn_repos_close() also did nothing
> (well, it cleared its local variable named "repos"... feh).
>
> Our desired model is that these objects are "closed" when their
> containing pool is destroyed. Getting rid of the close functions helps
> to clarify that.
While I *think* I understand what you are saying. An API which has an
open without a corresponding close is "highly suspect" :-)
BTW, can you explain to me why we need svn_fs_new vs having
svn_fs_create_berkeley and svn_fs_open__berkeley return the fs
structure. I'm assuming it's pool related?
gat
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Mar 6 14:06:45 2003