Re: [PATCH 12/13] odb/source-files: move alternates into the backend

[Patrick Steinhardt <ps@pks.im>]

On Tue, Oct 06, 2026 at 01:51:33PM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Originally, when designing pluggable object databases the goal was that
> > the object database can have multiple sources, and every source attached
> > to it could use a different backend. This would have allowed for quite a
> > lot of flexibility, as you could trivially mix and match different kinds
> > of object storages in whatever way you like.
> >
> > But while well-intentioned, this design led to a bunch of conceptual
> > problems:
> >
> >   - We're now trying to read objects in source order, whereas we
> >     previously tried to read objects via packfiles before trying to read
> >     them via loose objects. This led to a performance regression when
> >     using alternates or when using a quarantine directory.
> 
> Could the design be instead to use a mapping function which allows us to
> map objects to sources, based on some characteristics of the object?

You could, but it adds complexity that only needs to exist because of
the needs of the "files" backend. Ideally though, we'd not be leaking
internal implementation details of specific backends into callers and
have the interfaces be as agnoics as possible.

> >   - Some data structures are supposed to only ever exist once, like for
> >     example bitmaps and commit graphs. At the same time, those data
> >     structures also span across the union of all objects, so they may
> >     cross sources.
> 
> This is not really a problem for having multiple sources though.

Not necessarily, but it makes it extremely awkward. The sources now need
to reach into the other sources and be aware of them, and that is a huge
design smell. I've tried multiple times to squeeze these data structures
into the design, but everything single time the result was atrocious.

[snip]
> > In short, there are a bunch of conceptual mismatches when we have
> > alternates and pluggable object databases coexist. So while the original
> > idea was nice, it does not result in a system that is easy to reason
> > about.
> >
> > Correct course by moving alternates into the "files" source itself so
> > that it becomes an implementation detail thereof so that we can avoid
> > all of these shortcomings. While it's unfortunate that we cannot easily
> > mix and match sources now, that ability doesn't go away. It's still very
> > much feasible to introduce a new backend that allows for exactly that
> > use case, and such a backend may also be a lot more flexible as we can
> > now add new logic to determine which objects should be stored where. So
> > the original motivation for having per-source backends can still be
> > realized with the new architecture.
> >
> 
> Okay, this makes sense, so the new source could be merged source of some
> sorts, with internal logic which it uses to map to different sources.
> Nice.

Yes, exactly. And such a design would also have three important benefits:

  - We can start from scratch and be sure that such a filtering system
    is well defined instead of trying to shoehorn this into the object
    database somehow.

  - The design can be a lot more flexible because we start from scratch,
    and it can easily have configuration to fine-tune things.

  - The logic to handle this would be entirely self-contained in such a
    backend, and its design details would not have to leak into callers.

The only downside is that we'd have to have another backend specific to
such a thing. But I'd rather have a backend specifically designed for
this that is entirely self-contained compared to having to support such
a feature with code cluttered around our object subsystems.

It took me a while to realize this myself though.

> > diff --git a/midx.c b/midx.c
> > index c0f82c4163..8638ddf0be 100644
> > --- a/midx.c
> > +++ b/midx.c
> > @@ -829,21 +829,15 @@ void clear_incremental_midx_files_ext(struct odb_source_packed *source, const ch
> >
> >  void clear_midx_file(struct repository *r)
> >  {
> > -	struct odb_source_files *files;
> > +	struct odb_source_files *files = odb_source_files_downcast(r->objects->source);
> 
> We remove the the previous `if(r->objects)` check here, is that okay?

Yes, it is. There's only a single caller, and that caller
unconditionally dereferences `r->objects` already. And we also
dereference that pointer a bit further down in this same function here.
So the check was giving a false sense of security anyway, and we're
basically just moving up the unconditional dereference of the pointer
now.

Thanks!

Patrick