Skip to content

Add fallback in case the VERSION table is not present #2653

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Sep 30, 2025
Merged

Add fallback in case the VERSION table is not present #2653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
af3b117
initial commit
eric-maynard Sep 23, 2025
8cfccde
wire up
eric-maynard Sep 23, 2025
0b7960e
pastefix
eric-maynard Sep 23, 2025
3f5b0b4
Merge branch 'main' of github.com:apache/polaris into safer-version-c…
eric-maynard Sep 26, 2025
d2f42a9
change to postgres specific code
eric-maynard Sep 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ...bc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ public class DatasourceOperations {
private static final Logger LOGGER = LoggerFactory.getLogger(DatasourceOperations.class);

private static final String CONSTRAINT_VIOLATION_SQL_CODE = "23505";
private static final String RELATION_DOES_NOT_EXIST = "42P01";

// POSTGRES RETRYABLE EXCEPTIONS
private static final String SERIALIZATION_FAILURE_SQL_CODE = "40001";
Expand Down Expand Up @@ -394,6 +395,10 @@ public boolean isConstraintViolation(SQLException e) {
return CONSTRAINT_VIOLATION_SQL_CODE.equals(e.getSQLState());
}

public boolean isRelationDoesNotExist(SQLException e) {
return RELATION_DOES_NOT_EXIST.equals(e.getSQLState());
}

private Connection borrowConnection() throws SQLException {
return datasource.getConnection();
}
Expand Down
6 changes: 5 additions & 1 deletion ...src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -725,7 +725,8 @@ public boolean hasChildren(
}
}

