Skip to content

Conversation

@marcopaggioro
Copy link

@marcopaggioro marcopaggioro commented Nov 12, 2025

#174

  • VARCHAR tag in the Postgres schema of the durable_state table ➡️ VARCHAR without a fixed length is not available in MySQL, become VARCHAR(255), as specified in the Oracle and SQLServer schema.
  • state_payload BYTEA in the Postgres schema of the durable_state table ➡️ BYTEA is not available in MySQL, but become LONGBLOB.
  • global_offset BIGSERIAL in the Postgres schema of the durable_state table ➡️ BIGSERIAL (used as BIGINT with auto-increment) is not available in MySQL, become BIGINT AUTO_INCREMENT and add a UNIQUE KEY to global_offset.
  • Bypass SequenceNextValUpdater only for MySQL and use REPLACE query

UNIQUE KEY state_global_offset_uk (global_offset)
);
CREATE INDEX state_tag_idx on durable_state (tag);
CREATE INDEX state_global_offset_idx on durable_state (global_offset);
Copy link
Member

Choose a reason for hiding this comment

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

[nit] add a newline at end of file

Copy link
Author

Choose a reason for hiding this comment

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

👍

// case MySQL =>
// e shouldBe an[java.sql.SQLIntegrityConstraintViolationException]
case MySQL =>
e shouldBe an[java.sql.SQLIntegrityConstraintViolationException]
Copy link
Member

Choose a reason for hiding this comment

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

'a' not 'an' before 'j'

Copy link
Author

Choose a reason for hiding this comment

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

Just for MySQL with an inconsistency with the line below (case Oracle)?
Or both for Oracle and SqlServer (not H2 and Postgres)

case MySQL => slick.jdbc.MySQLProfile
case SqlServer => slick.jdbc.SQLServerProfile
case Oracle => slick.jdbc.OracleProfile
case _ => throw new UnsupportedOperationException(s"Unsupported <$s> for durableState.")
Copy link
Member

Choose a reason for hiding this comment

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

keep this - it causes no harm and is useful if we add a new db type and forget to support it here

Copy link
Author

Choose a reason for hiding this comment

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

If a new DBMS is added, isn't it easier to have a non-exhaustive match reported by the IDE (and even some plugins) rather than a case _ that would hide it?

} finally {
system.actorSelection("system/" + "pekko-persistence-jdbc-durable-state-sequence-actor").resolveOne().onComplete {
case Success(actorRef) => {
case Success(actorRef) =>
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a big fan of reformatting existing code that is not related to the PR

@@ -0,0 +1,21 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

new files should use the standard Apache header unless there are methods copied in from existing files - where we should use that file's header

eg https://github.com/apache/pekko-persistence-jdbc/blob/main/project/PekkoCoreDependency.scala#L1

@pjfanning
Copy link
Member

sbt scalafmtAll will fix the scala formatting issues

@marcopaggioro
Copy link
Author

I was fixing the tests when I realized that my code doesn't cover the case of "update the durable state record whose revision is -1 compared to the one I'm passing to the update" (WHERE clause in UPDATE #${durableStateTableCfg.columnNames.revision} = ${row.revision} - 1).

This is something you can't do with the REPLACE query.
Doing a SELECT before the REPLACE query isn't safe, and doing it within the REPLACE query isn't efficient.
I missed this point, so these changes aren't enough.
Honestly, I've found several workarounds to do this, but none that don't use locking, secondary tables, or inefficient query usage.
I have to retire from this PR, thanks for your time!

@pjfanning
Copy link
Member

@marcopaggioro thanks for having a look at solving the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants