Get Postgres Storage Provider Working #3384
Conversation
Add PostgreSqlStorageTests.
|
@ashtonkj, It will cover your contributions to all .NET Foundation-managed open source projects. |
|
@ashtonkj, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request. |
|
/cc @shayhatsor @veikkoeeva @halfnelson @rohail99 @dandago as you guys might help reviewing ang getting this in |
There was a problem hiding this comment.
Some minor notes from me. The tests indeed should pass. @ashtonK, have you tried this in a "normal setup" besides tests? I'm off from my own computer now, but I wouldn't like to delay this a day (or two) just because of that.
@jdom Should the documentation be updated to indicate there is a provider for PostgreSQL too? The documentation is at http://dotnet.github.io/orleans/Documentation/Getting-Started-With-Orleans/Grain-Persistence.html.
| @@ -859,3 +859,213 @@ VALUES | |||
| WHERE | |||
| ServiceId = @ServiceId AND @ServiceId IS NOT NULL; | |||
| '); | |||
|
|
|||
|
|
|||
| CREATE TABLE storage | |||
There was a problem hiding this comment.
For the sake of consistency with other scripts, using a capital 's' would be nicer here. I.e. define as Storage,
| THEN | ||
| _newGrainStateVersion := _GrainStateVersion + 1; | ||
| END IF; | ||
|
|
There was a problem hiding this comment.
Maybe should avoid this many empty lines.
| _PayloadXml, | ||
| (now() at time zone 'utc'), | ||
| 1 | ||
| WHERE NOT EXISTS |
There was a problem hiding this comment.
There could be a smarter way to do this in PostgreSQL, but maybe not an issue for this PR.
| GET DIAGNOSTICS RowCountVar = ROW_COUNT; | ||
| IF RowCountVar > 0 | ||
| THEN | ||
| _newGrainStateVersion := 1; |
There was a problem hiding this comment.
This is a nit, but the identations seem to be off here. This is unfortunately something I've introduced myself too to other scripts, but if you feel like labouring over this, maybe something to notice.
| ( | ||
| 'WriteToStorageKey',' | ||
|
|
||
| select * from WriteToStorage(@GrainIdHash, @GrainIdN0, @GrainIdN1, @GrainTypeHash, @GrainTypeString, @GrainIdExtensionString, @ServiceId, @GrainStateVersion, @PayloadBinary, CAST(@PayloadJson AS jsonb), CAST(@PayloadXml AS xml)); |
There was a problem hiding this comment.
As the previous nit, see intendentations.
| VALUES | ||
| ( | ||
| 'ReadFromStorageKey',' | ||
|
|
There was a problem hiding this comment.
Empty lines and intendation, see previous nits.
|
Thanks @veikkoeeva. I'll work on those this evening so that we can get it in at the right standard. |
|
@veikkoeeva Looks like there are a mix of tabs and spaces throughout the file. Do you mind if I increase the surface area of this PR by replacing all tabs with spaces (only in this file) so that we can get more consistent indentation? |
|
Absolutely, go for it |
Fix capitilisation of Storage table (though this is just for script purposes, Postgres will automatically lower case this anyway).
|
@veikkoeeva Changes made. |
|
This looks good to me. There could be room for improved performance in the future, but it's not a concern for this PR. |
|
@dotnet-bot test this please |
|
Thanks @ashtonkj and @veikkoeeva ! |
|
Seeing this only now (I've been travelling). So this adds support for PostgreSQL as a Storage Provider? Nice job! As @veikkoeeva suggested, we should probably update the docs once a release is shipped that includes this change. If you let me know when that happens, I could take care of it. |
|
@dandago it is in the version that is currently on MyGet. Over time we will probably optimise it more for Postgres, but it is working well in my current scenario. |
|
@ashtonkj OK, by MyGet I presume you mean an internal feed, and that it is not yet available on public NuGet. It is best to only update the docs when it is out there for public consumption, otherwise people would get confused. |
* Fix Postgresql CreateOrleansTables script to work as storage provider. * Add PostgreSqlStorageTests. * Normalize white-space.
* Fix Postgresql CreateOrleansTables script to work as storage provider. * Add PostgreSqlStorageTests. * Normalize white-space.
Fix Postgresql CreateOrleansTables script
Add PostgreSqlStorageTests.
Huge thanks to @halfnelson for getting the SQL script working. Tests seem to be passing except for those referenced in #3163 (comment).
See screenshot:
