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

tree: introduce concept of "default" collation #56598

Merged
merged 1 commit into from
Nov 12, 2020

Conversation

otan
Copy link
Contributor

@otan otan commented Nov 12, 2020

Resolves #54989

Release note (sql change): Introduced a pg_collation of "default".
Strings now return the "default" collation OID in the pg_attribute
table (this was previously en_US). The "default" collation is also
visible on the pg_collation virtual table.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@otan otan force-pushed the default_collation branch 2 times, most recently from 6bf5d4a to 9cd47b1 Compare November 12, 2020 06:48
Release note (sql change): Introduced a pg_collation of "default".
Strings now return the "default" collation OID in the pg_attribute
table (this was previously en_US). The "default" collation is also
visible on the pg_collation virtual table.
@otan otan force-pushed the default_collation branch from 9cd47b1 to 530057b Compare November 12, 2020 16:42
@otan otan requested review from rafiss and a team November 12, 2020 16:51
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm -- but wondering about char and name

@@ -1358,14 +1358,14 @@ ORDER BY oid
oid typname typndims typcollation typdefaultbin typdefault typacl
16 bool 0 0 NULL NULL NULL
17 bytea 0 0 NULL NULL NULL
18 char 0 3903121477 NULL NULL NULL
19 name 0 3903121477 NULL NULL NULL
18 char 0 3403232968 NULL NULL NULL
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like char in PG has a typcollation of 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, i think this is a separate issue in that this is what pg_type reports, which is different.
i think that's a separate issue. we currently base this column on the collation of the type...

18 char 0 3903121477 NULL NULL NULL
19 name 0 3903121477 NULL NULL NULL
18 char 0 3403232968 NULL NULL NULL
19 name 0 3403232968 NULL NULL NULL
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name has a collation of C in PG

???

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah i think that makes sense as name is an internal thing isn't it? either way we don't have C, so i think we can leave this as is

// To most behave like postgres, set the CollatedString type if a non-"default"
// collation is used.
if locale != DefaultCollationTag {
_, err := language.Parse(locale)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so i guess here would be the place where we could normalize the collation name using lex.EncodeLocaleName. do you think we should do that?

(either in this PR, or at any point?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, as well as change all language.Parse calls to ensure it does this appropriately...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that sounds about right. maybe somewhere with alter column type as well.
maybe in general we shouldn't use language.Parse but write our own wrapper around it.

@otan
Copy link
Contributor Author

otan commented Nov 12, 2020

char and name seems like an existing problem based on the type descriptor -- so i'm going to ignore this for now.

https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/pg_catalog.go#L2276

going to leave this to another issue.

TFTR!

bors r=rafiss

@craig
Copy link
Contributor

craig bot commented Nov 12, 2020

This PR was included in a batch that was canceled, it will be automatically retried

@craig
Copy link
Contributor

craig bot commented Nov 12, 2020

Build succeeded:

@craig craig bot merged commit a5e67b6 into cockroachdb:master Nov 12, 2020
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.

sql: pg_attribute table lacks the notion of a 'default' collation
3 participants