sql: delete index if there is an error when it's added to the registry

[mcarmonaa]

Related to src-d/gitbase#411

This is the simplest solution. If we want to avoid create the index if it can't be added to the registry, we must mix a little bit the create index and add index logic to rely on the IndexRegitry.AddIndex method to create the index instance. WDYT @src-d/data-retrieval ?

[kuba--]

Ok, it may work because in pilosa we have frames/fields as a combination of expression AND id

[kuba--]

Lets add test for this case

[mcarmonaa]

@kuba-- TestAddIndex is already testing it.

Maybe we will need to add an integration test in gitbase instead when this is merged and updated there.

[kuba--]

@mcarmonaa - I think there is no need to add it to gitbase, but
https://github.com/src-d/go-mysql-server/blob/master/sql/index_test.go#L138
just tries to add exactly the same index. I thought about low level test, where you try to add an another index with different id but the same expression(s). When it fails the index will be deleted but we should still have access to the previous/old one.

[mcarmonaa]

@kuba-- https://github.com/src-d/go-mysql-server/blob/master/sql/index_test.go#L131 is adding an instance of a new index with a different id

[ajnavarro]

I would prefer do not create a valid index folder when Create() method is called. We can create the .processing file at that moment. Then, we will throw an error saying that another index is using the same expression. After that, if we restart gitbase, the invalid index will be automatically deleted. WDYT?

[kuba--]

@mcarmonaa - ok, you're right (my bad). so maybe we can keep the old instance and again try to load the index.

@ajnavarro - so far .processing is created in Save. It's not a problem to move it to Create, but it means:

• All created indexes at the beginning will be invalid (til you save something).
• Processing will not be atomic operation, because we will create .processing in one function (Create) and remove it in other function (Save) after saving.

[mcarmonaa]

so maybe we can keep the old instance and again try to load the index.

👍

All created indexes at the beginning will be invalid (til you save something).

Is it an index useful if you don't save something on it?

[kuba--]

@mcarmonaa - it's just an empty index (no data)

I just wondering if it's possible no to create any index, I mean just compare expressions in potential new index with existing indexes expressions. Instead of first create and later validate

[mcarmonaa]

@ajnavarro @kuba-- Which solution will we go ahead with finally? the .processing file one?

[ajnavarro]

@mcarmonaa If it is possible to compare before the Save() method if the expression is already indexed, do it.

If not, create the processing file on Save().

I would prefer the first approach.

[kuba--]

What if we modify func (r *IndexRegistry) validateIndexToAdd(idx Index) error to sth, like:
func (r *IndexRegistry) ValidateIndex(db, table string, expr []sql.Expression) error.
After that we can call it in func (c *CreateIndex) RowIter(ctx *sql.Context) (sql.RowIter, error) instead of c.Catalog.AddIndex(index)

c.Catalog.IndexRegistry.ValidateIndex(db, table, expr)

WDYT?

[kuba--]

Generally we can split current functionality c.Catalog.AddIndex(index) by Validate and Add
We can even (just in case) leave current validation in AddIndex but add/make public a new one (lower level) which we can call before AddIndex

[kuba--]

...but if it's too complicated, lets go with .processing approach. The only thing which I would suggest to add is, maybe we can add sth. to the file (append content) like a progress counter, so we'll know if it's just created index, saving or failed during saving, etc.

[mcarmonaa]

I have implemented something similar to what you say @kuba--, the problem I see is that the process validate -> create -> add should be atomic, shouldn't be it?

So we need something like:

func (r *IndexRegistry) CreateAndAddIndex(
	driver IndexDriver,
	db, table, id string,
	expressions []Expression,
	config map[string]string,
) (idx Index, created chan<- struct{}, ready <-chan struct{}, err error)

WDYT @ajnavarro @kuba-- ?

[kuba--]

imo, each of them are atomic operation (just to test it out) and CreateAddIndex is just a wrapper on top of it.

[mcarmonaa]

Ok, let's with the .processing file approach then.

I'll close this PR and implement it in another one.