Make the Relational JDBC schema name configurable - #4945
Conversation
The Relational JDBC persistence backend previously hard-coded its schema
as POLARIS_SCHEMA in QueryGenerator and in the bootstrap SQL scripts, so
operators could not run multiple Polaris deployments in one database or
comply with a schema-naming policy.
Add polaris.persistence.relational.jdbc.schema-name to configure it,
defaulting to POLARIS_SCHEMA so existing deployments are unaffected:
- RelationalJdbcConfiguration exposes schemaName(); the Quarkus config
mapping picks it up for both the server and the admin tool.
- QueryGenerator becomes an instance bound to the schema; DatasourceOperations
resolves and validates it as a plain SQL identifier (it is interpolated
into SQL, not bound) and owns the QueryGenerator.
- Bootstrap scripts use a ${schema} placeholder substituted when the init
script runs, so the configured schema is created and used consistently.
- Exposed as persistence.relationalJdbc.schemaName in the Helm chart and
documented in the admin tool properties, metastore docs, and CHANGELOG.
| # default schema `POLARIS_SCHEMA`. Must be a valid SQL identifier: it must start with a letter or | ||
| # underscore and contain only letters, digits, and underscores. | ||
| # @section -- Persistence | ||
| schemaName: "" |
There was a problem hiding this comment.
Why not use the default?
| schemaName: "" | |
| schemaName: "POLARIS_SCHEMA" |
There was a problem hiding this comment.
I agree. Although the empty string would still resolve to the default POLARIS_SCHEMA at the code level, it would be much more straightforward for users to see the explicit default value in the helm chart values. I will fix this one first.
|
|
||
| CREATE SCHEMA IF NOT EXISTS POLARIS_SCHEMA; | ||
| SET search_path TO POLARIS_SCHEMA; | ||
| CREATE SCHEMA IF NOT EXISTS ${schema}; |
There was a problem hiding this comment.
Using placeholders looks like a code smell. If the schema name is dynamic, I'd suggest that the scripts should not include a CREATE SCHEMA statement at all.
Here is a better workflow imho:
- Java creates the schema programmatically before running the script - a single
CREATE SCHEMA IF NOT EXISTS <name>statement executed via JDBC (the schema name is already validated as a safe identifier). - Java sets the session search path on the connection before executing the script
SET search_path TO <name>for Postgres/CockroachDB,SET SCHEMA <name> for H2. This would required customization per-database type, but seems doable. - The SQL scripts use only unqualified table names, no schema references at all. They become truly database-agnostic with respect to schema placement.
There was a problem hiding this comment.
I totally agree that schema creation and setting the session search path should be done programmatically.
Here's what I think might be a valid concrete implementation.
- Schema creation is done programmatically at the beginning of
executeScriptinstance method ofDataSourceOperations - Session search path is set on
borrowConnectionmethod ofDataSourceOperations. If we are to avoid quoting schema names as per this comment, we avoid using JDBC'sConnection.setSchema()since pgjdbc always quotes the schema identifier and this may result in a regression for existing deployments. - Remove references to the schema name in the SQL scripts.
I'd love to hear your thoughts.
There was a problem hiding this comment.
To make the above concrete, I've implemented it in 1ec77a3fc (schema created programmatically in executeScript, session schema selected in borrowConnection) and 3cb5eb7ef (scripts reduced to pure schema-agnostic DDL). Happy to adjust if you'd envisioned any of the steps differently.
One nuance that came up in implementation: the CREATE SCHEMA IF NOT EXISTS has to run on a raw (non-schema-selected) connection, because on H2 selecting a not-yet-existing session schema fails — the schema must exist before regular connections can be borrowed. DatasourceOperationsSchemaTest covers that pre-bootstrap path against real H2.
| // TODO: make schema name configurable. | ||
| return "POLARIS_SCHEMA." + tableName; | ||
| String getFullyQualifiedTableName(String tableName) { | ||
| return schemaName + "." + tableName; |
There was a problem hiding this comment.
I think that quoting and casing will become a potential issue here. If a user inputs the schema name as MySchema, without quoting, Postgres will treat it as myschema – this may not be what users intended. I wonder if we should quote the schema name here so that it becomes case-sensitive, and change the default schema name to polaris_schema to match the current behavior.
But then we need to be careful when introducing support for MySQL in #3960: MySQL has a whole different way of handling case sensitivity and quoting.
There was a problem hiding this comment.
It seems that quoting + casing behaviour is different for each DBMS. For example PostgreSQL and CockroachDB assumes lowercase, H2 assumes uppercase, and MySQL as you've mentioned behaves in a totally different way (it depends on the underlying OS + user configurations). In fact, for MySQL, the database acts as a schema, so it may even be possible to drop the notion of a schema and just use the database specified in the JDBC URL when supporting MySQL. (Please correct me if I'm wrong.)
That said, since the schema case handling is already different between Postgres/CockroachDB and H2, I would like to gently propose to keep the unquoted behaviour to avoid breaking existing deployments. In this case the users need to be wary of the default case handling behaviour of unquoted schema names of their chosen DBMS. Clear documentation should be added as well.
There was a problem hiding this comment.
To make the proposal above concrete, the unquoted behavior is now documented in 40ba1f142 (metastore docs, admin tool properties, and the config interface javadoc): the name is used unquoted, so the database applies its usual identifier case folding — matching how the hard-coded scripts have always behaved on both databases. Of course, still very open to reworking this if you'd prefer quoted identifiers.
There was a problem hiding this comment.
I think that's reasonable for now, thanks!
| /** The database schema (namespace) that qualifies every generated table reference. */ | ||
| private final String schemaName; |
There was a problem hiding this comment.
I am wondering if this is the right approach.
Expanding on my idea stated previously that SQL scripts should be agnostic of the schema, I think the same is valid for JDBC connections in general. If DatasourceOperations sets the session search path on every connection it acquires, then:
QueryGeneratorgoes back to being a pure static utility- All table references in generated SQL are unqualified (e.g.
ENTITIES, notPOLARIS_SCHEMA.ENTITIES) - We avoid problems with quoting and casing of schema names in this class (the problem is transferred to
DatabaseOperationswhere we can apply different logics per database type) - The bulk of this PR's diff simply disappears
- The
${schema}substitution in SQL scripts disappears too.
WDYT?
There was a problem hiding this comment.
Yes, I agree here as well!
There was a problem hiding this comment.
Implemented in 1ec77a3fc: QueryGenerator is a pure static utility again, all generated SQL uses unqualified table names, and DatasourceOperations selects the session schema on every connection it borrows (SET search_path TO <name> for PostgreSQL/CockroachDB, SET SCHEMA <name> for H2). As you predicted, the bulk of the previous diff disappeared.
One deliberate choice worth flagging: the SET runs on every borrow rather than once per physical connection. It's a single cheap statement, and it keeps correctness independent of pool behavior; if it ever shows up in profiles it could move to the pool's new-connection SQL, but I didn't want correctness to depend on deployment-level Agroal configuration. Happy to iterate on this.
| try { | ||
| PreparedQuery pq = | ||
| QueryGenerator.generateInsertQuery( | ||
| queryGenerator.generateInsertQuery( |
There was a problem hiding this comment.
Here we have an inconsistency: queryGenerator is being applied to a (potentially) distinct DatasourceOperations instance (metricsOps).
If we ever allow different datasources for metrics, then we may also wonder if these datasources would all have the same schema name, or different ones.
Just thinking out loud.
There was a problem hiding this comment.
Good point, thanks for thinking ahead on this. If we go with the design you described in this comment, together with your suggestion to return QueryGenerator to a static utility, I believe this inconsistency resolves itself nicely: generated queries would no longer be bound to any particular schema. If a separate metrics report database is ever supported, getMetricsDatasource() could then simply return an isolated DatasourceOperations instance carrying its own configuration — including its own schema name.
There was a problem hiding this comment.
Following up now that the code is pushed: with 1ec77a3fc this is the case — the queryGenerator field is gone from JdbcBasePersistenceImpl, so nothing ties generated queries to a particular DatasourceOperations instance anymore.
| } | ||
| } | ||
|
|
||
| private static String resolveSchemaName(RelationalJdbcConfiguration configuration) { |
There was a problem hiding this comment.
I think you can replace this method with an annotation on the config class:
@Pattern(
regexp = "[A-Za-z_][A-Za-z0-9_]*",
message = "must start with a letter or underscore and contain only letters, digits, and underscores")
Optional<String> schemaName();Pattern would be jakarta.validation.constraints.Pattern.
There was a problem hiding this comment.
Thanks, I like this — it gives a clean startup error with the offending property name for free. Two things I ran into while trying it, and one question:
-
The constraint needs to be a container-element annotation, i.e.
Optional<@Pattern(...) String> schemaName()— Hibernate Validator doesn't implicitly unwrap a genericOptional<String>for a method-level constraint, so the method-level form fails at validation time withjakarta.validation.UnexpectedTypeException: HV000030: No validator could be found for constraint 'jakarta.validation.constraints.Pattern' validating type 'java.util.Optional<java.lang.String>'— even when the configured value is valid. I verified both forms against the SmallRye config-mapping validation path (BeanValidationConfigValidatorImpl): the container-element form correctly rejects an invalid value withConfigValidationException: <property> must match "[A-Za-z_][A-Za-z0-9_]*"and accepts valid ones. -
I'd like to keep a small programmatic check in
DatasourceOperationsin addition to the annotation, for two reasons:- The annotation is only enforced where a
jakarta.validationimplementation is on the classpath. That's true for the server (runtime/servicebringsquarkus-hibernate-validator), but not for the admin tool — and the admin tool is exactly the place that runs bootstrap, where the value ends up interpolated intoCREATE SCHEMA IF NOT EXISTS <name>. RelationalJdbcConfigurationis a plain interface that's also implemented outside Quarkus (tests, embedding), where bean validation never runs.
Since the name can't be a bind parameter, I'd rather the guard live at the point of interpolation than depend on the deployment's classpath.
- The annotation is only enforced where a
Would you be okay with annotation + programmatic check? Alternatively, if you'd prefer annotation-only, we could add the validator to the admin tool's dependencies — happy to go that way instead if you think the duplication isn't worth it.
There was a problem hiding this comment.
I've gone ahead and implemented the container-element form described above in 1ed89f430, keeping the programmatic check alongside it for now per the reasoning in my previous comment. If you'd rather go annotation-only (with the validator added to the admin tool), it's an easy switch — just let me know.
Per review feedback, make the chart default explicit instead of an empty string that falls back to the server-side default. The stale "when empty" wording is dropped from the value description accordingly (values.schema.json and reference.md regenerated to match).
Per review feedback on apache#4945: the schema is a connection-level concern, not a statement-level one. DatasourceOperations now selects the configured schema as the session schema on every connection it borrows (SET search_path TO <name> for PostgreSQL/CockroachDB, SET SCHEMA <name> for H2), and QueryGenerator returns to a pure static utility generating unqualified table names. executeScript now creates the schema programmatically before running a script: on H2, selecting a not-yet-existing session schema fails, so the schema must exist before connections can be borrowed. The bootstrap scripts themselves are unchanged in this commit and become schema- agnostic in the next one. This also removes the queryGenerator coupling between JdbcBasePersistenceImpl and a specific DatasourceOperations instance: a future dedicated metrics datasource would bring its own schema via its own configuration.
Remove the CREATE SCHEMA / SET search_path (SET SCHEMA) header and the
${schema} placeholder substitution: the schema is now created
programmatically and selected per connection by DatasourceOperations, so
the scripts contain pure DDL with no schema references at all.
Add @pattern to RelationalJdbcConfiguration.schemaName(), declared as a container-element constraint (Optional<@pattern(...) String>): Hibernate Validator does not implicitly unwrap a generic Optional<String> for a method-level constraint and fails with UnexpectedTypeException (HV000030) even for valid values. The programmatic check in DatasourceOperations stays: the annotation is only enforced where a jakarta.validation implementation is present (the server via quarkus-hibernate-validator, but not the admin tool), and the value is interpolated into CREATE SCHEMA / SET statements, so the guard belongs at the point of interpolation.
The schema name is used unquoted, so the database applies its usual identifier case folding (PostgreSQL folds to lowercase, H2 to uppercase), matching how the hard-coded scripts have always behaved on both databases. Also document that the database user needs USAGE on the schema at runtime and CREATE on the database for bootstrap.
Per review feedback on apache#4945: the schema is a connection-level concern, not a statement-level one. DatasourceOperations now selects the configured schema as the session schema on every connection it borrows (SET search_path TO <name> for PostgreSQL/CockroachDB, SET SCHEMA <name> for H2), and QueryGenerator returns to a pure static utility generating unqualified table names. executeScript now creates the schema programmatically before running a script: on H2, selecting a not-yet-existing session schema fails, so the schema must exist before connections can be borrowed. The bootstrap scripts themselves are unchanged in this commit and become schema- agnostic in the next one. This also removes the queryGenerator coupling between JdbcBasePersistenceImpl and a specific DatasourceOperations instance: a future dedicated metrics datasource would bring its own schema via its own configuration. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Hi Alexandre — since the proposals above have been sitting for a few days, I went ahead and implemented them so there's something concrete to evaluate. I hope that's alright; nothing here is meant to preempt the open discussions, and I'm very happy to revise or revert any part of it. The changes are split into one commit per review thread:
Each commit passes the module tests individually, and the Postgres Testcontainers ITs pass against the final state. I've updated the PR description to reflect the new design, and I'll keep the PR in draft until the open threads are settled. |
| CREATE SCHEMA IF NOT EXISTS POLARIS_SCHEMA; | ||
| SET search_path TO POLARIS_SCHEMA; |
There was a problem hiding this comment.
I don't think we need to change any old schema. We have discussed in a dev mailing thread about only keeping the latest version while releasing a new Polaris version, cc @dimas-b . There is an implicit minor behavior change we will need to apply, which is to remove this parameter for the bootstrap command , -v, --schema-version=<schema version>, https://polaris.apache.org/releases/1.6.0/admin-tool/#bootstrapping-realms-and-principal-credentials. This parameter allows users to bootstrap a realm with a historical schema. It might be useful for rare use cases, but not worth to do so.
There was a problem hiding this comment.
Hi @flyrain, thanks for the review!
I've replied to the dev mailing list that since the removal of --schema-version option is a discussion topic on its own, I'm assuming that the removal should be out of scope for this PR.
In that case, should we keep the changes to the historical DDL scripts just to let polaris run without errors, and perhaps remove the scripts once --schema-version option is removed?
Eager to hear your thoughts.
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks for the quick action. Now it seems safe to me as well to leave the historical DDL scripts unchanged.
There was a problem hiding this comment.
Thanks for the update. Is the PR ready for the review? It is still in the draft status now.
There was a problem hiding this comment.
As per the contribution guideline, I kept the PR as draft state until there is some consensus in the dev mailing list.
Happy to make it ready for review!
There was a problem hiding this comment.
I think the community has aligned on the direction, which should be good enough to move forward. The impl. details are usually discussed in the PR. cc @dimas-b
There was a problem hiding this comment.
I agree. Let's not alter legacy (non-current) schema versions.
# Conflicts: # CHANGELOG.md
Per review feedback: historical schema versions should not be modified, since only the latest schema version is carried forward. Now that the bootstrap command's --schema-version option has been removed upstream (apache#5044), new realms are always bootstrapped with the latest schema version, so the v0-v3 scripts keep their original CREATE SCHEMA / SET search_path header targeting POLARIS_SCHEMA and only the latest (v4) scripts are schema-agnostic.
|
@cobed95 : I see Claude being mentioned as a formal commit co-author. In Polaris the current guidelines are for humans to assume authorship. It's ok to use AI as an aid, though. I believe it is preferable to remove Claude from commit author tags. You can mention it in the description. |
|
|
||
| /** | ||
| * 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, |
There was a problem hiding this comment.
In this case why don't we make this method return DEFAULT_SCHEMA_NAME?
For example, you could have here:
default String schemaName() {
return DEFAULT_SCHEMA_NAME;
}And in QuarkusRelationalJdbcConfiguration:
@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.
There was a problem hiding this comment.
| // TODO: make schema name configurable. | ||
| return "POLARIS_SCHEMA." + tableName; | ||
| String getFullyQualifiedTableName(String tableName) { | ||
| return schemaName + "." + tableName; |
There was a problem hiding this comment.
I think that's reasonable for now, thanks!
| private Connection borrowConnection() throws SQLException { | ||
| return datasource.getConnection(); | ||
| Connection connection = datasource.getConnection(); | ||
| try (Statement statement = connection.createStatement()) { |
There was a problem hiding this 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.
There was a problem hiding this comment.
Done in 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 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.
| private void ensureSchemaExists() throws SQLException { | ||
| try (Connection connection = datasource.getConnection(); | ||
| Statement statement = connection.createStatement()) { | ||
| statement.execute("CREATE SCHEMA IF NOT EXISTS " + schemaName); |
There was a problem hiding this 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.
There was a problem hiding this comment.
Agreed — removed in 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). Existing deployments are unaffected since their schema already exists.
889d759 to
d75c444
Compare
@dimas-b Thank you for your guidance. I've amended and force pushed so all authorships point to me. New commits will not reference Claude as a formal commit co-author. |
# Conflicts: # persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java # persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/QueryGeneratorTest.java # persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/idempotency/RelationalJdbcIdempotencyStorePostgresIT.java
Upstream introduced schema v5 as the new latest version (apache#5086), which makes v4 a legacy version. Per the review agreement to leave legacy schema versions untouched, the v4 scripts are restored to their original form and the new v5 scripts drop the CREATE SCHEMA / SET search_path header instead, so the configurable schema name applies to the current schema version.
Per the dev-list discussion: the schema is now provided entirely through the datasource configuration (e.g. the PostgreSQL driver's currentSchema connection property), so the Polaris-level schema handling is removed: - RelationalJdbcConfiguration.schemaName() and its bean/programmatic validation are gone; the interface is back to its previous shape and no longer changes an extension point. - DatasourceOperations no longer resolves, validates, or selects a schema, and no longer creates one at bootstrap: creating the schema is a DBA step that requires elevated privileges Polaris should not need. - Generated SQL stays unqualified and the current (v5) bootstrap scripts stay schema-agnostic. - isRelationDoesNotExist learns H2's 42S04 (table not found, database empty): with unqualified references against a fresh database this state surfaces where a schema-qualified reference used to produce schema-not-found. - Tests provide the schema the way a real deployment would: the H2 datasources create and select POLARIS_SCHEMA via the INIT= URL setting, and the Postgres IT pre-creates the schema (the DBA step) and selects it with the driver's currentSchema property.
Keep upgrades and the out-of-the-box experience unchanged: without any
schema selection, unqualified SQL would resolve against the database's
default search path ("public" on PostgreSQL) instead of POLARIS_SCHEMA,
where every existing deployment holds its tables. The default covers
PostgreSQL and CockroachDB (both use the pgjdbc currentSchema connection
property); it reaches the driver through the connection-properties
channel, which pgjdbc documents as yielding to a currentSchema set in
the JDBC URL, so user URLs override it. The admin tool carries the same
default so server and bootstrap always agree on the schema.
H2 (test-only) ignores the unknown property; H2-based tests select
their schema via URL settings.
The persistence.relationalJdbc.schemaName value is kept as the operator- facing knob, but it now renders the standard Quarkus datasource property (quarkus.datasource.jdbc.additional-jdbc-properties.currentSchema) instead of a Polaris-specific option, matching the schema-agnostic persistence layer.
- The metastore docs describe schema selection via the currentSchema datasource property and the two-step fresh-deployment procedure (DBA creates the schema, then the admin tool bootstraps). - The getting-started assets create the schema on database init, the same DBA step a real deployment performs. - CHANGELOG gains a breaking-change note: Polaris no longer creates the schema during bootstrap; existing deployments are unaffected. - The generated config reference no longer lists the removed polaris.persistence.relational.jdbc.schema-name option.
|
@adutra Thanks for taking your time once again! I've replied on the dev mailing list with some additional points we might want to settle. In short, they are:
I've gone ahead and pushed implementation details first, but of course would be happy to make additional changes |
…erties
Replace the single-purpose persistence.relationalJdbc.schemaName value
with a persistence.relationalJdbc.additionalProperties map (default
{currentSchema: POLARIS_SCHEMA}) that renders each entry as a
quarkus.datasource.jdbc.additional-jdbc-properties.<name> property. This
keeps the schema selection as the default while letting operators
customize other JDBC driver connection properties through the same knob.
Polaris no longer creates the database schema during bootstrap, so: - Correct the admin-tool and production-configuration guides, which still claimed Polaris always creates polaris_schema during bootstrap; they now describe the schema as a prerequisite created by a DBA and selected via the driver's currentSchema property. - Create polaris_schema in the Helm CI PostgreSQL fixture (via a /docker-entrypoint-initdb.d ConfigMap), the same way the getting- started compose assets do, so bootstrap works against it with the shipped currentSchema default.
| try (Connection connection = dataSource.getConnection(); | ||
| PreparedStatement statement = | ||
| connection.prepareStatement( | ||
| "SELECT table_schema FROM information_schema.tables WHERE table_name = 'ENTITIES'")) { |
There was a problem hiding this comment.
Maybe add LIMIT 1 and return just the first row?
There was a problem hiding this comment.
Done in 142c64cc4 — bounded the lookup with LIMIT 1 and return the single row. Thanks!
| # Create the Polaris schema on first init: Polaris does not create it itself | ||
| - type: bind | ||
| source: ${ASSETS_PATH}/postgres/create-polaris-schema.sql | ||
| target: /docker-entrypoint-initdb.d/create-polaris-schema.sql |
| volumeMounts: | ||
| # Create the Polaris schema on first init: Polaris does not create it itself. | ||
| - name: init-schema | ||
| mountPath: /docker-entrypoint-initdb.d |
There was a problem hiding this comment.
I love the docker-entrypoint-initdb.d approach 😄
| # connection property, also used for CockroachDB). Override this property, or set currentSchema | ||
| # directly in the JDBC URL (the URL takes precedence). The schema must exist before Polaris | ||
| # connects; creating it is a DBA task requiring elevated privileges. | ||
| quarkus.datasource.jdbc.additional-jdbc-properties.currentSchema=POLARIS_SCHEMA |
There was a problem hiding this comment.
Does this(currentSchema) only work with PostgreSQL driver? How about other drivers, like MySQL? We will need a different way to inject the schema name, right?
quarkus.datasource.jdbc.url=jdbc:mysql://mysql.example.com:3306/polaris_schema
I understand that Polaris only supports PostgreSQL style driver now. However, there is a ongoing PR(#4281) to add MySQL support. This approach adds a hard dependency on the driver's behavior. Are we OK with it?
There was a problem hiding this comment.
Hi @flyrain, I've answered to your concern on the dev mailing list. Leaving a short answer here as well just for the record.
I've verified empirically that currentSchema acts as a silent no-op when connecting with MySQL.
However, I agree wholeheartedly that more community input is desirable!
I'll wait for some more responses!
Note in the additionalProperties docs that currentSchema is PostgreSQL/CockroachDB-specific and is silently ignored by other drivers such as MySQL, where the schema is instead the database named in the JDBC URL.
Per review: the information_schema lookup returns the single schema holding the ENTITIES table, so bound it with LIMIT 1 and return that one row instead of collecting a list.
# Conflicts: # extensions/metrics-reports/persistence/relational-jdbc/src/test/java/org/apache/polaris/extension/metrics/jdbc/JdbcMetricsPersistenceTest.java
Now that the persistence layer is schema-agnostic and the schema is selected by the datasource, tests that provision a fresh database must create and select POLARIS_SCHEMA themselves (the DBA step a real deployment performs), otherwise tables land in the database default schema while the shipped currentSchema default (and tests that query POLARIS_SCHEMA directly) expect POLARIS_SCHEMA: - The Postgres and CockroachDB test lifecycle managers create POLARIS_SCHEMA after starting the container; the Postgres metrics named datasource is pointed at the same schema. - The H2-backed runtime-service profiles create POLARIS_SCHEMA via the JDBC URL INIT and select it with new-connection-sql on every connection (H2 ignores the currentSchema connection property). - JdbcEventsPersistenceTest selects POLARIS_SCHEMA via H2 INIT so its events land there for every schema version.
Run the schema generator so values.schema.json includes the currentSchema sub-property derived from the additionalProperties default, fixing the helm-schema-verify check.
|
Update: rebased on main + CI fixes Brought the branch up to date with
|
| static String getFullyQualifiedTableName(String tableName) { | ||
| // TODO: make schema name configurable. | ||
| return "POLARIS_SCHEMA." + tableName; | ||
| } |
There was a problem hiding this comment.
is it possible to have 2 part identifier still ? can we get the schema and add this here ?
- i know we are setting this in connection context the schema but this seems more easy to debug stuff
There was a problem hiding this comment.
or to put it in a different way are we logging what is the current_schema
There was a problem hiding this comment.
IMHO, keeping the 2 part identifier coded into SQL statements seems a little redundant. However, I'm open to suggestions!
There was a problem hiding this comment.
I also think it's best to not deal with schemas in the code.
However it should certainly be possible to log the current schema somewhere; it should also be possible to put the schema name in the MDC context if needed.
| # running Polaris server. Override this property, or set currentSchema directly in the JDBC URL | ||
| # (the URL takes precedence). The schema must exist before bootstrapping; creating it is a DBA | ||
| # task requiring elevated privileges. | ||
| quarkus.datasource.jdbc.additional-jdbc-properties.currentSchema=POLARIS_SCHEMA |
There was a problem hiding this comment.
checking this : https://quarkus.io/guides/datasource#quarkus-agroal_quarkus-datasource-jdbc-additional-jdbc-properties-property-key
is this property supported by all drivers ? or is it just PG / Cockroach thing ?
There was a problem hiding this comment.
Hi @singhpk234 , this property works only for PG and CockroachDB and is a no-op for MySQL + etc. as mentioned above. My rationale was that MySQL would define the schema in the JDBC URL anyway (as PG / CockroachDB should for database), so no config change is needed if we start supporting MySQL.
| - Added the `DEFAULT_UNIQUE_TABLE_LOCATION_ENABLED` feature flag (off by default). When enabled, a managed location generated for a table or view created without an explicit location is given a unique, unpredictable suffix, so that no two tables share a path prefix. | ||
| - Added the `ALLOW_CLIENT_SPECIFIED_TABLE_LOCATION` feature flag (on by default). When set to false, a caller-specified location (the `location` field, a `SetLocation` update, or the `write.data.path` / `write.metadata.path` properties) on a create-table (including a staged create-table request), create-view, update-table, replace-view, or commit-transaction request is rejected, forcing Polaris to manage all locations. Federated catalogs, committing an already staged create, and `register table` / `register view` are unaffected. | ||
| - Added `maintenance` support in Helm chart. | ||
| - The database schema used by the Relational JDBC persistence backend is now configurable through standard datasource configuration: the JDBC driver's `currentSchema` connection property (defaulted to `POLARIS_SCHEMA` via `quarkus.datasource.jdbc.additional-jdbc-properties.currentSchema`) selects the schema, and the persistence layer is agnostic of the schema name. Also exposed as `persistence.relationalJdbc.schemaName` in the Helm chart. |
There was a problem hiding this comment.
This says the Helm exposes persistence.relationalJdbc.schemaName, but the chart exposes and renders only persistence.relationalJdbc.additionalProperties.currentSchema (values, template). An operator following the release note gets no rendered setting and will continue using the default schema. Would be good to change ti to persistence.relationalJdbc.additionalProperties.currentSchema.
There was a problem hiding this comment.
Thanks for catching this. This reference was stale and I've missed it.
Fixed in 2e97e2d
# Conflicts: # CHANGELOG.md # persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/QueryGeneratorTest.java
The release note pointed operators at `persistence.relationalJdbc.schemaName`, but the chart neither exposes nor renders that value: schema selection is configured through `persistence.relationalJdbc.additionalProperties`, whose shipped default sets `currentSchema`. An operator following the note would set a value that renders nothing and silently keep using the default schema. Point the note at `persistence.relationalJdbc.additionalProperties.currentSchema`, matching values.yaml and the generated Helm reference.
|
@adutra @flyrain @dimas-b @singhpk234 @snazy The only conflict resolutions of note were moving the two CHANGELOG entries back into Happily awaiting for additional reviews! |
flyingImer
left a comment
There was a problem hiding this comment.
I’m aligned with moving schema selection into datasource configuration, but I don’t think the current head is merge-ready against current main. Fresh bootstrap now selects schema v6, whose hard-coded POLARIS_SCHEMA statements override the configured schema, and the upgrade note misses existing JDBC URLs that already select another currentSchema. Once the latest schema scripts and regression test preserve datasource-selected placement, and the migration note covers that URL case, I have no other blockers from this pass.
| private static InputStream openSchemaScript() { | ||
| return DatasourceOperations.class.getClassLoader().getResourceAsStream("h2/schema-v5.sql"); | ||
| } |
There was a problem hiding this comment.
This test no longer covers the bootstrap path this PR will ship. Current main sets v6 as the latest schema, but openSchemaScript still loads v5; once merged, a fresh bootstrap executes v6, whose CREATE SCHEMA and SET SCHEMA statements reset the datasource-selected schema to POLARIS_SCHEMA. Could we keep the latest bootstrap path schema-agnostic? One option is to update the v6 scripts and load this test through DatabaseType.H2.openInitScriptResource(DatabaseType.H2.getLatestSchemaVersion()), so future schema bumps exercise the same invariant.
There was a problem hiding this comment.
You're right, and thanks. This would have shipped broken. I'll update the test reference and remove the schema related statements in the v6 scripts.
| the admin tool's `bootstrap` command. Existing deployments are unaffected — their schema already | ||
| exists, and the shipped `currentSchema` default (`POLARIS_SCHEMA`) preserves the previous | ||
| behavior on upgrade. |
There was a problem hiding this comment.
The “existing deployments are unaffected” upgrade contract has one exception: an installation whose JDBC URL already selects a different currentSchema. Before this PR, fully qualified POLARIS_SCHEMA queries ignored that setting; after this PR, unqualified queries follow it, and pgjdbc gives the URL value precedence over the shipped default, so an upgrade can point Polaris away from its existing tables. Could we make that exception and the safe upgrade state explicit? For example, ask operators to remove the existing setting or point it at the schema that already contains their Polaris tables before upgrading.
There was a problem hiding this comment.
While this may be unlikely, it still seems to be a valid exception. I'll update the changelog breaking-change note.
Schema v6 landed after this branch last merged main, and its scripts open with CREATE SCHEMA IF NOT EXISTS POLARIS_SCHEMA followed by SET search_path (SET SCHEMA on H2). Since DatabaseType.getLatestSchemaVersion() returns 6, a fresh bootstrap would run those statements, re-create the schema this branch stopped creating and re-select POLARIS_SCHEMA, overriding the schema the operator chose through the datasource. Drop both statements from the postgres, h2 and cockroachdb v6 scripts, matching v5. The historical v1-v4 scripts are left untouched. DatasourceOperationsSchemaTest pinned h2/schema-v5.sql, so it asserted the invariant against a script that is no longer the bootstrap path and passed while v6 broke it. Load the script through DatabaseType.H2.openInitScriptResource(getLatestSchemaVersion()) and assert the recorded version against the same helper, so a future schema bump exercises the invariant instead of silently bypassing it.
"Existing deployments are unaffected" holds except for an installation whose JDBC URL already sets currentSchema. Polaris previously qualified every query with POLARIS_SCHEMA, so the setting was ignored; queries are now unqualified and follow it, and the JDBC driver gives a value in the URL precedence over the currentSchema default shipped in the datasource configuration. Such an upgrade points Polaris away from its existing tables, and a subsequent bootstrap creates a second, empty set of tables in the other schema. Call the exception out in the CHANGELOG breaking-change note and in the Relational JDBC metastore docs, with the two safe pre-upgrade states: remove the setting, or point it at the schema that already holds the Polaris tables.
|
@adutra @dimas-b @flyingImer @flyrain @singhpk234 @snazy |
What & why
The Relational JDBC persistence backend hard-codes its database schema as
POLARIS_SCHEMA(inQueryGeneratorand the bootstrap SQL scripts), sooperators cannot run multiple Polaris deployments in a single database or
comply with a schema-naming policy. This makes the schema configurable while
keeping
POLARIS_SCHEMAas the default, so existing deployments areunaffected.
How
Following the review and dev-list discussion, the persistence layer is now
completely agnostic of the schema name; the schema is selected through
standard datasource configuration:
bootstrap scripts are pure, schema-agnostic DDL. Legacy schema versions
(v0–v4) are untouched.
currentSchemaconnection property. A default
(
quarkus.datasource.jdbc.additional-jdbc-properties.currentSchema=POLARIS_SCHEMA)is shipped in the runtime defaults and the admin tool, preserving the
previous behavior for upgrades and out-of-the-box use. A
currentSchemaset in the JDBC URL takes precedence over the shipped default (pgjdbc
documented behavior, verified empirically against PostgreSQL and
CockroachDB).
CREATE SCHEMAis a privilegedDBA operation. Fresh deployments create the schema first, then bootstrap
(see the breaking-change note in
CHANGELOG.md); the getting-startedassets perform this init step. Existing deployments are unaffected.
persistence.relationalJdbc.schemaNameas theoperator-facing knob, now rendering the datasource property.
fresh H2 database, H2 reports SQLSTATE
42S04(table not found, databaseempty) where a schema-qualified reference used to produce
schema-not-found;
isRelationDoesNotExistnow recognizes it so thepre-bootstrap fallback keeps working.
There is no Polaris configuration option and no extension-point change:
RelationalJdbcConfigurationis unchanged frommain.Testing
./gradlew :polaris-relational-jdbc:checkand./gradlew compileAllgreen.DatasourceOperationsSchemaTest(real H2) proves the layer isschema-agnostic end-to-end: the schema is created and selected purely via
the datasource URL, unqualified SQL resolves against it, and without a
datasource-selected schema tables land in the database default schema.
the datasource configured the way a real deployment would be: the schema
pre-created (the DBA step) and selected via the driver's
currentSchema.LocalIcebergCatalogRelationalOverlapTest)passes with the shipped default present (H2 ignores the unknown driver
property; H2 tests select their schema via URL settings).
schemaName.Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)Disclosure: prepared with AI assistance (Claude Code); the author is
responsible for the change.
Fixes #4944