-
Notifications
You must be signed in to change notification settings - Fork 85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TT-13421 create indexes on sharded sql pumps #861
Conversation
@sredxny since we're using |
pumps/sql.go
Outdated
return errors.New("cannot create index for non existent column " + column) | ||
} | ||
|
||
sql := fmt.Sprintf("CREATE INDEX %s IF NOT EXISTS %s ON %s (%s)", option, indexName, tableName, column) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better naming for this variable could be query
, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to this, let's bring in practise to be careful when forming SQL queries. I know the values in this query are totally under our control (consts), but still I feel like it's better to use an sql builder. https://gorm.io/docs/sql_builder.html - this might help
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed.... @jeffy-mathew in such case we would need to add some logic like:
query := "CREATE INDEX IF NOT EXISTS ? ON ? (?)"
if option != "" {
query = fmt.Sprintf("CREATE INDEX %s IF NOT EXISTS ? ON ? (?)", option) // Add option dynamically
}
As the original query contains a param "option" that can be "concunrrently" or empty, and db.Raw method doesnt allows it and will mark it as bad query
/release to release-1.11 |
Working on it! Note that it can take a few minutes. |
* started to add indexes to shards * create indexes when SQL pump is sharded * added test for ensure index in SQL pump * update sql aggregate to handle multiple indexes on sharded analytics * update SQL aggregate test * isolate index build logic * some refactor * fixing test for indexes in SQL pump * gofmt sql_test.go * check drop table err * gofumpt * ensuring table for SQL * gofumpt * refactoring ensureIndex in sql pump * improve logging * refactor logic while looping over indexes * gofumpt * only use the waitgroup for background --------- Co-authored-by: sredny buitrago <[email protected]> (cherry picked from commit 544ccb3)
@sredxny Seems like there is conflict and it require manual merge. |
* started to add indexes to shards * create indexes when SQL pump is sharded * added test for ensure index in SQL pump * update sql aggregate to handle multiple indexes on sharded analytics * update SQL aggregate test * isolate index build logic * some refactor * fixing test for indexes in SQL pump * gofmt sql_test.go * check drop table err * gofumpt * ensuring table for SQL * gofumpt * refactoring ensureIndex in sql pump * improve logging * refactor logic while looping over indexes * gofumpt * only use the waitgroup for background --------- Co-authored-by: sredny buitrago <[email protected]> (cherry picked from commit 544ccb3) Co-authored-by: Sredny M. <[email protected]>
Description
Related Issue
Motivation and Context
How This Has Been Tested
Screenshots (if appropriate)
Types of changes
Checklist
fork, don't request your
master
!master
branch (left side). Also, you should startyour branch off our latest
master
.go mod tidy && go mod vendor
go fmt -s
go vet