Skip to content
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.

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

Added DbBatch support #148

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Added DbBatch support #148

wants to merge 1 commit into from

Conversation

jarlebh
Copy link

@jarlebh jarlebh commented Jun 9, 2022

Fixes #147

Changes

Added new PostgreDbBatchSqlJournal that uses DbBatch when persisting, this increases performane of PersistAll x 20, also added .net6 support as this is needed to get right version of DbBatch in .net6

Checklist

For significant changes, please ensure that the following have been completed (delete if not relevant):

Latest dev Benchmarks

Include data from the relevant benchmark prior to this change here.

This PR's Benchmarks

Include data from after this change here.

@Aaronontheweb
Copy link
Member

cc @CumpsD any thoughts on this?

Copy link
Member

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

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

Some of your new unit tests are failing, but also - how do we handle this for users who can't upgrade to newer driver versions due to Entity Framework diamond dependencies?

I'd love to incorporate this change but I sincerely need community members to weigh in.

@@ -4,7 +4,7 @@
<PropertyGroup>
<AssemblyTitle>Akka.Persistence.PostgreSql</AssemblyTitle>
<Description>Akka Persistence journal and snapshot store backed by PostgreSql database.</Description>
<TargetFrameworks>$(NetStandardLibVersion)</TargetFrameworks>
<TargetFrameworks>$(NetStandardLibVersion);$(NetVersion)</TargetFrameworks>
Copy link
Member

Choose a reason for hiding this comment

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

We're generally planning on making dual targeting to .NET 6 available for Akka.NET v1.5, but I guess it doesn't harm much to have it here.

@@ -46,8 +46,9 @@ public PostgreSqlQueryExecutor(PostgreSqlQueryConfiguration configuration, Akka.
{Configuration.TagsColumnName} VARCHAR(100) NULL,
{Configuration.SerializerIdColumnName} INTEGER NULL,
CONSTRAINT {Configuration.JournalEventsTableName}_uq UNIQUE ({Configuration.PersistenceIdColumnName}, {Configuration.SequenceNrColumnName})
);";

); CREATE INDEX IF NOT EXISTS IX_{Configuration.JournalEventsTableName}_{Configuration.SequenceNrColumnName} ON {Configuration.FullJournalTableName} USING btree (
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a migration SQL script to apply this to existing journal configs?

Copy link
Author

Choose a reason for hiding this comment

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

Sure, automatically or just a SQL file ? This index is needed for large Journals

Copy link
Member

Choose a reason for hiding this comment

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

@jarlebh a SQL file is fine - users should apply it manually.

src/common.props Outdated
@@ -14,11 +14,12 @@
<PropertyGroup>
<XunitVersion>2.4.1</XunitVersion>
<XunitRunnerVersion>2.4.3</XunitRunnerVersion>
<AkkaVersion>1.4.35</AkkaVersion>
<PostgresLowVersion>5.0.11</PostgresLowVersion>
Copy link
Member

Choose a reason for hiding this comment

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

How do we reconcile that change with this one? #125

.NET Core 3.1 support sunsets this December.

Copy link
Author

Choose a reason for hiding this comment

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

Actually we might be able to compile with lower version, I could try. However it will not work if you try to use the new PostgreSqlDbBatchQueryExecutor

@CumpsD
Copy link
Member

CumpsD commented Jun 23, 2022

cc @CumpsD any thoughts on this?

If it passes the test, I'm all for it, since it is opt-in anyway via HOCON

I do have to mention I am not busy with akka.net on postgresql (or akka.net in general) right now due to lack of time

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.

[PERF] Use DbBatch to increase PersistAll performance
3 participants