[BUG] Potential eclipselink schema upgrade issue

[snazy]

Is this a possible security vulnerability?

• This is NOT a possible security vulnerability

Describe the bug

The change #438 changes the database schema. However, there seems to be an issue that the schema is not properly updated - at least not via PolarisEclipseLinkMetaStoreManagerTest, see repro steps below.

/cc @eric-maynard

To Reproduce

1. git checkout e232b000760da335721e9112f9d52fe041289613 (commit before Do not persist plaintext secrets in the metastore #438)
2. rm -rf extension/persistence/eclipselink/build (just get a clean state)
3. ./gradlew :polaris-eclipselink:test --rerun (passes fine)
4. git checkout ab095cfab84e5b4e694c72011f4166589eb25f38 (Do not persist plaintext secrets in the metastore #438)
5. ./gradlew :polaris-eclipselink:test --rerun (fails)
6. rm -rf extension/persistence/eclipselink/build/test_data (database state)
7. ./gradlew :polaris-eclipselink:test --rerun (passes fine)

Actual Behavior

No response

Expected Behavior

No response

Additional context

No response

System information

No response

[eric-maynard]

I'm able to repro this using the steps provided.

I believe that the simplest fix is to enable automatic schema migrations in eclipselink by setting:

<property name="eclipselink.ddl-generation" value="create-or-extend-tables"/>

By adding this between steps (4) and (5) in your instructions, I am able to get the tests in step (5) to mostly pass -- PolarisEclipseLinkMetaStoreManagerTest.testPrivileges still fails, but I think that's a different issue.

I've noted this down this on the PR that caused this, as well.

[eric-maynard]

For the time being, shall we revert the offending PR?

I am also happy to fix-forward with the config suggested above. In any case, I think this is a good indicator that cross-version tests would be a good idea.

[snazy]

Honestly, I don't think that reverting is an option.

Cross-version tests (or backwards/forwards/migration compatibility tests) are a good idea!
SQL schema migration however complicates things, especially in a distributed / horizontally scalable system w/ rolling upgrades.

Re cross-version tests (backwards/forwards compatibility + rolling upgrades), we do have something like that in Nessie, where we run REST services from older releases and test the newest client (on main) against it, and older clients against the newest REST service (on main). It requires quite a bit of class-loading trickery though. With Iceberg/Java (which we don't use in those tests in Nessie) it would be quite hard, because Iceberg/Java and especially the Azure SDK keep resources like threads and thread-locals around, which cause class leaks that then cause OOMs - not great.

However, testing relevant things (persistence is one) in isolation should be doable - as dedicated / isolated tests.

[eric-maynard]

I see. Yeah, I think it will be complicated to set up true cross-version tests. However since we do have a persistence layer I think it will be important to ensure that upgrades/downgrades can work with various states in the persistence layer. Other stuff, like cross-version notification compatibility, could come later.

Filed this PR to fix-forward: #456

I want to look more closely at the one test that's failing; I think that's a legitimate issue with #438. Perhaps the ID sequence does not get preserved when the schema is automatically upgraded.

[collado-mike]

Honestly, I don't think that reverting is an option.

Why is this not an option?

[eric-maynard]

cc @jbonofre -- with the release coming up, how do you want to approach this?

Currently, anyone who was using EclipseLink-backed Polaris prior to #438 (who has not configured EclipseLink for automatic schema migrations) will not be able to upgrade to 0.9.0 without a manual schema migration. On the other hand, if we revert, anyone who created a metastore between #438 merging and being reverted will be stuck.

[adutra]

anyone who was using EclipseLink-backed Polaris prior to #438 (who has not configured EclipseLink for automatic schema migrations) will not be able to upgrade to 0.9.0 without a manual schema migration

If they haven't set up schema migration, that's expected right? Couldn't we just mention this fact in the release notes and keep #438 in 0.9.0?

[eric-maynard]

@adutra that would be reasonable, too. I guess the question is whether we want to force users to do schema migrations (maybe only across minor/major versions?) or whether we want this to be something Polaris itself handles.

In either case I think it should be an explicit decision. If we want users to do this manually, we should not merge #456

[danielbwatson]

In case this helps someone in the future, if you are using Postgres for your Polaris database you can use the sql from this gist to update your database from pre-release (~Aug) Polaris to the 0.9 schema. The highlighted portion will salt your secrets.