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.

Sign up for GitHub

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

Jump to bottom

sql: cast to name should truncate to at most 63 characters #107895

Closed
rafiss opened this issue Jul 31, 2023 · 2 comments · Fixed by #108811
Closed

sql: cast to name should truncate to at most 63 characters #107895

rafiss opened this issue Jul 31, 2023 · 2 comments · Fixed by #108811
Assignees
@msirek
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. C-test-failure Broken test (automatically or manually discovered). O-rsg Random Syntax Generator T-sql-queries SQL Queries Team

Comments

@rafiss
Copy link
Collaborator

rafiss commented Jul 31, 2023 •
edited by cockroach-jira-scripts
Loading

Postgres has the following behavior:

postgres=# select length(repeat('a', 70)::name);
 length
--------
     63

CockroachDB does not:

root@localhost:26257/defaultdb> select length(repeat('a', 70)::name);;
  length
----------
      70

This should match Postgres. I'm not sure how to think of backwards compatibility here (though regardless, it should be documented as a backwards-incompatible change).

Discovered through randomized testing: #82867 (comment)

Jira issue: CRDB-30248
@rafiss rafiss added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. C-test-failure Broken test (automatically or manually discovered). A-sql-pgcompat Semantic compatibility with PostgreSQL O-rsg Random Syntax Generator T-sql-queries SQL Queries Team labels Jul 31, 2023
@github-project-automation github-project-automation bot moved this to Triage in SQL Queries Jul 31, 2023
@rafiss rafiss mentioned this issue Jul 31, 2023
Closed
@mgartner
Copy link
Collaborator

mgartner commented Aug 1, 2023

AFAICT our documentation does not mention the NAME type. The Postgres docs mention that the NAME type is "not intended for general-purpose use, only for use in the internal system catalogs"1. So I think we should only consider the backwards compatibility of any internal tables or queries involving the NAME type, though I'm not sure how to best discover all these cases and determine if this change would break them.

Footnotes

  1. https://www.postgresql.org/docs/current/datatype-character.html#DATATYPE-CHARACTER-SPECIAL-TABLE ↩

@mgartner mgartner moved this from Triage to New Backlog in SQL Queries Aug 3, 2023
@michae2 michae2 moved this from New Backlog to Triage in SQL Queries Aug 11, 2023
@michae2
Copy link
Collaborator

michae2 commented Aug 11, 2023

Moving back to triage to remove the NAME type from the randomized test that found the failure.

@msirek msirek self-assigned this Aug 15, 2023
msirek pushed a commit to msirek/cockroach that referenced this issue Aug 15, 2023
sqlsmith: disable NAME type in postgres mode
b475856 
The NAME type is known to have different behavior in CRDB than in
postgres, in that casting to name truncates a string to 63 characters in
postgres, but not in CRDB, causing sqlsmith test failures. For backwards
compatibility, CRDB behavior is not being changed to match postgres,
at least for now.

The fix it to prevent sqlsmith tests which use postgres mode (which
implies it is comparing CRDB results with postgres) from generating NAME
as a random type for testing.

Fixes: cockroachdb#107895

Release note: None
@msirek msirek mentioned this issue Aug 15, 2023
Merged
@yuzefovich yuzefovich moved this from Triage to Active in SQL Queries Aug 16, 2023
craig bot pushed a commit that referenced this issue Aug 16, 2023
Merge #108811
e0de1f1 
108811: sqlsmith: disable NAME type in postgres mode r=michae2 a=msirek

The NAME type is known to have different behavior in CRDB than in postgres, in that casting to name truncates a string to 63 characters in postgres, but not in CRDB, causing sqlsmith test failures. For backwards compatibility, CRDB behavior is not being changed to match postgres, at least for now.

The fix is to prevent sqlsmith tests which use postgres mode (which implies it is comparing CRDB results with postgres) from generating NAME as a random type for testing.

Fixes: #107895

Release note: None

Co-authored-by: Mark Sirek <[email protected]>
@craig craig bot closed this as completed in 2f22b2a Aug 16, 2023
@github-project-automation github-project-automation bot moved this from Active to Done in SQL Queries Aug 16, 2023
blathers-crl bot pushed a commit that referenced this issue Aug 16, 2023
sqlsmith: disable NAME type in postgres mode
1096e05 
The NAME type is known to have different behavior in CRDB than in
postgres, in that casting to name truncates a string to 63 characters in
postgres, but not in CRDB, causing sqlsmith test failures. For backwards
compatibility, CRDB behavior is not being changed to match postgres,
at least for now.

The fix is to prevent sqlsmith tests which use postgres mode (which
implies it is comparing CRDB results with postgres) from generating NAME
as a random type for testing.

Fixes: #107895

Release note: None
This was referenced Aug 16, 2023
Merged
Merged
blathers-crl bot pushed a commit that referenced this issue Aug 16, 2023
sqlsmith: disable NAME type in postgres mode
910a6f8 
The NAME type is known to have different behavior in CRDB than in
postgres, in that casting to name truncates a string to 63 characters in
postgres, but not in CRDB, causing sqlsmith test failures. For backwards
compatibility, CRDB behavior is not being changed to match postgres,
at least for now.

The fix is to prevent sqlsmith tests which use postgres mode (which
implies it is comparing CRDB results with postgres) from generating NAME
as a random type for testing.

Fixes: #107895

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@msirek msirek

Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. C-test-failure Broken test (automatically or manually discovered). O-rsg Random Syntax Generator T-sql-queries SQL Queries Team
Projects
SQL Queries
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sqlsmith: disable NAME type in postgres mode msirek/cockroach
4 participants
@mgartner @rafiss @msirek @michae2