ExistsSql on SQL Server and subclassing IDatabase

[baki-reveal]

i, thanks for grate. We've been using it for quite a while with both SQL Server and PostgreSQL.

While looking into a SQL Server performance issue, I found something that seems worth raising. I also ran into a related problem when trying to override it locally.

ExistsSql on SQL Server

AnsiSqlDatabase.ExistsSql uses INFORMATION_SCHEMA.TABLES with LOWER() on both columns. SqlServerDatabase does not override it, so SQL Server uses that implementation.

On SQL Server this is more expensive than a direct object lookup, and in our case these queries show up in blocking chains while application DDL is running. We counted 22 calls during a migration of a 364-table database.

I tried replacing it with a SQL Server-specific lookup using OBJECT_ID and sys.objects, which avoids querying INFORMATION_SCHEMA.TABLES.

Would you be open to a PR that adds a SQL Server override for ExistsSql? I'm happy to include tests. If there is a reason the current implementation is preferred, I'd be interested to know.

Subclassing IDatabase

Before changing grate itself, I tried subclassing SqlServerDatabase and overriding ExistsSql in our application.

That override is called, but the migration then creates no grate tables and eventually fails with:

Invalid object name 'grate.ScriptsRun'

The reason seems to be that the bootstrap scripts are located using:

this.Database.GetType()

The code then searches that type's assembly for the embedded bootstrap SQL. Since my subclass is in a different assembly, no resources are found. No error is raised at that point, so the migration continues until it later tries to use grate.ScriptsRun.

This made me wonder what the intended extension point is.

Is subclassing SqlServerDatabase or replacing IDatabase supported?
If not, would it make sense to throw an error when no bootstrap resources are found? That would make this failure much easier to diagnose.
If subclassing is intended to work, would you consider resolving the bootstrap resources from the provider's assembly rather than the concrete derived type?

I'm happy to put together a PR for either the SQL Server ExistsSql override or the bootstrap issue, depending on which approach you think is appropriate.

Thanks.

[wokket]

Hi,

We're always open to PR's that make things better for people. A lot of that underlying code was migrated directly from Roundhouse, so there's definitely room for better, more modern approaches. A couple of things to watch out for with this change are:

• Ensuring we don't break back-compat with older versions of SQL... I'm not too stressed if we break SQL 2000 at this point, but we'd want to know we've done it :)
• We have a bit of a history of issues with case-sensitive collations (again, largely inherited from RH), and so we'd want to keep an eye on that given the LOWER() currently in use.

In theory, subclassing IDatabase or AnsiDatabase in an external assembly should work, but it's old thing of "until someone goes to the effort to make it work, it wont". Again, we're very open to PR's here. Personally I use the pre compiled images/executables rather than running grate as code, so I'm not going to make that change myself.

Thanks for the detailed chat and thought you've put in so far 👍