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

x/pkgsite: test databases don't override LC_COLLATE and LC_CTYPE #40347

Closed
akolar opened this issue Jul 22, 2020 · 3 comments
Closed

x/pkgsite: test databases don't override LC_COLLATE and LC_CTYPE #40347

akolar opened this issue Jul 22, 2020 · 3 comments
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. pkgsite
Milestone

Comments

@akolar
Copy link

akolar commented Jul 22, 2020

What is the URL of the page with the issue?

n/a

What is your user agent?

n/a

Screenshot

n/a

What did you do?

Run ./all.bash while using a development PostgreSQL database.

Minimal example below:

docker run --rm -d \
    -P -p 127.0.0.1:5432:5432 \
    -e POSTGRES_PASSWORD="$GO_DISCOVERY_DATABASE_TEST_PASSWORD" \
    -e LANG=en_US.UTF-8  \
   --name pg postgres:latest && \
go clean -testcache && \
./all.bash

Note that LANG=en_US.UTF-8 which means that template1 will use en_US.UTF-8 for its collation since initdb uses whatever is set in $LANG as a default value (and for non-production systems, it is reasonable to assume that $LANG is not C).

Unlike discovery-db created in https://github.com/golang/pkgsite/blob/master/devtools/create_local_db.sh, dbtest.CreateDBIfNotExists() does not override template/locale used by the CREATE DATABASE statement. This means that the two are carried over from template1 (which may or may not use C locale).

Listing all databases returns the following:

postgres=# \l
                                          List of databases
            Name            |  Owner   | Encoding |   Collate   |    Ctype    |   Access privileges   
----------------------------+----------+----------+-------------+-------------+-----------------------
 discovery-db               | postgres | UTF8     | C           | C           | 
 discovery_frontend_test    | postgres | UTF8     | en_US.UTF-8 | en_US.UTF-8 | 
 discovery_integration_test | postgres | UTF8     | en_US.UTF-8 | en_US.UTF-8 | 
 discovery_postgres_test    | postgres | UTF8     | en_US.UTF-8 | en_US.UTF-8 | 
 discovery_worker_test      | postgres | UTF8     | en_US.UTF-8 | en_US.UTF-8 | 
 postgres                   | postgres | UTF8     | en_US.UTF-8 | en_US.UTF-8 | 
 template0                  | postgres | UTF8     | en_US.UTF-8 | en_US.UTF-8 | =c/postgres          +
                            |          |          |             |             | postgres=CTc/postgres
 template1                  | postgres | UTF8     | en_US.UTF-8 | en_US.UTF-8 | =c/postgres          +
                            |          |          |             |             | postgres=CTc/postgres
(8 rows)

(discovery_*_test databases have collate and ctype en_US.UTF-8).

The behaviour described above is extremely flakey (for example, when the example is run on postgres:alpine instead of postgres:latest, tests pass). Related: https://www.postgresql.org/message-id/flat/23053.1337036410%40sss.pgh.pa.us#23053.1337036410@sss.pgh.pa.us

I think that the documentation should either make it clear that template1 must use C as its locale or that we should fix dbtest.CreateDBIfNotExists() and postgres.recreateDB() so that they create databases the same way ./devtools/create_local_db.sh does.

What did you expect to see?

All tests pass.

What did you see instead?

TestPostgres_GetTaggedAndPseudoVersions/want_releases_and_prereleases_only failed because entries were out of order.


Unless you have other plans with this, I'm more than happy to create a CL since most files that need changing are already identified above.

@gopherbot gopherbot added this to the Unreleased milestone Jul 22, 2020
@toothrot toothrot added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. pkgsite labels Jul 22, 2020
@toothrot
Copy link
Contributor

/cc @julieqiu

@julieqiu
Copy link
Member

Thanks, @akolar! Fixing dbtest.CreateDBIfNotExists() and postgres.recreateDB() makes sense to me - feel free to send a CL.

@gopherbot
Copy link

Change https://golang.org/cl/244517 mentions this issue: internal: set collation and ctype on test databases

@golang golang locked and limited conversation to collaborators Jul 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. pkgsite
Projects
None yet
Development

No branches or pull requests

4 participants