Orleans: Reminders period overflow issue in ADO.NET Reminders Table

[xontab]

In the ADO.NET reminders table implementation, the period is stored in milliseconds as Integer. Thus it is not possible to set it more than 24.8 days, as it will overflow and thus the wrong value is stored in the DB. At the moment there is no validation so it will not throw any exceptions or errors when this happens.

https://github.com/dotnet/orleans/blob/38e1e86f5af92994bcc83b37c5228df95be6b892/src/AdoNet/Shared/Storage/DbStoredQueries.cs#L399-L402

We are ready to create a PR with a fix, however there are a number of options:

• Option 1: Change it to Long - This will ensure backwards compatibility with the existing DB Schema
• Option 2: Change it to String - Similar to other reminder storage providers (example Azure and DynamoDB)
• Option 3: Change it to Double - Same type used for TotalMilliseconds to support both whole and fractional milliseconds

What do you think?

[sergeybykov]

Tagging @veikkoeeva for his opinion.

[veikkoeeva]

I think Orleans cannot support fractional milliseconds and I'm not aware of such plans. Also looking at https://github.com/dotnet/orleans/blob/master/src/Orleans.Runtime/ReminderService/ReminderRegistry.cs#L30 the maximum is 4294967294 ms of type uint, or 49.7 days while calculations use long if I got my mental pseudocalculations correct.

In practice see:

1. All database scripts needs to be changed to long to match code.
2. Needs to be changed in the code.
3. Tests run. I can run the MySQL and PostgreSQL and/or SQL Server depending what is needed if this is needed.

So, the change to long a necessary fix unless @sergeybykov or @xontab can come up with other arguments.

As a side note, I remember this topic came up in the very early days while checking other bugs during refactoring the codebase. In hindsight this ought to have refactored already back then, but the plan was also to make a better implementation of the reminder table (narrow name indexes, good range index for queries taking cues from current Storage (which came later)). In enterprise space there are solutions and 3rd party services doing this in scaleable with good enough accuracy and resolution, so this is an important subservice. Here we are again, glad to see this receiving attention. Go for it! :)

[xontab]

Thanks @veikkoeeva. Yes I agree that long is the best option at the moment since this will guarantee backwards compatibility with the old schema and requires no changes in the existing ReminderRegistry (since long is already used for both dueTime and period including validation).

Will create a PR with the agreed changes and test on all the different DB engines.

[xontab]

@veikkoeeva Created a PR and tested on all the supported engines. I had to add a condition for backwards compatibility with the old SQL schemas to still use int when value is in range. Otherwise it will throw the following exception.

Connection id "0HLU5JP0HU7QQ", Request id "0HLU5JP0HU7QQ:00000003": An unhandled exception was thrown by the application.
Npgsql.PostgresException (0x80004005): 42883: function upsert_reminder_row(text, text, text, timestamp without time zone, bigint, integer) does not exist
   at Npgsql.NpgsqlConnector.<>c__DisplayClass160_0.<<DoReadMessage>g__ReadMessageLong|0>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at Npgsql.NpgsqlConnector.<>c__DisplayClass160_0.<<DoReadMessage>g__ReadMessageLong|0>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at Npgsql.NpgsqlDataReader.NextResult(Boolean async, Boolean isConsuming)

[veikkoeeva]

@xontab Sounds like you did quality work. :) I'll give eyes tomorrow as its late already today.

[veikkoeeva]

@sergeybykov, @ReubenBond Can this be closed now that #6390 was merged?

[sergeybykov]

Resolved via #6390.

Thank you, @xontab and @veikkoeeva!