adutra commented on code in PR #4945:
URL: https://github.com/apache/polaris/pull/4945#discussion_r3588920881
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java:
##########
@@ -461,8 +492,45 @@ public boolean isRelationDoesNotExist(SQLException e) {
&& databaseType == DatabaseType.H2);
}
+ /**
+ * Borrows a connection from the datasource and selects the configured
schema as the session
+ * schema, so that the unqualified table names in generated SQL resolve
against it. Pooled
+ * connections may be reused across borrowers, so the schema is
(re-)selected on every borrow.
+ */
private Connection borrowConnection() throws SQLException {
- return datasource.getConnection();
+ Connection connection = datasource.getConnection();
+ try (Statement statement = connection.createStatement()) {
Review Comment:
TBH I am not sure this is the right place to do this.
IMHO the more idiomatic option is to let users define the schema through a
JDBC configuration property.
Both PostgreSQL and H2 support specifying the schema as a connection
property/URL param: `currentSchema` for Postgres/CockroachDB, and `SCHEMA` for
H2 (iirc).
Users can set this in two ways:
- via `quarkus.datasource.jdbc.url` (by appending the property to the URL)
- via `quarkus.datasource.jdbc.additional-jdbc-properties.*` (separate
property)
Besides, the driver would apply it when the _physical_ connection is
created, so it's set once per physical connection, not re-executed on every
logical borrow.
The more it goes the more I think that `DatasourceOperations` and related
classes should become completely agnostic of the schema name.
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java:
##########
@@ -461,8 +492,45 @@ public boolean isRelationDoesNotExist(SQLException e) {
&& databaseType == DatabaseType.H2);
}
+ /**
+ * Borrows a connection from the datasource and selects the configured
schema as the session
+ * schema, so that the unqualified table names in generated SQL resolve
against it. Pooled
+ * connections may be reused across borrowers, so the schema is
(re-)selected on every borrow.
+ */
private Connection borrowConnection() throws SQLException {
- return datasource.getConnection();
+ Connection connection = datasource.getConnection();
+ try (Statement statement = connection.createStatement()) {
+ // schemaName is validated as a plain SQL identifier in
resolveSchemaName; identifiers cannot
+ // be bind parameters.
+ statement.execute(selectSessionSchemaStatement());
+ return connection;
+ } catch (SQLException | RuntimeException e) {
+ try {
+ connection.close();
+ } catch (SQLException closeException) {
+ e.addSuppressed(closeException);
+ }
+ throw e;
+ }
+ }
+
+ private String selectSessionSchemaStatement() {
+ return switch (databaseType) {
+ case H2 -> "SET SCHEMA " + schemaName;
+ case POSTGRES, COCKROACHDB -> "SET search_path TO " + schemaName;
+ };
+ }
+
+ /**
+ * Creates the configured schema if it does not exist yet. Uses a raw
connection instead of {@link
+ * #borrowConnection()}: on some databases (H2) selecting a non-existent
session schema fails, so
+ * the schema must exist before connections can be borrowed.
+ */
+ private void ensureSchemaExists() throws SQLException {
+ try (Connection connection = datasource.getConnection();
+ Statement statement = connection.createStatement()) {
+ statement.execute("CREATE SCHEMA IF NOT EXISTS " + schemaName);
Review Comment:
Do we absolutely need this feature? I am not convinced that it's the
application responsibility to create the schema it should be using. This
statement requires a lot more privileges than the ones the application should
be normally granted.
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/RelationalJdbcConfiguration.java:
##########
@@ -35,4 +40,22 @@ public interface RelationalJdbcConfiguration {
* the JDBC connection metadata. Supported values: "postgresql",
"cockroachdb", "h2"
*/
Optional<String> databaseType();
+
+ /**
+ * The database schema (namespace) that holds the Polaris tables. If not
specified, it defaults to
+ * {@code POLARIS_SCHEMA}. The schema is created during bootstrap if it does
not already exist,
Review Comment:
In this case why don't we make this method return `DEFAULT_SCHEMA_NAME`?
For example, you could have here:
```java
default String schemaName() {
return DEFAULT_SCHEMA_NAME;
}
```
And in `QuarkusRelationalJdbcConfiguration`:
```java
@Pattern(
regexp = "[A-Za-z_][A-Za-z0-9_]*",
message =
"must start with a letter or underscore and contain only letters,
digits, and underscores")
@WithDefault(RelationalJdbcConfiguration.DEFAULT_SCHEMA_NAME)
@Override
String schemaName();
```
Note: I'm moving the `@Pattern` annotation to
`QuarkusRelationalJdbcConfiguration` since you mentioned that it won't be
effective outside of a Quarkus-enabled context.
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java:
##########
@@ -422,8 +438,7 @@ public static PreparedQuery generateOverlapQuery(
return new PreparedQuery(query.sql(), where.parameters());
}
- static String getFullyQualifiedTableName(String tableName) {
- // TODO: make schema name configurable.
- return "POLARIS_SCHEMA." + tableName;
+ String getFullyQualifiedTableName(String tableName) {
+ return schemaName + "." + tableName;
Review Comment:
I think that's reasonable for now, thanks!
--
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]
