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

feat(postgres): allow customizing client_min_messages #10448

Merged
merged 1 commit into from Feb 15, 2019

Conversation

2 participants
@gabegorelick
Copy link
Contributor

gabegorelick commented Feb 14, 2019

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

This comment has been minimized.

Copy link
Contributor Author

gabegorelick commented Feb 14, 2019

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

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.