Skip to content
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

Execute alterTable statements in order #227

Closed
wants to merge 1 commit into from

Conversation

ataft
Copy link
Contributor

@ataft ataft commented Mar 26, 2020

async.each() in alterTable executes the statements in parallel, allowing for things like creating an index before dropping it. Change async.each to async.eachSeries in order to execute the statements in proper order.

Fixes #180

async.each() in alterTable executes the statements in parallel, allowing for things like creating an index before dropping it. Change async.each to async.eachSeries in order to execute the statements in proper order.
@slnode
Copy link

slnode commented Mar 26, 2020

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@dhmlau
Copy link
Member

dhmlau commented Mar 27, 2020

@strongloop/sq-lb-apex , there's an existing PR loopbackio/loopback-connector#168. If it's fixed in loopback-connector, do we still need it for particular connector? Thanks.

@ataft
Copy link
Contributor Author

ataft commented Mar 27, 2020

Yes, we need it because the .each logic is in the alterTable function of this connector, which is called by autoupdate in loopback-connector.

Also, many of the connectors override automigrate/autoupdate from loopback-connector. In many cases, each vs. eachSeries isn't critical because it's just the order of the models. In this case, it's critical because it's the order of SQL statements. For example, an index has to be dropped before it's created.

@dhmlau
Copy link
Member

dhmlau commented Mar 27, 2020

@ataft, thanks for your contribution. Could you please add some tests to verify your changes? There's a similar PR: loopbackio/loopback-connector#170 which can be used as your reference.

Also, please fix the commit linter error as well:

**************************************************
**
**  Linting commit logs
**
**  1 problems found:
**    3daa474 - Execute alterTable statements in order: line 3 longer than 72 characters.
**
**************************************************

Thanks!

@dhmlau dhmlau added the community-contribution Patches contributed by community label Mar 27, 2020
@ataft
Copy link
Contributor Author

ataft commented Mar 27, 2020

@dhmlau the current test covers this scenario because it does an automigrate (create the table) and then an autoupdate, which will call alterTable. BTW, this test should be failing intermittently, but given that the order is random it'd be hard to catch.

Regarding the commit linter error, what exactly is line 3? I only made one simple change to line 343.

@dhmlau
Copy link
Member

dhmlau commented Mar 27, 2020

@ataft, the error is about the commit message, not the actual change. It seems like there are 4 lines of commit message?

Execute alterTable statements in order

async.each() in alterTable executes the statements in parallel, allowing for things like creating an index before dropping it. Change async.each to async.eachSeries in order to execute the statements in proper order.

@ataft
Copy link
Contributor Author

ataft commented Mar 27, 2020

Yeah, not sure how to do that. There is something called word wrap that's pretty great, it wraps text so people can read it. Submitted new PR: #228

@ataft ataft closed this Mar 27, 2020
@dhmlau
Copy link
Member

dhmlau commented Mar 28, 2020

Thanks. As a future reference, you can also amend the commit message by following the instructions here: https://help.github.com/en/github/committing-changes-to-your-project/changing-a-commit-message

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution Patches contributed by community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autoupdate error: index or statistics already exists on table
3 participants