apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <li...@rowe-clan.net>
Subject Re: sdbm locks, second try
Date Sat, 05 May 2001 13:06:35 GMT
Anyone who might care to work with apr_sdbm databases, please jump forward
two paragraphs, and contribute your preference between options 1 and 2.

From: "Greg Stein" <gstein@lyra.org>
Sent: Saturday, May 05, 2001 2:38 AM

> On Fri, May 04, 2001 at 12:41:46AM -0500, William A. Rowe, Jr. wrote:
> > From: "Greg Stein" <gstein@lyra.org>:
> >...
> > Yes.  I don't believe there is any reason to promote more 'errno/h_errno' style
> > entities (c.f. OS2's approach to the world.)  I'd be fine with doing away with
> > an error tracking mechanism _we_ never retest.
> Can we do this first to simplify the next steps? As I mentioned, your
> patches are already needing to deal with the ioerr() stuff. And the
> introduction of get_err and clear_err was kind of a mistake given that you
> did the Right Thing with returning the status codes in the first place.

Yes.  If you don't patch by tonight, I'll get to it then.

> Okay... so let's say that the app can (externally) establish the lock. And
> that the app should be able to establish either kind: shared (read only) or
> exclusive (read/write).
> ...
> > > The *current* apr_sdbm_(un)lock functions would be
> > > renamed and kept private -- the "exclusive" flag is for internal use. 
> > 
> > Ok, so you are suggesting we leave things 'simple' internally (no extra lock
> > if it's already locked?)
> So... the simple approach. Record the type of lock: shared/exclusive. Record
> the number of times that type was locked. Bail if some requests an exclusive
> lock and all we have is shared (i.e. no promotion). Asking for shared when
> an exclusive is held is fine.
> The reason for refcounting: the app is going to call apr_sdbm_lock(). And
> then apr_sdbm_store() is going to call it, too. We don't want the second to
> fail. And we don't want apr_sdbm_store's unlock to toss what the app did.

Here's my problem with the contraditions here, and why I'm entirely -1 on
giving the user a choice of locks, and allowing nested locks.  The comments
above are mutually exclusive.

It's entirely difficult for the user to debug the fact that _someone_ (themself,
a library call, or something else) grabbed a shared lock, and then within their
apparently simple and proper code, to figure out why they fail when trying 
to write the record.

So _if_ they want something simple (from the _programmer's_ perspective) that
is predictable (also from their perspective) we need to either:

  1. all locks are exclusive when the dbm is opened read/write (and shared for
     readonly), giving the user no choice.  Offer a refcount to track nested locks.

  2. allow the user to choose a lock (shared or exclusive), keep the implicit 
     locking (on fetch or store), but give up on the refcount.

Frankly, I'm +1 on option 2, but I'd bend to option 1 based on concensus.

> > > The external function just calls it with the "exclusive" flag enable. The lock
> > > function can do two things: record a refcount, and record the type (so we
> > > can fail if an exclusive was attempted, but a shared already exists (the
> > > promotion thing you mention)).
> > 
> > I think I have a simpler suggestion that I hate.  If it's going to come down
> > to "We Must Have Refcounted Locks" (not necessarily a bad thing, but new to any
> > apr entity) then perhaps your suggestion not to always lock exclusive applies
> > to files that are opened for r/w.  If the dbm is opened r/o, then every lock
> > is a shared lock.  That would sort of lick all the problems at once.
> I'm not sure what you're saying here. I'm talking about refcounting in SDBM,
> *NOT* in the apr_file or locking code.

I was anticipating my objections I just reiterated above.

> > I personally prefer caution on the part of the user, and not refcounting locks
> > (if the user wants to wrap the lock/unlock fn with refcounting themselves, then

> > great!)  This allows a r/w opened file to have either type of lock, so they can
> > Sharelock what they want to check up on, and excllock when they need to look up,
> > make a judgement/calculation, and write back their opinion.
> The user can't do this properly. If they have a "big lock" that they wrap
> around all the operations, the first store() is going to toast their lock on
> exit. The locks need to be refcounted.

HUH?  Read the patch.  It does no such thing.  It decided if it needs an implicit
lock, grabs it and releases it.  If it doesn't grab a lock, it doesn't release a
lock.  Boolean refcounting.  Very simple.

> ... So we should do the cache wipe at unlock time. We still
> have to consider the case of a read that decides it doesn't have to hit the
> disk, so it doesn't bother to take out a lock. Since it never goes for a
> lock, we can't toss the cache at lock time.

Programatically it doesn't matter.  If you prefer it at unlock that's fine.

View raw message