Update PolarisEclipseLinkStore to work with more databases #247

eric-maynard · 2024-09-03T22:10:59Z

Description

Some of the code used by EclipseLink was not compatible with MySQL (or engines other than Postgres). This PR introduces several changes to the persistence layer while maintaining backwards compatibility.

Long VARCHARs

Many databases have more restrictive limits on column sizes than Postgres.

To support them, long VARCHARs in the persistence layer (properties and internalProperties) have been changed to TEXT.

Sequence Support

POLARIS_SEQ relies on a NEXTVAL native query that is not supported by most databases. It's removed, and backwards compatibility is preserved.

The sequence POLARIS_SEQ is deprecated and will get renamed to POLARIS_SEQ_UNUSED the first time it's used.

After POLARIS_SEQ is used that last time, POLARIS_SEQUENCE will be used as a sequence table and will track all IDs that have been assigned. From that point forward, the POLARIS_SEQUENCE table will be used to obtain new IDs.

If Polaris starts up and POLARIS_SEQ was never created or it was already renamed, then it should just be ignored.

Fixes #241

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
Documentation update
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

With this change, I am able to bootstrap Polaris backed by MySQL (view previous commits for e.g. persistence.xml changes)

Checklist:

Please delete options that are not relevant.

I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings

eric-maynard · 2024-09-04T00:45:01Z

I've refactored and will change the description to reflect the new approach.

eric-maynard · 2024-09-04T18:05:01Z

...e/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java

