Skip to content

FIX: init db with C locale and UTF-8#38706

Merged
MikeMcQuaid merged 2 commits intoHomebrew:masterfrom
SamSaffron:patch-1
Apr 12, 2019
Merged

FIX: init db with C locale and UTF-8#38706
MikeMcQuaid merged 2 commits intoHomebrew:masterfrom
SamSaffron:patch-1

Conversation

@SamSaffron
Copy link
Contributor

Without this fix fulltext on the db behaves strange due to:

https://stackoverflow.com/a/24541592

Without this fix fulltext on the db behaves strange due to:

https://stackoverflow.com/a/24541592
@zbeekman
Copy link
Contributor

zbeekman commented Apr 9, 2019

Syntax errors and you clearly didn't try to run this locally first. Closing. Feel free to resubmit and follow the PR template.

@zbeekman zbeekman closed this Apr 9, 2019
@SamSaffron
Copy link
Contributor Author

@zbeekman fair enough I had one extra comma there at the end I will ask someone from my team to follow this up and run this on local.

It generated regular support requests for us at Discourse.

@zbeekman zbeekman reopened this Apr 9, 2019
Co-Authored-By: SamSaffron <sam.saffron@gmail.com>
@SamSaffron
Copy link
Contributor Author

@zbeekman thank you! I applied the change.

@zbeekman
Copy link
Contributor

@BrewTestBot test this please

@apjanke
Copy link
Contributor

apjanke commented Apr 11, 2019

This might have more wide-ranging affects, including collation and case conversion operations on text data in that database. Maybe a review of that and some testing is in order before flipping the switch on this? Since we don't know the use cases of all Homebrew users here, it's probably worth considering all its ramifications.

@zbeekman zbeekman added the maintainer feedback Additional maintainers' opinions may be needed label Apr 11, 2019
@zbeekman
Copy link
Contributor

@Homebrew/maintainers Thoughts on this?

@MikeMcQuaid
Copy link
Member

I'd like to see a bunch more people request this before we change the default. A lot of people will assume the previous default here.

@zbeekman zbeekman mentioned this pull request Apr 11, 2019
@claui
Copy link
Contributor

claui commented Apr 11, 2019

@apjanke While your point is valid, this change isn’t ever going to touch an existing database, or is it?

@MikeMcQuaid This change is going to align collation behaviour with user expectations. If we want to stick with creating the database cluster on the user’s behalf, this PR would be the Right Thing to do for 99.9% of users.

Everyone else can be reasonably expected to be well aware of their specific use case, and is free to re-create the database cluster themselves.

I’d love for at least one more maintainer to chime in though – just in case I’m missing something really obvious here!

@sjackman
Copy link
Contributor

Here's an article on this topic in favour of using initdb --locale=C.
https://justatheory.com/2004/08/postgres-always-use-c-locale/

@claui
Copy link
Contributor

claui commented Apr 11, 2019

Thanks @sjackman, good to know!

@MikeMcQuaid
Copy link
Member

This change is going to align collation behaviour with user expectations. If we want to stick with creating the database cluster on the user’s behalf, this PR would be the Right Thing to do for 99.9% of users.

@claui Do you/have you used PostgreSQL for "real" projects? If so: I 100% defer to you here.

@SamSaffron
Copy link
Contributor Author

We do manage about 2000 or so Discourse sites including this one https://discourse.brew.sh/ 😸

We use Postgres and have been using it ever since we started the project, the way PG requires you init with C locale is a huge curve ball for contributors who get confused that they created a UTF-8 site yet for some reason full text does not work right. Debian / Ubuntu default to this Arch mentions this in the sample initdb command.

I agree though 100% you got to be prudent here.

@MikeMcQuaid
Copy link
Member

@SamSaffron lol! Works for me then!

@MikeMcQuaid MikeMcQuaid dismissed zbeekman’s stale review April 12, 2019 08:37

Changes justified.

@MikeMcQuaid MikeMcQuaid merged commit 95657f0 into Homebrew:master Apr 12, 2019
@MikeMcQuaid
Copy link
Member

Thanks so much for your first contribution! Without people like you submitting PRs we couldn't run this project. You rock, @SamSaffron!

willbowditch added a commit to willbowditch/homebrew-core that referenced this pull request Apr 12, 2019
…to cypher-shell

* 'master' of https://github.com/Homebrew/homebrew-core: (69 commits)
  postgresql: init db with C locale and UTF-8 (Homebrew#38706)
  rust: update 1.34.0 bottle.
  rust 1.34.0
  xcodegen: update 2.5.0 bottle.
  xcodegen 2.5.0
  imagemagick: update 7.0.8-39 bottle.
  imagemagick 7.0.8-39
  glooctl: update 0.13.14 bottle.
  glooctl 0.13.14
  wtf 20190410
  unrar: update 5.7.4 bottle.
  unrar 5.7.4
  pixman: update 0.38.4 bottle.
  pixman 0.38.4
  jid: update 0.7.6 bottle.
  jid 0.7.6
  ipv6calc: update 2.1.0 bottle.
  ipv6calc 2.1.0
  hyperscan: update 5.1.1 bottle.
  hyperscan 5.1.1
  ...
@SamSaffron
Copy link
Contributor Author

SamSaffron commented Apr 12, 2019

Hi Mike, Thank you! I always enjoy using Homebrew on the Mac and think you and the Homebrew team do a fantastic job. If any issues arise due to this patch please @mention me and I will do my best to help out.

@MikeMcQuaid
Copy link
Member

Will do @SamSaffron! Thanks again for the PR and for (our) Discourse!

@lock lock bot added the outdated PR was locked due to age label May 12, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

maintainer feedback Additional maintainers' opinions may be needed outdated PR was locked due to age

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants