Verify column name length in PostgreSQL and SQL Server #13742

ebyhr · 2022-08-19T00:55:28Z

Description

Documentation

(x) No documentation is needed.

Release notes

(x) Release notes entries required with the following suggested text:

# PostgreSQL
* Prevent using column names with a name which is longer
  than the max length supported by PostgreSQL. Previously, long names were
  truncated to the max length. ({issue}`13742`)
# SQL Server
* Prevent renaming a column to a name which is longer
  than the max length supported by SQL Server. Previously, long names were
  truncated to the max length. ({issue}`13742`)

findepi · 2022-08-23T20:04:30Z

@ebyhr can you please rebase? there are conflicts.

ebyhr · 2022-08-23T21:21:23Z

Rebased on upstream to resolved conflicts.

findepi · 2022-08-24T08:33:06Z

plugin/trino-mariadb/src/main/java/io/trino/plugin/mariadb/MariaDbClient.java

@@ -150,6 +150,8 @@
    // An empty character means that the table doesn't have a comment in MariaDB
    private static final String NO_COMMENT = "";

+    private static final int PARSE_ERROR = 1064;


is it documented? can you link to docs?

findepi · 2022-08-24T08:34:07Z

plugin/trino-mariadb/src/main/java/io/trino/plugin/mariadb/MariaDbClient.java