@@ -280,8 +283,8 @@ public void runActionInTransaction(
          LOGGER.debug("transaction committed");
        }
      } catch (Exception e) {
+        LOGGER.debug("Rolled back transaction due to an error", e);
        tr.rollback();


If this rollback fails, we never saw any logging previously.

Should we retry in certain case(e.g., database isn't available temporily) of rollback failure? It's not a blocker for this PR though. We can fix it in a follow-up PR if needed.

Agreed this is not in scope for this PR, but maybe we could consider it. As long as we maintain transactionality, anything is ok.

However my guess is that most (99+%) of failures will be non-recoverable. Before changing this logic, I would advocate for us collecting stats on recoverable failures and perhaps trying to tailor the retry to only recoverable failures.

Currently, we would retry if OptimisticLockException is hit - concurrent access. Yeah, we can collect what can be retried here. I remember the caller will do retry. Is that right?

In my tests, no the caller did not retry if this query failed.

flyrain

Thanks @eric-maynard for working on this. LGTM overall. Left some minor comments.

...e/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java

flyrain · 2024-09-11T22:19:26Z

...e/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java

@@ -280,8 +283,8 @@ public void runActionInTransaction(
          LOGGER.debug("transaction committed");
        }
      } catch (Exception e) {
+        LOGGER.debug("Rolled back transaction due to an error", e);
        tr.rollback();


Should we retry in certain case(e.g., database isn't available temporily) of rollback failure? It's not a blocker for this PR though. We can fix it in a follow-up PR if needed.

...n/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisSequenceManager.java

flyrain · 2024-09-11T22:50:08Z

...n/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisSequenceManager.java

+   * Generates a new ID from `POLARIS_SEQUENCE` or `POLARIS_SEQ` depending on availability. If
+   * `POLARIS_SEQ` exists, it will be renamed to `POLARIS_SEQ_UNUSED`.
+   */
+  public static Long getNewId(EntityManager session) {


renaming suggestion: getNewId() -> nextSequence()? Id is OK to use, but here we may use sequence for consistency.

Sequence is kind of an overloaded term. The table itself can be considered like a sequence. Previously, we used the postgres object of type "SEQUENCE" which is what caused issues.

Really, the user wants a single ID (BIGINT type) and they do not want a SEQUENCE object. So the PolarisSequenceManager, which manages the lifecycle of the sequence object/table, uses that sequence to generate and return an ID here.

flyrain · 2024-09-11T22:53:37Z

...n/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisSequenceManager.java

+  private static synchronized Optional<Long> getSequenceId(EntityManager session) {
+    DatabasePlatform databasePlatform = getDatabasePlatform(session);
+    if (databasePlatform instanceof PostgreSQLPlatform) {
+      Optional<Long> result = Optional.empty();
+      if (!sequenceCleaned.get()) {
+        try {
+          LOGGER.info("Checking if the sequence POLARIS_SEQ exists");
+          String checkSequenceQuery =
+              "SELECT COUNT(*) FROM information_schema.sequences WHERE sequence_name IN ('polaris_seq', 'POLARIS_SEQ')";
+          int sequenceExists =
+              ((Number) session.createNativeQuery(checkSequenceQuery).getSingleResult()).intValue();
+
+          if (sequenceExists > 0) {
+            LOGGER.info("POLARIS_SEQ exists, calling NEXTVAL");
+            long queryResult =
+                (long) session.createNativeQuery("SELECT NEXTVAL('POLARIS_SEQ')").getSingleResult();
+            result = Optional.of(queryResult);
+          } else {
+            LOGGER.info("POLARIS_SEQ does not exist, skipping NEXTVAL");
+          }
+        } catch (Exception e) {
+          LOGGER.info(
+              "Encountered an exception when checking sequence or calling `NEXTVAL('POLARIS_SEQ')`",
+              e);
+        }
+        if (result.isPresent()) {
+          removeSequence(session);
+        }
+        sequenceCleaned.set(true);
+      }
+      return result;
+    } else {
+      LOGGER.info("Skipping POLARIS_SEQ / NEXTVAL check for platform " + databasePlatform);
+      return Optional.empty();
+    }
+  }


We may extract a new method for Postgres like this, so that we confine the database specific code into to a separated method.

private static synchronized Optional<Long> getSequenceId(EntityManager session) { DatabasePlatform databasePlatform = getDatabasePlatform(session); if (databasePlatform instanceof PostgreSQLPlatform) { return getSequenceFromPostgres(session); } else { return Optional.empty(); } }

It's not strictly Postgres that supports SEQUENCE -- H2 also supports it, and other DBs support similar logic with different syntax.

Really the goal here is to patch this ASAP and then migrate all new usage to the table. Eventually, we can yank all this postgres-specific code.

Sorry I wasn't clear in my first comment. The code is mainly for migration in case anyone is already using Postgres in production. We have two approaches:

Ignore it, assuming no one is actually using it in production yet—especially since Apache Polaris hasn't had its first release.

Wrap the migration logic into a method so we can easily remove it in the future.

I prefer the second option. With that, we could make the following changes:

Call the migration code only in the initialization method, instead of reusing it in getNewId(), since the migration happens only once. In case of migration failure, we should just throw.

Simplify getNewId(), as we don't need to distinguish if it's migrated or not. Specifically, we can remove the line getSequenceId(session).ifPresent(modelSequenceId::setId);.

Eliminate PolarisSequenceUtil::sequenceCleaned and PolarisEclipseLinkStore::initialized, as we throwed in case of migration failure, there is no need to check if migration actually happened.

The migration code is getSequenceId itself. Eventually, it can be replaced with return Optional.empty() or the method can be ripped out.

I've moved most of this into initialize and shifted some things around -- let me know if this more closely matches your expectations

...n/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisSequenceManager.java

.../java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkStore.java

flyrain · 2024-09-12T00:44:33Z

...n/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisSequenceManager.java

+    // If a legacy sequence ID is present, use that as an override:
+    getSequenceId(session).ifPresent(modelSequenceId::setId);
+


I feel it is easier to understand if we do this

ModelSequenceId modelSequenceId = new ModelSequenceId(); DatabasePlatform databasePlatform = getDatabasePlatform(session); if (databasePlatform instanceof PostgreSQLPlatform) { getSequenceFromPostgres(session).ifPresent(modelSequenceId::setId); } // Persist the new ID: session.persist(modelSequenceId); session.flush();

I think it's better to encapsulate the implementation of getSequenceId in that private method.

The responsibility of getNewId is simply to create a ModelSequenceId and persist it. The implementation of how the ModelSequenceId gets its ID is not that relevant to readers of getNewId. I would be opposed to leaking getSequenceId implementation details (e.g. databases X, Y support method A, database Z supports method B) into this getNewId.

sfc-gh-aixu · 2024-09-16T17:40:22Z

...e/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java

@@ -280,8 +283,8 @@ public void runActionInTransaction(
          LOGGER.debug("transaction committed");
        }
      } catch (Exception e) {
+        LOGGER.debug("Rolled back transaction due to an error", e);
        tr.rollback();


Currently, we would retry if OptimisticLockException is hit - concurrent access. Yeah, we can collect what can be retried here. I remember the caller will do retry. Is that right?

sfc-gh-aixu · 2024-09-16T17:47:26Z

...main/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisSequenceUtil.java

+ * available it will be used then cleaned up. In all other cases the `POLARIS_SEQUENCE` table is
+ * used directly.
+ */
+class PolarisSequenceUtil {


Can you help me understand the purpose of this change? Is this to handle different syntax for different databases?

The existing code seems to only work with Postgres or a very limited set of databases because of the native query. We need to move away from that and choose a construction that works with all/most databases that EclipseLink supports. At the same time, we need to preserve the IDs already generated by any existing Postgres deployments.

flyrain

+1

flyrain · 2024-09-16T18:45:03Z

Thanks @eric-maynard for the PR. As a followup, we may figure out a deprecating strategy for the one-time migration. Hopefully we can have a release soon, so that we have a solid deprecating way based on the versions.

Thanks @aihuaxu for the review.

sfc-gh-aixu · 2024-09-16T19:01:58Z

@eric-maynard We have testCreateTasksInParallel() to test the id generation to make sure they are unique. Can you help run PolarisEclipseLinkMetaStoreManagerTest.testCreateTasksInParallel() against mysql database if we haven't done that?

eric-maynard · 2024-09-16T19:15:50Z

@aihuaxu after this change, Postgres should go through the new code path as well. So the existing test actually tests the new code path. What is not necessarily covered is the migration path, where you start using the old code and then upgrade. Big +1 to @flyrain's comment that we need a versioning & deprecation strategy. I think that will also help test things like this (cross-version compatibility)

sfc-gh-aixu · 2024-09-16T20:03:51Z

+1.

eric-maynard added 5 commits September 3, 2024 10:19

fix string size

fbf059c

check in

826d0bf

use two IDs

00cdf87

successful bootstrap

eabd1e2

working

a47cf76

eric-maynard requested review from jbonofre, ashvina, RussellSpitzer, snazy, vvcephei, takidau and jackye1995 as code owners September 3, 2024 22:11

eric-maynard added 2 commits September 3, 2024 15:11

reverts

a8583f5

add a comment

34190c3

eric-maynard marked this pull request as draft September 3, 2024 22:29

eric-maynard added 7 commits September 3, 2024 15:44

refactor for legacy table

5e8d0fb

enable postgres

7879675

refactors

e83d3f9

revert postgres-specific changes

472a435

cherry-pick postgres changes for now

79d0a9d

debug

ccf0e1b

lint

f14626a

eric-maynard marked this pull request as ready for review September 4, 2024 00:40

eric-maynard added 2 commits September 3, 2024 17:43

cleanups, reverts

c7c97ab

one fix

c9898b4

eric-maynard added 4 commits September 3, 2024 18:10

use auto sequence

a62c664

can bootstrap

bb6a44f

nit

6a03848

implement initialize

938d071

eric-maynard added 3 commits September 3, 2024 21:08

revert mysql changes

c1ac151

only do check for postgres

601e696

revert mysql persistence changes

abaf946

eric-maynard commented Sep 4, 2024

View reviewed changes

flyrain reviewed Sep 12, 2024

View reviewed changes

eric-maynard requested a review from flyrain September 13, 2024 18:23

eric-maynard added 5 commits September 13, 2024 11:23

some changes per revieW

d076784

autolint

b75bda8

check intialization

2218468

remove a method

c5747ed

autolint

e8a2d70

sfc-gh-aixu reviewed Sep 16, 2024

View reviewed changes

eric-maynard added 5 commits September 16, 2024 11:20

persist the ID during init

5151528

autolint

2bf0a08

fix comments

3e94c2a

autolint

cacb1cb

remove catch

3097cf4

flyrain approved these changes Sep 16, 2024

View reviewed changes

flyrain merged commit 537eee6 into apache:main Sep 16, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update PolarisEclipseLinkStore to work with more databases #247

Update PolarisEclipseLinkStore to work with more databases #247

eric-maynard commented Sep 3, 2024 •

edited

Loading

eric-maynard commented Sep 4, 2024

eric-maynard Sep 4, 2024

flyrain Sep 11, 2024

eric-maynard Sep 12, 2024

sfc-gh-aixu Sep 16, 2024

eric-maynard Sep 16, 2024

flyrain left a comment

flyrain Sep 11, 2024

flyrain Sep 11, 2024

eric-maynard Sep 13, 2024 •

edited

Loading

flyrain Sep 11, 2024

eric-maynard Sep 13, 2024

flyrain Sep 13, 2024

eric-maynard Sep 16, 2024 •

edited

Loading

eric-maynard Sep 16, 2024

flyrain Sep 12, 2024

eric-maynard Sep 13, 2024 •

edited

Loading

sfc-gh-aixu Sep 16, 2024

sfc-gh-aixu Sep 16, 2024

eric-maynard Sep 16, 2024

flyrain left a comment

flyrain commented Sep 16, 2024

sfc-gh-aixu commented Sep 16, 2024

eric-maynard commented Sep 16, 2024

sfc-gh-aixu commented Sep 16, 2024

		// If a legacy sequence ID is present, use that as an override:
		getSequenceId(session).ifPresent(modelSequenceId::setId);

Update PolarisEclipseLinkStore to work with more databases #247

Update PolarisEclipseLinkStore to work with more databases #247

Conversation

eric-maynard commented Sep 3, 2024 • edited Loading

Description

Long VARCHARs

Sequence Support

Type of change

How Has This Been Tested?

Checklist:

eric-maynard commented Sep 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flyrain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-maynard Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-maynard Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-maynard Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flyrain left a comment

Choose a reason for hiding this comment

flyrain commented Sep 16, 2024

sfc-gh-aixu commented Sep 16, 2024

eric-maynard commented Sep 16, 2024

sfc-gh-aixu commented Sep 16, 2024

eric-maynard commented Sep 3, 2024 •

edited

Loading

eric-maynard Sep 13, 2024 •

edited

Loading

eric-maynard Sep 16, 2024 •

edited

Loading

eric-maynard Sep 13, 2024 •

edited

Loading