field_sql_storage_field_storage_query() with 'count' option breaks on PostgreSQL and SQLite [#706248]

field_sql_storage_field_storage_query() uses COUNT(DISTINCT f1, f2, f3), which is not supported by any current versions of Postgres. Support is apparently in the code base for Postgres 9.0. Current versions only support a single expression in COUNT(DISTINCT ) as does MSSQL and Oracle. The only other engine providing current support for a list of fields in COUNT(DISTINCT ) is DB/2 from version 8.

Patch rebuilds the query into the form SELECT COUNT(*) FROM (SELECT DISTINCT ...) ... since this offers better performance and fewer syntax compatibility problems than using CONCAT and CAST to create a single expression.

Comment	File	Size	Author
#10	706248-count-query-with-distinct.patch	3.81 KB	Damien Tournoud
#10
#8	field_sql_storage_count_2.patch	1.23 KB	bellHead
#8
#3	field_sql_storage_count_1.patch	1.27 KB	bellHead
#3
	field_sql_storage_count.patch	1.28 KB	bellHead

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

bellHead CreditAttribution: bellHead commented 6 February 2010 at 06:36

Status:

Active

» Needs review

Comment #2

yched CreditAttribution: yched commented 6 February 2010 at 13:51

Will the patch impact performance on MySQL ? If so, should this go in a Postgres-only conditional code branch ?

Style-wise, the comment needs to use //, not /* ... */

Comment #3

bellHead CreditAttribution: bellHead commented 7 February 2010 at 08:08

File	Size
field_sql_storage_count_1.patch	1.27 KB

I'm not sure what the expected distribution of distinct values is in this case so I ran two performance tests.

There is a 10% performance penalty on a 55000 record set with 120 distincts and 10% performance gain on a 55000 record set with 30000 distincts. Performance neutral overall I'd say.

Comment marker style changed. The style guide seems to permit both, at what length of comment should the multiline comment style start being used?

Comment #4

10 February 2010 at 04:10

Status:	Needs review	» Needs work
Issue tags:	-PostgreSQL Surge, -Fields in Core

The last submitted patch, field_sql_storage_count_1.patch, failed testing.

Comment #5

bellHead CreditAttribution: bellHead commented 10 February 2010 at 19:07

Status:	Needs work	» Needs review
Issue tags:		+PostgreSQL Surge, +Fields in Core

#3: field_sql_storage_count_1.patch queued for re-testing.

Comment #6

Josh Waihi CreditAttribution: Josh Waihi commented 11 February 2010 at 07:10

Status:

Needs review

» Reviewed & tested by the community

Awesome, this makes sense. And the test bot is happy also.

Comment #7

Dries CreditAttribution: Dries commented 11 February 2010 at 08:08

+++ modules/field/modules/field_sql_storage/field_sql_storage.module	7 Feb 2010 07:52:24 -0000
@@ -540,8 +540,16 @@ function field_sql_storage_field_storage
+    // Postgres < 9.0 (SQL99 COUNT(DISTINCT ) only takes a single expression)

This has some style issues: extra space before ), needs to end with . instead of ).

Quick reroll?

Comment #8

bellHead CreditAttribution: bellHead commented 11 February 2010 at 12:07

File	Size
field_sql_storage_count_2.patch	1.23 KB

Rerolled, shortened the bit on SQL99 since I couldn't get it to read nicely.

Comment #9

Damien Tournoud CreditAttribution: Damien Tournoud commented 16 February 2010 at 15:13

Status:

Reviewed & tested by the community

» Needs work

I don't buy that. We should make sure that:

db_select($table)
  ->fields($table, array('field1', 'field2'))
  ->distinct()
  ->countQuery()
  ->execute()
  ->fetchField();

works as intended, ie: returns the count of distinct fields.

Comment #10

Damien Tournoud CreditAttribution: Damien Tournoud commented 16 February 2010 at 15:36

Status:

Needs work

» Needs review

File	Size
706248-count-query-with-distinct.patch	3.81 KB

This patch implements proper support of distinct clauses in SelectQuery::countQuery(), uses that in field_attach_query() and adds an additional unit test.

Comment #11

Damien Tournoud CreditAttribution: Damien Tournoud commented 16 February 2010 at 18:10

Title:

field_sql_storage_field_storage_query() with 'count' option breaks on Postgres

» field_sql_storage_field_storage_query() with 'count' option breaks on PostgreSQL and SQLite

And yes, SQLite is affected too.

Comment #12

bellHead CreditAttribution: bellHead commented 16 February 2010 at 19:46

Looks good. I wasn't aware that DBTNG was intended to support that construct. I have updated the page on the multi-field DISTINCT MySQLism accordingly