jerryshao commented on code in PR #14521:
URL: https://github.com/apache/gravitino/pull/14521#discussion_r4464314127
##########
catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java:
##########
@@ -792,16 +802,123 @@ protected ResultSet getTable(Connection connection,
String schema, String tableN
protected ResultSet getColumns(Connection connection, String schema, String
tableName)
throws SQLException {
DatabaseMetaData metaData = connection.getMetaData();
- return metaData.getColumns(
- connection.getCatalog(),
- metadataPattern(metaData, schema),
- metadataPattern(metaData, tableName),
- null);
+ CachedRowSet columns = RowSetProvider.newFactory().createCachedRowSet();
+ try (ResultSet metadata =
+ metaData.getColumns(
+ connection.getCatalog(),
+ metadataPattern(metaData, schema),
+ metadataPattern(metaData, tableName),
+ null)) {
+ columns.populate(metadata);
+ }
+
+ boolean needsDeclarations = false;
+ while (columns.next()) {
+ String type = columns.getString("TYPE_NAME");
+ if ("varchar".equals(type)
+ || "bpchar".equals(type)
+ || "_varchar".equals(type)
+ || "_bpchar".equals(type)
+ || (getAutoIncrementInfo(columns) && columns.getString("COLUMN_DEF")
!= null)) {
+ needsDeclarations = true;
+ break;
+ }
+ }
+ columns.beforeFirst();
+ if (!needsDeclarations) {
+ return columns;
+ }
+
+ // COLUMN_SIZE substitutes the configurable pgjdbc unknownLength for
unbounded character
+ // types. Serial defaults also need their declaration: JDBC marks shared
sequence defaults as
+ // auto-incrementing too. One catalog query resolves both without querying
per column.
+ Set<String> unboundedColumns = new HashSet<>();
+ Set<String> serialColumns = new HashSet<>();
+ // Match only a bare nextval call and verify its dependency on the owned
sequence. Ownership
+ // alone would also match custom defaults such as nextval(owned_sequence)
+ 1.
+ // Resolve ownership by OID: pg_get_serial_sequence would resolve the
table name and require
+ // schema USAGE even when JDBC and catalog metadata are readable without
that privilege.
+ String sql =
+ "SELECT a.attname, a.atttypmod = -1 AND a.atttypid IN "
+ + "('pg_catalog.varchar'::pg_catalog.regtype,
'pg_catalog.bpchar'::pg_catalog.regtype, "
+ + "'pg_catalog.varchar[]'::pg_catalog.regtype, "
+ + "'pg_catalog.bpchar[]'::pg_catalog.regtype) AS
unbounded_character, "
+ + "a.attnotnull AND a.atttypid IN
('pg_catalog.int2'::pg_catalog.regtype, "
Review Comment:
Question: this declaration query doesn't read `attidentity`, and every
auto-increment column is recreated as `GENERATED BY DEFAULT AS IDENTITY` (line
342, also 473 and 681). So a `GENERATED ALWAYS` identity column is downgraded
to `BY DEFAULT` on copy, and an explicit-id insert that PostgreSQL would reject
on the source succeeds on the copy. `docs/jdbc-postgresql-catalog.md:159`
already documents this. If that contract stays, could
`TestPostgreSqlTableOperations` (around line 464) assert that an explicit id
insert on the copy succeeds, so the downgrade is locked in? It currently only
inserts `DEFAULT VALUES`. Preserving `ALWAYS` would need more than the boolean
`autoIncrement` flag, so a follow-up issue seems fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
