General IndexCatalog implementation

[ajnavarro]

[ajnavarro]

Instead of having IndexLoader and an IndexSaver, we can have an IndexDriver having all the logic of both interfaces into one.

IndexCatalog can be private, and instead of registering Indexes, we can register that IndexDrivers.

We also need a method that will be called when an Index is being deleted.

[erizocosmico]

IndexCatalog cannot be private, you need to retrieve indexes from the catalog

[erizocosmico]

After thinking a bit more about this, I think we should make a few changes to make this work correctly.

Create driver method

Right now, as we agreed, we have the IndexDriver interface which has the methods ID, Load, Save and Delete. Perhaps it's worth it to add the Create(id string, table Indexable, expr Expression) (Index, error) method to that interface or change Save to Save(id string, table Indexable, expr Expression, path string) or something like this. I prefer adding the Create method instead of mixing that into the Save method just because we can perfectly define boundaries between those functionalities and make sense as separate methods.

Having the Index is not very useful in the Save method. For one, the Index needs to have a concrete implementation and that should be provided by the driver. The generic execution of the create index query won't be able to create specific indexes if this is not on the driver interface.
Besides, in the query we already have the table and the expressions all resolved. Passing the index would mean having to look it up again.

So, the flow would be like this:

CREATE INDEX query -> Tree is resolved -> Tree execution -> Take driver name from the query and get the driver -> create the index using the driver -> add it to the catalog -> spawn a background job to create the index and mark it as ready when it's done.

Simplification of the indexes loading logic

Also, I think we can simplify the logic for loading the indexes on startup with this:

Instead of storing the indexes in a folder that is the name of the index we might do use ${DRIVERNAME}__${INDEXNAME}. That way, we always know the index driver from the name of the folder and there is no need to store it elsewhere in a config file. This simplifies the process of figuring out which loader we should use to load the index.

This way just listing the directories of the indexes path we can gather a full list of indexes with their drivers, which is easier and simpler than reading config files. If the index needs a config file, it's up to them afterwards.

Other questions

Is every driver in charge of making the index creation fault tolerant? What happens if the server is stopped or crashes before we're done creating the index? What should happen if there are some partial files in the index because of that?

Do we need any mechanism to keep track of indexes that are still being created? Like for waiting until they are before stopping the server, etc.

Should we make Load and Save accept a context.Context? Maybe for Load it's ok not to do it but for Save maybe it is.

[ajnavarro]

Is every driver in charge of making the index creation fault tolerant?

Yes, because every index implementation can be really different from others, and need several ways to avoid index corruption.

What happens if the server is stopped or crashes before we're done creating the index?

The index implementation should know that it has trash from an unfinished index creation and delete them.

What should happen if there are some partial files in the index because of that?

The same as before, the index implementation should know if a process was incomplete before, and delete that incomplete files.

Do we need any mechanism to keep track of indexes that are still being created?

We can have a Disabled()bool method on Index interface to check before use it. This method can be used to avoid queries to use it on Index creation and also to avoid queries to use it when the index is being deleted.

Summarizing, the methods on IndexDriver will be:

• Create(ID, Expression, config map[string]string)(Index,error)
• Save(context.Context, path, Index, IndexKeyValueIterator) error (the logic to get IndexValueIterator is always the same, we don't need to delegate that logic to the IndexDriver implementation. Also this method can be called on a background task.)
• Load(path)(Index,error)
• ID()string to get the unique ID of that driver

WDYT?

[erizocosmico]

Is the ctx on Create a context.Context or a sql.Context?
Isn't the Save method missing the path?

We can have a Disabled()bool method on Index interface to check before use it. This method can be used to avoid queries to use it on Index creation and also to avoid queries to use it when the index is being deleted.

I didn't mean that. We already have the status to know if it's being created or not. What I meant is: we need to keep track of how many queries are using an index. Do we refcount index usage or something?

Like, Q1 and Q2 are using IDX1 and while they're being executed IDX1 is marked for deletion. How do we know we should delete IDX1 when Q1 and Q2 finish?

[erizocosmico]

Also, Load may need access to the database to get the table from it.

Maybe it's also nice if the Index had a Table and Database method to return those quickly? That would simplify some things a little bit too.

Create would also need the path. After you get the index from create you should be able to use Get and Has, which needs to read stuff from disk, so it will need the path to the root of the index storage.

[ajnavarro]

Removed ctx from Create and added to Save. The context should be a context.Context. Also added config map[string]string to be able to send to the index random needed configuration (in our case, pilosa url as an example).

Yes, you are right, we need a way to track the number of queries that are using an index. Doing refcount is a good option.

I Added the missing path to Save.

Why Load need the table? Load loads the index from disk.

[erizocosmico]

How can we pass config to the Create method? That will be called from the execution of the create index, which is general. Shouldn't the config be passed to the driver constructor already?

What about the database and the path in the Create method?

WDYT about something like this?

type Index interface {
	Get(key interface{}) (IndexLookup, error)
	Has(key interface{}) (bool, error)
	ID() string
	Expression() Expression
	Status() IndexStatus
	Database() string
	Table() string
}

type IndexDriver interface {
	ID() string
	Create(path, id, db string, expr Expression, config map[string]string) (Index, error)
	Load(path string) (Index, error)
	Save(ctx context.Context, path string, idx Index, iter IndexKeyValueIter) error
	Delete(ctx context.Context, path string, idx Index) error
}

EDIT: whole db instance is not needed.
EDIT2: added config