cobed95 commented on code in PR #4945:
URL: https://github.com/apache/polaris/pull/4945#discussion_r3610725009
##########
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:
This became moot with the rework: following the dev-list direction,
`schemaName()` is removed entirely in
[b95591572](https://github.com/apache/polaris/pull/4945/commits/b95591572) —
`RelationalJdbcConfiguration` is unchanged from `main`, and the schema is now
provided through the standard datasource configuration (with a shipped default
in [06dd6f35c](https://github.com/apache/polaris/pull/4945/commits/06dd6f35c)).
##########
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:
Done in
[b95591572](https://github.com/apache/polaris/pull/4945/commits/b95591572): the
per-borrow statement is gone and `DatasourceOperations` no longer knows about
schemas at all — the driver's `currentSchema` connection property does the
work, applied once per physical connection as you noted. The only remaining
delta in this class is recognizing H2's `42S04` SQLSTATE (table not found on an
empty database), which unqualified references surface where schema-qualified
ones used to produce schema-not-found.
To keep upgrades and the out-of-the-box experience unchanged,
[06dd6f35c](https://github.com/apache/polaris/pull/4945/commits/06dd6f35c)
ships
`quarkus.datasource.jdbc.additional-jdbc-properties.currentSchema=POLARIS_SCHEMA`
as a default (runtime defaults + admin tool); a `currentSchema` in the JDBC
URL takes precedence over it — pgjdbc documents this, and I verified it
empirically against PostgreSQL and CockroachDB.
##########
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:
Agreed — removed in
[b95591572](https://github.com/apache/polaris/pull/4945/commits/b95591572).
Polaris no longer issues `CREATE SCHEMA`; the docs now describe the two-step
procedure (a DBA creates the schema, then the admin tool bootstraps), the
getting-started assets perform that init step, and `CHANGELOG.md` carries a
breaking-change note for fresh installations
([4876c4152](https://github.com/apache/polaris/pull/4945/commits/4876c4152)).
Existing deployments are unaffected since their schema already exists.
--
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]