@@ -457,7 +459,7 @@ public void renameColumn(ConnectorSession session, JdbcTableHandle handle, JdbcC
            execute(connection, sql);
        }
        catch (TrinoException e) {
-            if (e.getCause() instanceof SQLSyntaxErrorException) {
+            if (e.getCause() instanceof SQLSyntaxErrorException syntaxError && syntaxError.getErrorCode() == PARSE_ERROR) {


SQLSyntaxErrorException contains invalid column name.

add a code comment

// Note: SQLSyntaxErrorException can be throw also when column name is invalid

BTW do we need this complicated logic?
why not "drop support" for the older MariaDB version and simplify this?

I think throwing an MariaDB exception without conversion is also fine. Let me handle in a follow-up PR.

findepi · 2022-08-24T08:35:42Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

+                jdbcColumn.getColumnName(),
+                newRemoteColumnName);


Why aren't these quoted?

(i know they weren't quoted before the refactor)

Thanks for catching that. Let me fix in a follow-up PR.

findepi · 2022-08-24T08:37:38Z

plugin/trino-postgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlClient.java

+        // PostgreSQL truncates table name to 63 chars silently
+        // PostgreSQL driver caches the max column name length in a DatabaseMetaData object. The cost to call this method per column is low.
+        if (columnName.length() > databaseMetadata.getMaxColumnNameLength()) {
+            throw new TrinoException(NOT_SUPPORTED, format("Column name must be shorter than or equal to '%s' characters but got '%s'", databaseMetadata.getMaxColumnNameLength(), columnName.length()));


Quote the contents of the columnName in the message besides their length.

It's important for CREATE TABLE statement, where there can be multiple potentially problematic columns in play

findepi · 2022-08-24T08:37:53Z

plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java

+        // SQL Server truncates table name to the max length silently when renaming a column
+        // SQL Server driver doesn't communicate with a server in getMaxColumnNameLength. The cost to call this method per column is low.
+        if (columnName.length() > databaseMetadata.getMaxColumnNameLength()) {
+            throw new TrinoException(NOT_SUPPORTED, format("Column name must be shorter than or equal to '%s' characters but got '%s'", databaseMetadata.getMaxColumnNameLength(), columnName.length()));


Quote the contents of the columnName in the message besides their length

SQLSyntaxErrorException contains invalid column name.

testRenameColumn was skipped by SUPPORTS_CREATE_TABLE condition.

ebyhr · 2022-08-24T13:20:22Z

CI hit #13825 & #13199

hashhar · 2023-09-10T10:44:00Z

Today while in looking into something related I saw a very disturbing behaviour from postgres. Directly in Postgres:

tpch=# select * from (select name as zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz, regionkey as zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz from tpch.region) t;
NOTICE:  identifier "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz" will be truncated to "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz"
NOTICE:  identifier "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz" will be truncated to "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz"
 zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz | zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz
-----------------------------------------------------------------+-----------------------------------------------------------------
 AFRICA                                                          |                                                               0
 AMERICA                                                         |                                                               1
 ASIA                                                            |                                                               2
 EUROPE                                                          |                                                               3
 MIDDLE EAST                                                     |                                                               4
(5 rows)

tpch=# select zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz from (select name as zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz, regionkey as zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz from tpch.region) t;
NOTICE:  identifier "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz" will be truncated to "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz"
NOTICE:  identifier "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz" will be truncated to "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz"
ERROR:  column reference "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz" is ambiguous
LINE 1: select zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz...
               ^
tpch=# select zzzzzzzzzzzzzzzzzzzzzzzzzzzzz from (select name as zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz, regionkey as zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz from tpch.region) t;
NOTICE:  identifier "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz" will be truncated to "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz"
NOTICE:  identifier "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz" will be truncated to "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz"
ERROR:  column "zzzzzzzzzzzzzzzzzzzzzzzzzzzzz" does not exist
LINE 1: select zzzzzzzzzzzzzzzzzzzzzzzzzzzzz from (select name as zz...

This means that Postgres truncates ALL occurences of long identifiers and the truncated values are treated equivalently.
i.e. If postgres only allowed 2 char identifiers then zzz == zzzz == zzzzzzz and any other combination.

However when selecting from a relation which produces zzzz and zzz both get truncated to zz and the SELECT complains about ambiguity.

So it seems we're still "safe" in the sense that if for example join pushdown caused truncated identifiers then the worst thing that can happen would be query failure because postgres would complain of ambiguity.

cla-bot bot added the cla-signed label Aug 19, 2022

ebyhr force-pushed the ebi/long-column-name branch 4 times, most recently from 4bb1cb8 to 4c10f34 Compare August 19, 2022 09:34

ebyhr marked this pull request as ready for review August 19, 2022 23:14

ebyhr force-pushed the ebi/long-column-name branch from 4c10f34 to 88dae14 Compare August 19, 2022 23:21

ebyhr requested a review from hashhar August 20, 2022 03:10

ebyhr force-pushed the ebi/long-column-name branch from 88dae14 to 831a156 Compare August 23, 2022 21:05

findepi approved these changes Aug 24, 2022

View reviewed changes

ebyhr added 4 commits August 24, 2022 19:03

Check error code for RENAME COLUMN failure in MariaDB

7f4a08a

SQLSyntaxErrorException contains invalid column name.

Set SUPPORTS_RENAME_COLUMN as false in unsupported connectors

6381cff

testRenameColumn was skipped by SUPPORTS_CREATE_TABLE condition.

Add renameColumnSql method to JDBC connectors

23b483c

Verify column name length in PostgreSQL and SQL Server

b01e763

ebyhr force-pushed the ebi/long-column-name branch from 831a156 to b01e763 Compare August 24, 2022 10:05

ebyhr merged commit fe16cae into master Aug 24, 2022

ebyhr deleted the ebi/long-column-name branch August 24, 2022 22:04

ebyhr mentioned this pull request Aug 24, 2022

Release notes for 394 #13723

Closed

github-actions bot added this to the 394 milestone Aug 24, 2022

colebow mentioned this pull request Aug 29, 2022

Add Trino 394 release notes #13813

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify column name length in PostgreSQL and SQL Server #13742

Verify column name length in PostgreSQL and SQL Server #13742

ebyhr commented Aug 19, 2022 •

edited

Loading

findepi commented Aug 23, 2022

ebyhr commented Aug 23, 2022

findepi Aug 24, 2022

findepi Aug 24, 2022

ebyhr Aug 24, 2022

findepi Aug 24, 2022

ebyhr Aug 24, 2022

findepi Aug 24, 2022

findepi Aug 24, 2022

ebyhr commented Aug 24, 2022 •

edited

Loading

hashhar commented Sep 10, 2023

Verify column name length in PostgreSQL and SQL Server #13742

Verify column name length in PostgreSQL and SQL Server #13742

Conversation

ebyhr commented Aug 19, 2022 • edited Loading

Description

Documentation

Release notes

findepi commented Aug 23, 2022

ebyhr commented Aug 23, 2022

findepi Aug 24, 2022

Choose a reason for hiding this comment

findepi Aug 24, 2022

Choose a reason for hiding this comment

ebyhr Aug 24, 2022

Choose a reason for hiding this comment

findepi Aug 24, 2022

Choose a reason for hiding this comment

ebyhr Aug 24, 2022

Choose a reason for hiding this comment

findepi Aug 24, 2022

Choose a reason for hiding this comment

findepi Aug 24, 2022

Choose a reason for hiding this comment

ebyhr commented Aug 24, 2022 • edited Loading

hashhar commented Sep 10, 2023

ebyhr commented Aug 19, 2022 •

edited

Loading

ebyhr commented Aug 24, 2022 •

edited

Loading