SQL Server Compatiblity [#2044223]

During deployment on SQL Server we found serveral compatiblity issues.

I attach patches in non GIT format, hope this can be formally posted and commited at some time to the core!

The changes are minor and should brake no functionality.

Comment	File	Size	Author
#8	2044223-sql-server-compat.patch	586 bytes	longwave
#1	uc_attribute.admin_.inc_.patch	1020 bytes	david_garcia
	uc_reports.admin_.inc_.patch	3.99 KB	david_garcia
	uc_attribute.admin_.inc_.PATCH	723 bytes	david_garcia

Comments

Comment #1

david_garcia commented 17 July 2013 at 18:12

Status	File	Size
new	uc_attribute.admin_.inc_.patch	1020 bytes

Updated file with new incompatiblity issues solved.

Comment #2

tr commented 18 July 2013 at 00:27

Priority:	Major	» Normal
Status:	Active	» Postponed (maintainer needs more info)

Please read #2012186: Product attribute adjustment page has PDOException with PostgreSQL and SQL Server

It looks like you're doing two things in your patches (correct me if I'm wrong - you didn't explain what errors you were seeing, what actions caused those errors, or what you changed and why you changed it, so it's quite possible I missed some subtlety here...):

1) adding groupBy() clauses, presumably because SQL Server is similar to PostgreSQL in requiring that all result fields appear in a groupBy() clause if you want to order the results.

2) Renaming some of the placeholders, presumably because the SQL Server DBTNG driver has a problem if the same placeholder appears in more than one expression.

I consider both of these problems to be problems with the Drupal SQL Server driver. If that's all there is, you should look in the issue queue for the driver to see if it's been reported before and if not open an issue there with a minimal example showing a query that is written to Drupal specs but fails on SQL Server.

Comment #3

david_garcia commented 18 July 2013 at 02:08

Sorry for the lack of detail, I had to solve this in a rush and just wanted to share...

1) Totally true. This will never have a solution at a driver level (i've had several discussions on this in the driver's issue queue) because, indeed, it is a argumentable error not to include the fields in the group by statement. MySql is preserving this behaviour just not to brake compatibility because people are used to this loose way of constructing statements.

Someone has taken the time to explain this with some plain language, but it is still a bit difficult to understand:

http://weblogs.sqlteam.com/jeffs/archive/2007/07/20/but-why-must-that-co...

2) This issue has been discused in another issue of the SQL Server Driver and the conclusion is that it is wrong for a module to use duplicate placeholders:

https://drupal.org/node/1905324#comment-7494256

Per the Drupal database placeholder documentation, "A query may have any number of placeholders, but all must have unique names even if they have the same value.".

Comment #4

david_garcia commented 18 July 2013 at 02:12

By the way, I forgot to update on one the patches there is a wrong statement:

->addExpression('CASE WHEN po.ordering IS NULL THEN NULL ELSE 1 END', 'null_order');

should be

->addExpression('CASE WHEN po.ordering IS NULL THEN 1 ELSE 0 END', 'null_order');

Comment #5

tr commented 23 July 2013 at 01:21

Yes, I see what you're saying about placeholders. We should fix that. Perhaps we should also do something about the groupBy(), although I'm not convinced - it still sounds like a problem either at the Drupal API level or at the driver level. Can you provide a link to the " several discussions on this in the driver's issue queue" so we can read them?

Also, I'm concerned about the change in #4; Raw SQL in an expression is not very portable - is there a way you can get the same result using the DB API?

Comment #6

longwave

he/him

English

commented 2 August 2013 at 22:16

I have committed the placeholders part of the uc_reports patch, thanks for this.

Regarding group by, I believe requiring all columns in an aggregate query is considered standard in most SQL implementations other than MySQL, otherwise the result may be non-deterministic. I don't think the core DB layer can or should protect against all possible bad queries. Further discussion of this problem in uc_attribute should be handled in #2012186: Product attribute adjustment page has PDOException with PostgreSQL and SQL Server as Postgres suffers from the same issue.

For ordering NULL first, then CASE .. WHEN .. THEN seems to be standard SQL, and should be supported by all SQL implementations, so I think this is safe, but this would require a bit of testing.