static int loadSchemaVersion(DatasourceOperations datasourceOperations) {
static int loadSchemaVersion(
DatasourceOperations datasourceOperations, boolean fallbackOnDoesNotExist) {
PreparedQuery query = QueryGenerator.generateVersionQuery();
try {
List<SchemaVersion> schemaVersion =
Expand All @@ -736,6 +737,9 @@ static int loadSchemaVersion(DatasourceOperations datasourceOperations) {
return schemaVersion.getFirst().getValue();
} catch (SQLException e) {
LOGGER.error("Failed to load schema version due to {}", e.getMessage(), e);
if (fallbackOnDoesNotExist && datasourceOperations.isRelationDoesNotExist(e)) {
return SchemaVersion.MINIMUM.getValue();
}
throw new IllegalStateException("Failed to retrieve schema version", e);
}
}
Expand Down
9 changes: 8 additions & 1 deletion ...main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@
import javax.sql.DataSource;
import org.apache.polaris.core.PolarisCallContext;
import org.apache.polaris.core.PolarisDiagnostics;
import org.apache.polaris.core.config.BehaviorChangeConfiguration;
import org.apache.polaris.core.config.PolarisConfigurationStore;
import org.apache.polaris.core.config.RealmConfig;
import org.apache.polaris.core.context.RealmContext;
import org.apache.polaris.core.entity.PolarisEntityConstants;
Expand Down Expand Up @@ -74,6 +76,7 @@ public class JdbcMetaStoreManagerFactory implements MetaStoreManagerFactory {
@Inject PolarisStorageIntegrationProvider storageIntegrationProvider;
@Inject Instance<DataSource> dataSource;
@Inject RelationalJdbcConfiguration relationalJdbcConfiguration;
@Inject PolarisConfigurationStore configurationStore;

protected JdbcMetaStoreManagerFactory() {}

Expand All @@ -98,7 +101,11 @@ private void initializeForRealm(
// RealmContext (request-scoped bean) can still create a JdbcBasePersistenceImpl
String realmId = realmContext.getRealmIdentifier();
// determine schemaVersion once per realm
final int schemaVersion = JdbcBasePersistenceImpl.loadSchemaVersion(datasourceOperations);
final int schemaVersion =
JdbcBasePersistenceImpl.loadSchemaVersion(
datasourceOperations,
configurationStore.getConfiguration(
realmContext, BehaviorChangeConfiguration.SCHEMA_VERSION_FALL_BACK_ON_DNE));
sessionSupplierMap.put(
realmId,
() ->
Expand Down
2 changes: 2 additions & 0 deletions ...bc/src/main/java/org/apache/polaris/persistence/relational/jdbc/models/SchemaVersion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
import org.apache.polaris.persistence.relational.jdbc.DatabaseType;

public class SchemaVersion implements Converter<SchemaVersion> {
public static final SchemaVersion MINIMUM = new SchemaVersion(0);
Copy link
Contributor

@flyrain flyrain Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/polaris-catalog/polaris/blob/eb7618bce8b4ecb259f0030d5d24458aa2d91343/persistence/relational-jdbc/src/main/resources/postgres/schema-v1.sql#L26-L26

JDBC impl. is only with the v1 schema. There is no v0 for jdbc implementations. Should we use 1 here?

Copy link
Contributor Author

@eric-maynard eric-maynard Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that's why we want to use 0 -- the v1 schema file includes the VERSION table, so the lack of a VERSION table indicates an earlier (pre-1) schema.

We don't use this MINIMUM number to choose a schema file, only the reverse -- the detect what schema version an already-running metastore was bootstrapped with.

Copy link
Contributor

@flyrain flyrain Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The v1 schema released with 1.0.x doesn't include the version table.
Also, do we want to distinguish v0 and v1 in the code base? I'd prefer not.

Copy link
Contributor Author

@eric-maynard eric-maynard Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we do need to distinguish them. For a given Polaris version e.g. 1.1.0, we have a notion of a v1 schema as defined by the v1 schema file and that contains the version table. If there's a metastore without the version table, it definitely can't be v1.

Copy link
Contributor

@flyrain flyrain Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might miss something. What would be behavior difference between a 1.0 v1 schema and a 1.1 v1 schema? I think they should be identical. The problem we are trying to resolve is that the 1.0 v1 schema doesn't have a version table when 1.1 Polaris deployed. The system behavior should be the same as if there is a version table with version 1.

Copy link
Contributor Author

@eric-maynard eric-maynard Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can have two different schema versions without any behavior difference between them, so I'm not sure I follow your question.

The problem we are trying to solve is identifying the schema version for a metastore without a schema version table. The schema version table starts at version 1. A metastore without this table is therefore a pre-1 version.

Copy link
Contributor

@dimas-b dimas-b Sep 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think we have any formal requirement for the schema versions to be comparable to each other numerically.

Current JDBC Persistence code makes some numerical comparisons, but I tend to think that this is an implementation concern. As long as the interpretation of schema version numbers is consistent with the schemas themselves, the code is fine.

With that in mind, using 0 for missing schema information should produce correct runtime behaviour, as far as I can tell.

Copy link
Contributor

@flyrain flyrain Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will need to use the schema version for things proposed here, https://lists.apache.org/thread/5d9rl1l2jflbbnrl12ofmczjbcw8qv89. "Pre-1" and "1" are going to have the same behavior in terms of how we handling the missing column location_without_schema.

Copy link
Contributor

@flyrain flyrain Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a minor comment for me. Feel free to merge it as is.


private final Integer value;

public SchemaVersion() {
Expand Down
10 changes: 10 additions & 0 deletions polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,4 +84,14 @@ protected BehaviorChangeConfiguration(
+ " credential vending.")
.defaultValue(false)
.buildBehaviorChangeConfiguration();

public static final BehaviorChangeConfiguration<Boolean> SCHEMA_VERSION_FALL_BACK_ON_DNE =
PolarisConfiguration.<Boolean>builder()
.key("SCHEMA_VERSION_FALL_BACK_ON_DNE")
.description(
"If set to true, exceptions encountered while loading the VERSION table which appear to be"
+ " caused by the VERSION table not existing will be interpreted as meaning that the"
+ " schema version is currently 0.")
.defaultValue(true)
.buildBehaviorChangeConfiguration();
}