Skip to content

Conversation

@gabegorelick
Copy link
Contributor

Pull Request check-list

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

For some reason, Sequelize sets client_min_messages to warning when connecting to Postgres. This was introduced in #3041 with the only justification being "There is a new SET that increases the message level to warning (up from notice) since the upgraded pg client is now capable of bubbling these messages."

pg does indeed allow you to register an event when the server returns a message, but it doesn't follow that Sequelize needs to override the default client_min_messages.

Furthermore, this behavior accidentally became coupled to the keepDefaultTimezone setting in 95a5c0d. When keepDefaultTimezone is true, client_min_messages is not overridden.

This behavior has been in place so long that it may be safest to keep client_min_messages set to warning by default. But we can at least allow users to customize it and decouple it from keepDefaultTimezone.

If we want to make a riskier change, we can set client_min_messages to null by default and have Sequelize default to not updating the DB's setting. I have not done that in this PR, but it's an easy change if we want to make it.

@gabegorelick
Copy link
Contributor Author

Failing test is due to a MySQL deadlock. Almost certainly a flaky test and not caused by these changes in the Postgres connection manager.

@sushantdhiman sushantdhiman merged commit c463314 into sequelize:master Feb 15, 2019
@gabegorelick gabegorelick deleted the client_min_messages branch May 16, 2019 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants