Issue 217 in noda-time: Compile flag without BCL support (for SQLCLR)

[noda...@googlecode.com]

Status: New
Owner: ----

New issue 217 by mj1856: Compile flag without BCL support (for SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

Since SQL Server doesn't have any built-in support for time zone
conversions (while Oracle and MySql have tzdb support built in), I thought
I would try to use NodaTime from a SQLCLR user defined function. This came
up while I was looking into this question
http://stackoverflow.com/q/16231482/634824

Apparently, it's really difficult to use the BCL TimeZoneInfo class from
SQLCLR because it's marked with a HostProtection attribute with
MayLeakOnAbort=true. (yikes)

So of course, I thought NodaTime would be a great alternative. Well, at
first, SQL wouldn't even load the NodaTime assembly into its catalog.
There's a few restrictions that static fields must be marked readonly. We
had a few that weren't. Also, it didn't like caching the
TimeZoneInfo.Local value in a static field, so I had to bypass that. After
working through the exceptions it complained about during loading, I was
ultimately able to get the assembly to load and show up in the database's
assembly catalog.

Then I wrote a simple SQL CLR function:

[SqlFunction]
public static SqlDateTime UtcToZone(SqlDateTime dateTime, SqlString zoneId)
{
var zone = DateTimeZoneProviders.Tzdb[zoneId.Value];
var dtUtc = DateTime.SpecifyKind(dateTime.Value, DateTimeKind.Utc);
var instant = Instant.FromDateTimeUtc(dtUtc);
var dtConverted = instant.InZone(zone).ToDateTimeUnspecified();
return new SqlDateTime(dtConverted);
}

Of course, it needs some argument checking, but it should work as a simple
test. This loaded into SQL just fine. Then I tried to run it, as such:

select dbo.UtcToZone(GETUTCDATE(), 'America/Phoenix')

And I fail again. It seems just loading the NodaTime assembly initializes
the Bcl DateTimeZone provider, and calls into TimeZoneInfo, which gives the
same problems as before:

Msg 6522, Level 16, State 2, Line 7
A .NET Framework error occurred during execution of user-defined routine or
aggregate "UtcToZone":
System.TypeInitializationException: The type initializer
for 'NodaTime.DateTimeZoneProviders' threw an exception. --->
System.Security.HostProtectionException: Attempted to perform an operation
that was forbidden by the CLR host.

The protected resources (only available with full trust) were: All
The demanded resources were: MayLeakOnAbort

System.Security.HostProtectionException:
at NodaTime.TimeZones.BclDateTimeZoneSource.GetIds()
at NodaTime.TimeZones.DateTimeZoneCache..ctor(IDateTimeZoneSource source)
at NodaTime.DateTimeZoneProviders..cctor()
System.TypeInitializationException:
at NodaTime.DateTimeZoneProviders.get_Tzdb()
at NodaSQL.UserDefinedFunctions.UtcToZone(SqlDateTime dateTime,
SqlString zoneId)
.


So, it looks like getting full NodaTime implementation is going to have the
same problems because it references TimeZoneInfo. Is there any way we
could have some conditional compilation to compile NodaTime with only TZDB
support?

Do you think even without this that we'd run into other SQL CLR
restrictions?


--
You received this message because this project is configured to send all
issue notifications to this address.
You may adjust your notification preferences at:
https://code.google.com/hosting/settings

[noda...@googlecode.com]

Comment #1 on issue 217 by jonathan.skeet: Compile flag without BCL support
(for SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

I have no experience of SQL CLR restrictions, to be honest. If we had a
complete list of them *and* some automated way of telling whether or not we
comply with them, it might be feasible... but I don't know how easy that
would be to do.

To get rid of the BclDateTimeZoneProvider, you could use the PCL version
instead (or just build with the PCL symbol set). But that will still try to
use TimeZoneInfo.Local in methods to find the system default time zone. I
don't know whether it will work if you just don't call those methods.

Any idea how many people would be likely to use this support, if we went
ahead with it? Is SQLCLR use widespread?

(Don't get me wrong, it's a fun idea to play around with - I'm just dubious
about the value for the effort of properly supporting it.)

[noda...@googlecode.com]

Comment #2 on issue 217 by malcolm.rowe: Compile flag without BCL support
(for SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

I agree: I don't think we're going to get very far without TimeZoneInfo at
all.

(I do note that there's no reason the BCL provider needs to be eager,
though, but I'll do that separately.)

[noda...@googlecode.com]

Comment #3 on issue 217 by mj1856: Compile flag without BCL support (for
SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

I know its usage is normally discouraged, but I wonder if the old TimeZone
class could be of use here? I checked and it appears that
TimeZone.CurrentTimeZone.StandardName is identical to
TimeZoneInfo.Local.Id. It doesn't appear that it changes based on culture
or ui culture. At least for the TZDB, when we're calling
DateTimeZoneProviders.Tzdb.GetSystemDefault() to use for CLDR lookup, it
could avoid TimeZoneInfo class. I'll prototype on some of this locally
with SQLCLR and see if I get anywhere. If it works, I'll check in to my
fork for you to review.

With regards to who might actually use this - I do read a lot of S.O.
questions about time zone support in databases. In particular, Oracle and
MySql have built-in time zone support using TZDB. SQL Server has nothing -
not even support for Windows time zones. I do know people that count on
SQLCLR on a regular basis. This would be a great item to have in their
toolchest.

I've seen more than a few cases of people storing timezone tables in their
databases and writing complex logic to work with them - just to find that
they never update it. Ever. They usually oversimplify the problem also,
so their tables aren't very accurate. It would be a killer solution to
simply use NodaTime directly in the database for adding TZDB support to SQL
Server.

Probably having some simple SQLCLR UDFs defined in a separate assembly
would make things even more useful. So a developer unfamiliar with SQLCLR
could simply add the assemblies through the SQL Management Studio and get
access to a variety of useful conversion functions they can start using
right away. Assuming I can get the basic stuff working, I'd be willing to
publish this as a separate project.

[noda...@googlecode.com]

Comment #4 on issue 217 by jonathan.skeet: Compile flag without BCL support
(for SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

Okay, it sounds like it's at least worth investigating further. Note that
StandardName may not be the same as Id in all situations - but we already
have a mapping for situations where it's not, as Id isn't available on the
PCL. I'd expect TimeZone.StandardName to be the same as
TimeZoneInfo.StandardName.

I'd still want to use TimeZoneInfo.Id in the "normal" build though, I think.

I wonder how feasible it is to run the unit tests within SQL Server, as a
good way of proving that everything works...

[noda...@googlecode.com]

Comment #5 on issue 217 by mj1856: Compile flag without BCL support (for
SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

Quite easy actually, thanks to SSDT w/ VS2012:

http://blogs.msdn.com/b/ssdt/archive/2012/12/07/getting-started-with-sql-server-database-unit-testing-in-ssdt.aspx

Like I said, if we can get over some of the basics of the NodaTime assembly
working in SQL-CLR (as a "Safe" assembly), then I'll be glad to spin up a
NodaSQL project separately, unit tests and all. (NodaSql?
NodaTime.SqlServer? hmmm...)

[noda...@googlecode.com]

Comment #6 on issue 217 by mj1856: Compile flag without BCL support (for
SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

Well, the good news is that I was able to get NodaTime working in SQLCLR.

It works as-is when loaded into SQL as an "unsafe" assembly. But I think
we can do better than that.

As a "safe" assembly, it gets stuck on the following areas:

- Static fields that should be marked readonly. (This should probably be
done anyway):
- NodaTime.Calendars.IslamicCalendarSystem.Calendars
- NodaTime.Text.ParseResult.TimeSeparatorMismatch
- NodaTime.Text.ParseResult.DateSeparatorMismatch
- NodaTime.Text.ParseResult.MissingNumber
- NodaTime.Text.ParseResult.UnexpectedNegative

- Assignment of value to static field
- NodaTime.DateTimeZoneProviders.xmlSerializationProvider
- NodaTime.TimeZones.BclDateTimeZone.systemDefault

- lock statements

If I remove the xml stuff, skip caching the default system zone, and
comment out all of the lock statements, then it loads as a "safe" assembly.
AND IT WORKS!

Of course, that seems a bit scary. Especially removing the locks. I'm
sure they're there for good reason. But using a lock statement causes an
exception because it invokes HostProtection with ExternalThreading and
Synchronization - which only work with fulltrust/unsafe.

Some background:
http://www.codeproject.com/Articles/17855/SQL-Server-CLR-Integration-Part-1-Security

There's also this:
http://stackoverflow.com/q/1284628/634824

The hack mentioned there to use a [CompilerGenerated] attribute didn't
work. I think MS have plugged that hole.

The lock exception messages look like this:

System.Security.HostProtectionException: Attempted to perform an operation
that was forbidden by the CLR host.
The protected resources (only available with full trust) were: All

The demanded resources were: Synchronization, ExternalThreading

I suppose I can start looking through the locks one at a time to see if
they are really necessary or if there are workarounds.

Also, I didn't try accessing the local time zone. The lazy-load that
Malcolm put it worked so that TimeZoneInfo didn't load. I think we could
write something like BclDateTimeZone.FromTimeZone instead
of .FromTimeZoneInfo.

[noda...@googlecode.com]

Comment #7 on issue 217 by malcolm.rowe: Compile flag without BCL support
(for SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

I marked the static fields as readonly in r4cf86a9ff27e, which was easy
enough.

However, the "assignment of value to static field" check seems fairly
unreasonable:
- for xmlSerializationProvider, it's true that this is mutable static
state, but there isn't a reasonable way to avoid that given XML
serialisation.
- for BclDateTimeZone.systemDefault, it's a mutable cache as an
optimisation. I'm not sure why this would be unreasonable.

Likewise, I don't see that we can realistically avoid some of the lock
statements.


Backing up a bit: how useful is NodaTime as an unsafe assembly? Would that
in practice make it less usable, or would it just be better if we could
mark it as safe? (Sorry, it's been a while since I've done anything with
SQLCLR, and it's probably changed since then anyway.)

[noda...@googlecode.com]

Comment #8 on issue 217 by mj1856: Compile flag without BCL support (for
SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

This is my first venture into SQLCLR also, so I'm probably not the best one
to talk about best practices are. But the number of hoops you have to jump
through to use an unsafe assembly are discouraging to say the least. And
the documentation at every step along the way tries to talk you out of it.
So while it might be functional, I don't think it's a good idea for a 3rd
party library like NodaTime to run unsafe. Certainly, it will get much
more real-world use if it can run as a safe assembly.

I'm going to proceed cautiously, with a separate build configuration and a
SQLCLR conditional compilation flag (like we have the PCL build). We can
always undo it later if I uncover any optimizations that make sense outside
of SQLCLR.

I'm a bit unsure of what the xml stuff is used for anyway, and why it needs
to be mutable. Can I just skip this for the SQLCLR build?

Regarding BclDateTimeZone.systemDefault - It would seem that SQLCLR doesn't
want you to do any of your own internal caching. I can bypass the cache
for the SQLCLR build. But I wonder if there is some other mechanism that
it would prefer instead. I'll have to do some more research.

It appears most of the lock statements we have are also part of caching
logic. Surely there is some recommended way to handle caching safely in
SQLCLR. If I can figure out what mechanism they prefer, I can probably
work around the existing locks.

On a hunch, I checked to see if ConcurrentDictionary is allowed - but no,
it has the same problems as a lock.

[noda...@googlecode.com]

Comment #9 on issue 217 by mj1856: Compile flag without BCL support (for
SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

http://stackoverflow.com/q/16303557/634824

[noda...@googlecode.com]

Comment #10 on issue 217 by jonathan.skeet: Compile flag without BCL

support (for SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

Okay, I *think* I'd like to support this as another build target of Noda
Time if possible - while forking would obviously work, it would cause more
work in the long run. It sounds like we may even want two different build
configurations - an "unsafe" one which still does caching, and a "safe" one
which is slower due to not caching at all.

Most of the caching should be relatively easy to opt out of. One problem I
can foresee is DateTimeZoneCache. The obvious fix here is to make that
*eagerly* load all the time zones, before construction completes. That way
we never need to lock, but we still get all the caching behaviour.

How important is performance in SQLCLR likely to be? The cache can make a
huge difference in time zone calculations. For the Gregorian calendar
up-front cache for 1900-2100 (year and month starts) probably means the
BasicCalendar cache becomes a lot less relevant.

Are you doing this for work (where it's presumably relatively urgent) or
would it be okay to defer this for now, and then take it up seriously
post-1.2? (We could put it on the roadmap either before, after or
coincident with the CLDR work.)

[noda...@googlecode.com]

Comment #11 on issue 217 by mj1856: Compile flag without BCL support (for
SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

As you may have seen by the responses to my S.O. question, there doesn't
appear to be a "safe" way to do thread-safe caching in a SQLCLR object. It
seems to want to be completely single-threaded.

Since the normal build works fine in unsafe mode as-is, I don't think we
need a build target for that. It would just be the one new target that
doesn't do any caching, and therefore need no locks. I also like your idea
about eagerly loading the zone data. It would have to go into a readonly
static field. I think this would be just fine for something like SQL
Server.

It's not super important for work - no. It can wait. I'm thinking more
about usefulness to others than to myself. I'd love to see NodaTime
everywhere. I'm more into RavenDB than SQL Server these days anyway. :)

I'd love to help though. I can certainly at least do all of the SQL
specific parts. The goal being to have ready-to-use functions that make
sense to a SQL developer.

[noda...@googlecode.com]

Comment #12 on issue 217 by wiyono.a...@gmail.com: Compile flag without BCL

support (for SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

Sorry if it is a separate issue, but I stumbled upon this ticket searching
for why DateTimeZoneProviders.Tzdb.GetSystemDefault on PCL build throws
DateTimeZoneNotFoundException when the display language (this is on WP8) is
set to non-English. Digging into the source, I found that on PCL, the
library uses TimeZoneInfo.StandardName instead of normally TimeZoneInfo.Id
for looking up the mapping. This apparently, is a localised name
(http://msdn.microsoft.com/en-GB/library/system.timezoneinfo.standardname(v=vs.95).aspx)
so this would not work properly. Eg. "Westeuropäische Zeit" instead of "GMT
Standard Time" Or is this a known issue?

[noda...@googlecode.com]

Comment #13 on issue 217 by jonathan.skeet: Compile flag without BCL

support (for SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

@wiyono.aten: Ah, humbug. That's a real pain. Could you file that as a
separate bug? (It's not really related to this one.)

I'll have to see what I can do about it - I'm sure I've seen other times
where changing culture *doesn't* change the result of StandardName, so it's
quite possible that it's dependent on *exactly* how the culture is changed.
I can see this being a pain to fix, but I'll do what I can.

[noda...@googlecode.com]

Comment #14 on issue 217 by wiyono.a...@gmail.com: Compile flag without BCL

support (for SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

Yes it is unfortunately, as we just found this out quite late in our
development cycle at the moment :) I have filed a separate issue now
https://code.google.com/p/noda-time/issues/detail?id=221. I can see the fix
can be real pain, so thanks!

[noda...@googlecode.com]

Updates:
Labels: Type-Enhancement Milestone-unscheduled

Comment #15 on issue 217 by malcolm.rowe: Compile flag without BCL support
(for SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

(No comment was entered for this change.)

[noda...@googlecode.com]

Comment #16 on issue 217 by mj1856: Compile flag without BCL support (for
SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

Just an update, I may be able to complete this for the 1.2 milestone. I've
learned a lot about SQLCLR and have most of it done (locally). I'll be
publishing to my fork soon for review.

The only real strangeness is a few of the existing lock statements now look
like this:

#if !SQL
lock (foo)
#endif
{
//...
}

While this looks a bit scary (intentionally ignoring a lock), it turns out
that SQLCLR calls are always single-threaded anyway. So the lock is
extraneous.

Of course we'll want to do some concurrency testing just to make sure all
holds up. But I think it will be good.

I think it will be a very nice story to say that Noda Time 1.2 works with
SQL Server. I also want to prove that it will work with EF/NH/L2S so
people can start using Noda Time types in their data models.

[noda...@googlecode.com]

Comment #17 on issue 217 by jonathan.skeet: Compile flag without BCL

support (for SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

I'd rather not make this change before 1.2, to be honest - it sounds like a
large change to put in *just* before a release, especially as we've still
got some build/packaging changes. I'd be very happy to punt the future
releases so that 1.3 could be purely SQLCLR, so that it doesn't need to be
too far out. Does that sound reasonable?

(I'm surprised about the threading part - do you have any links so I could
read more about that?)

[noda...@googlecode.com]

Comment #18 on issue 217 by mj1856: Compile flag without BCL support (for
SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

That sounds just fine. That gives more time for testing and documentation
as well.

Regarding the threading model, I'll see if I can dig up some links. I've
already forgotten where I read that.

[noda...@googlecode.com]

Updates:
Labels: -Milestone-unscheduled Milestone-1.3.0

Comment #19 on issue 217 by jonathan.skeet: Compile flag without BCL

support (for SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

Duly set as milestone 1.3 :)

Will update the roadmap later. Depending on how much you need to do and how
far I get in my thinking, we could try to fit the "out of bounds" handling
into that release too, or punt the latter to 1.4 (with CLDR).

[noda...@googlecode.com]

Comment #20 on issue 217 by mj1856: Compile flag without BCL support (for
SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

This presentation was one of the most informative I could find:
http://www.sqlbits.com/Sessions/Event7/SQL_CLR_Demystified
A few things relevant to threading at:
- Slide 6, Video at 05:26
- Slide 54, Video at 45:27

Some MSDN docs here:
http://msdn.microsoft.com/en-us/library/ms131047.aspx

I'm not sure "single threaded" is the right terminology. It does have the
ability to work with multiple threads, although any thread management is
specifically prohibited in "SAFE" assemblies. The docs talk about how
SQL's threading model is significantly different than the CLR, so any
threading should be avoided.

I guess what I'm most interested here is in whether SQL queues up
concurrent calls to CLR objects or if they might run in parallel and
potentially cause a collision on our caches. I have more research and
testing to do.

[noda...@googlecode.com]

Comment #21 on issue 217 by jonathan.skeet: Compile flag without BCL

support (for SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

Great, thanks - will have to look all of that up. The locking in 1.2 has
moved around a bit. If you can do without text processing, that would get
rid of a lot of it :)

[noda...@googlecode.com]

Comment #22 on issue 217 by jonathan.skeet: Compile flag without BCL

support (for SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

I think I've worked out a way of avoiding the lock for
YearMonthDayCalculator, by cunningly storing all the relevant information
in a single int. It'll take a bit of work to implement it and then compare
the performance, but I have some hopes that going lock-free could be good...

[noda...@googlecode.com]

Comment #23 on issue 217 by Ivan.Ant...@gmail.com: Compile flag without BCL

support (for SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

Dear All,

do you have any schedule/plan on when 1.3 with sqlclr support will ship?
(and if you've actually decided to include SQLCLR support in 1.3?)

[noda...@googlecode.com]

Comment #24 on issue 217 by jonathan.skeet: Compile flag without BCL

support (for SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

@Ivan: I believe Matt no longer has personal use of the SQLCLR stuff, so
may not have worked on it for a while. I'm hoping to ship v1.3 "soonish"
with a collection of minor features. If the SQLCLR code is nearly ready to
merge in, I'd be happy for it to go in. Over to Matt :)

[noda...@googlecode.com]

Comment #25 on issue 217 by mj1856: Compile flag without BCL support (for
SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

Looks like I have some work to do. :)

It will probably be a few weeks, but I think it is possible.

I do have some working prototype code from last year, but it needs to be
refreshed before it's ready for consumption.

[noda...@googlecode.com]

Comment #26 on issue 217 by Ivan.Antsipau: Compile flag without BCL support
(for SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

@John, @Matt, thanks for your quick replies. @Matt, please, let me know if
there's anything I can help you with.

[noda...@googlecode.com]

Updates:
Labels: -Milestone-1.3.0 Milestone-1.4-consider

Comment #27 on issue 217 by malcolm.rowe: Compile flag without BCL support
(for SQLCLR)
http://code.google.com/p/noda-time/issues/detail?id=217

(No comment was entered for this change.)