Add storage sqlserver #591

bpfz · 2024-11-22T15:01:20Z

No description provided.

Ulimo

Added some initial comments, will continue checking

Ulimo · 2024-11-23T15:59:38Z

src/FlowtideDotNet.Storage.SqlServer/Data/BaseSqlRepository.cs

+{
+    internal abstract class BaseSqlRepository : IDisposable
+    {
+        public Dictionary<long, HashSet<ManagedStreamPage>> ManagedPages { get; private set; }


I dont fully understand why this needs to have a hashset?
Cant it keep only the latest version information?

Yes we could probably remove that, it's kinda a remnant from the previous solution and I was a bit unsure if we wanted to keep track of previous version here. But considering how delete etc. removes old rows we can just keep the latest one.

Ulimo · 2024-11-23T16:01:42Z

src/FlowtideDotNet.Storage.SqlServer/Data/BaseSqlRepository.cs

+        public virtual void AddStreamPage(long key, byte[] value)
+        {
+            var page = new StreamPage(ToPageKey(key, Stream.Metadata.CurrentVersion), Stream.Metadata.StreamKey, key, value, Stream.Metadata.CurrentVersion);
+            UnpersistedPages.Add(page);


I wonder if a check is required here or not if UnpersistedPages reaches a certain value, say 32mb or similar of data and it would force saving at that point.

Say an operator loads in 1 billion+ rows but the stream is set to use low amount of memory, this could cause the stream to go over that memory limit.

In the "session"/streamPageRepository the unpersited pages gets written if the WritePagesBulkLimit setting is reached. So it should be similar in functionality, just that it writes based of number of items rather than size. We could maybe use "size" for the setting instead?

In the dictionary for "managed pages" should we have some similar "rule" ie clear some items after a certain point or do we feel like that can be kept "forever"?

I did not add it to the base method since the "storage" repository does not have that functionality that it writes after a certain number of items. Reasoning was that it will be a low number of pages added through that repository.

Ulimo · 2024-11-25T09:32:33Z

tests/FlowtideDotNet.Storage.SqlServer.Tests/SqlStorageTests.cs

+            await session.Write(pageId, Encoding.UTF8.GetBytes(pageId.ToString()));
+            await session.Commit();
+            await session.Delete(pageId);
+            var storedPage = await session.Read(pageId);


storedPage should be null here, since it was deleted, but should not be null if it was restored from previous checkpoint

Removed this test case, since it tested a case that shouldn't exist.

Ulimo · 2024-11-25T09:34:25Z

tests/FlowtideDotNet.Storage.SqlServer.Tests/SqlStorageTests.cs

+            for (int i = 1; i <= numberOfPages; i++)
+            {
+                var page = await session.Read(i);
+                Assert.NotEmpty(page);


maybe an assert for content aswell to make sure data returned is correct

Added, also added two (one for storage and one for session) explicit tests for asserting written data is returned.
94e2476

Added, also added two (one for storage and one for session) explicit tests for asserting written data is returned on read.

Ulimo · 2024-11-25T09:36:22Z

tests/FlowtideDotNet.Storage.SqlServer.Tests/SqlStorageTests.cs

+            await session.Commit();
+
+            await storage.CheckpointAsync([], false); // new version
+            await session.Write(pageId, Encoding.UTF8.GetBytes("1"));


maybe change from "1" to "2" and make sure it is reverted back to "1" in this test.

This assert has been added in 7062770.

…ed pages outside the bg task

…e last successful version of the stream

…is removed newly inserted row versions (since the stream is not updated yet)

…ize removed unsuccessful rows

…re occurs. Also changed the managed pages to only hold a single page version

…d only delete the lowest version from the parent select

…then old versions

Ulimo · 2024-12-05T10:52:02Z

src/FlowtideDotNet.Storage.SqlServer/Data/SessionRepository.cs

+        {
+            await WaitForBackgroundTasks();
+            await SaveStreamPagesAsync();
+            // todo: do we need to delete old versions etc. here as well (or only on checkpoint)?


We only delete after the checkpoint or at the same time as a checkpoint (in a transaction)

github-actions

Benchmark

Benchmark suite	Current: `a738ff6`	Previous: `7db1083`	Ratio
`FlowtideDotNet.Benchmarks.Stream.StreamBenchmark.InnerJoin`	`525948410` ns (`± 12232438.308807356`)	`588638700` ns (`± 9231369.002121696`)	`0.89`
`FlowtideDotNet.Benchmarks.Stream.StreamBenchmark.LeftJoin`	`628923520` ns (`± 30603702.457230017`)	`632393477.7777778` ns (`± 23156158.76418074`)	`0.99`
`FlowtideDotNet.Benchmarks.Stream.StreamBenchmark.ProjectionAndNormalization`	`194127860` ns (`± 19741954.10541835`)	`171268600` ns (`± 6644227.006958748`)	`1.13`
`FlowtideDotNet.Benchmarks.Stream.StreamBenchmark.SumAggregation`	`222199900` ns (`± 8188662.915953441`)	`190822830` ns (`± 6415026.852121856`)	`1.16`
`FlowtideDotNet.Benchmarks.Stream.StreamBenchmark.ListAggWithMapAggregation`	`2141062200` ns (`± 132086665.61217383`)	`2656394200` ns (`± 136074715.58524996`)	`0.81`

This comment was automatically generated by workflow using github-action-benchmark.

…onexisting), updated and added tests

…ss on TMetadata

…called for session that calls write

Ulimo

This looks g!ood Thank you for the pull request!

bpfz added 12 commits October 2, 2024 14:36

added storage initialization metadata argument to IPersistentStorage

d6036dd

added project and initial implementation for sql storage

e1515aa

added overloaded metho for providing all settings

588b77b

changed to interlocked instead of checking bag page count directly

8ef67c2

changed to CopyTo when creating a copy before saving

7ba827a

added Commit for persistent storage sessions.

5c61da6

removed old implementation

d838b57

removed unused file

2224cb9

added new slq server storage implementation

e919f51

added test project and initial tests for sql server storage

44e32af

cleaned usings

eb5e8cb

removed call for stream.reset, this is done in the caller of ResetStream

2af936a

Ulimo reviewed Nov 23, 2024

View reviewed changes

Ulimo reviewed Nov 25, 2024

View reviewed changes

bpfz added 13 commits November 27, 2024 14:51

added tests for asserting written data is returned on read

94e2476

changed write background task to take a copy of the current unpersist…

f41507f

…ed pages outside the bg task

updated testcontainer to newest version

1e297c3

changed DeleteOldVersionsInDbAsync to only delete rows if it's not th…

6fdacc8

…e last successful version of the stream

changed to not delete unsuccessful stream page rows on checkpoint. Th…

11f3fb5

…is removed newly inserted row versions (since the stream is not updated yet)

added assert that the checkpointed page row is returned after initial…

7062770

…ize removed unsuccessful rows

changed InitializeAsync to wait and clear session repositories on init

56c301e

changed to not allow "deleted" session pages to be read until a resto…

ca14319

…re occurs. Also changed the managed pages to only hold a single page version

fixed test to get stream key after initialize

c72e404

changed repository names to make it more clear what their usage is

1678b8c

changed DeleteOldVersionsInDb to filter on stream key for subquery an…

d6ba9a2

…d only delete the lowest version from the parent select

changed order on initialize, removes unsuccessful versions first and …

643abb8

…then old versions

removed unused using

a8666cc

Ulimo reviewed Dec 5, 2024

View reviewed changes

bpfz added 2 commits December 5, 2024 12:07

removed todo comment, situation is clarified and no action required

adb05e9

merged from main

43500fb

github-actions bot reviewed Dec 5, 2024

View reviewed changes

bpfz added 5 commits December 5, 2024 14:22

added $(DefaultNetTargetFramework)

66117fc

added exceptions when trying to read unavailable pages (deleting or n…

19dc1b8

…onexisting), updated and added tests

moved session commit out of the metadata commited check

b4ba0a9

extracted WriteMetadata method on commit, added type constrant to cla…

83fe0cc

…ss on TMetadata

added DebugWrite logger that conditionally writes on debug

f9edd55

bpfz force-pushed the add_storage_sqlserver branch from f0df34c to f9edd55 Compare December 6, 2024 16:20

bpfz added 5 commits December 17, 2024 14:46

added requirement that session commit is called before checkpoint is …

953a317

…called for session that calls write

added settings for StreamTableName and StreamPageTableName

3b0a540

Add support for alternate schema in SQL tests

8b37cb7

added documentation section for sql server storage

eae3e88

added example to setup storagetables in another schema than default

10e1f47

Ulimo approved these changes Dec 19, 2024

View reviewed changes

Merge branch 'main' into add_storage_sqlserver

a738ff6

bpfz merged commit 9ac7ced into main Dec 19, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add storage sqlserver #591

Add storage sqlserver #591

bpfz commented Nov 22, 2024

Ulimo left a comment

Ulimo Nov 23, 2024

bpfz Nov 27, 2024

Ulimo Nov 23, 2024

bpfz Nov 27, 2024

bpfz Nov 27, 2024

Ulimo Nov 25, 2024

bpfz Dec 5, 2024

Ulimo Nov 25, 2024

bpfz Nov 27, 2024 •

edited

Loading

bpfz Nov 27, 2024

Ulimo Nov 25, 2024

bpfz Nov 28, 2024

Ulimo Dec 5, 2024

github-actions bot left a comment •

edited

Loading

Ulimo left a comment

Add storage sqlserver #591

Add storage sqlserver #591

Conversation

bpfz commented Nov 22, 2024

Ulimo left a 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

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

bpfz Nov 27, 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

Choose a reason for hiding this comment

github-actions bot left a comment • edited Loading

Choose a reason for hiding this comment

Benchmark

Ulimo left a comment

Choose a reason for hiding this comment

bpfz Nov 27, 2024 •

edited

Loading

github-actions bot left a comment •

edited

Loading