Running Drupal 7.9, when I try to create a node using a non-admin account, I'm getting this error as soon as click to create any node type (I'm using PostgreSQL 9 database backend):

PDOException: SQLSTATE[42803]: Grouping error: 7 ERROR: column "td.vid" must appear in the GROUP BY clause or be used in an aggregate function at character 23: SELECT td.tid AS tid, td.vid AS vid, BIT_OR(COALESCE(ta.grant_create, tad.grant_create, tadg.grant_create)) AS grant_create FROM {taxonomy_term_data} td INNER JOIN {taxonomy_access_default} tadg ON tadg.vid = 0 LEFT OUTER JOIN {taxonomy_access_default} tad ON tad.vid = td.vid AND tad.rid = tadg.rid LEFT OUTER JOIN {taxonomy_access_term} ta ON ta.tid = td.tid AND ta.rid = tadg.rid WHERE (tadg.rid IN (:db_condition_placeholder_0, :db_condition_placeholder_1)) AND (td.vid IN (:db_condition_placeholder_2)) GROUP BY td.tid; Array ( [:db_condition_placeholder_0] => 2 [:db_condition_placeholder_1] => 6 [:db_condition_placeholder_2] => 1 ) in _taxonomy_access_user_term_grants() (line 1594 of /u01/html/drupal-7.9/sites/all/modules/taxonomy_access/taxonomy_access.module).

To make the function _taxonomy_access_user_term_grants actually generate correct SQL that is compliant with PostgreSQL, I provide the following patch. I have no idea if I'm breaking something with my patch, but it appears to be functioning okay after this fix. Appears in PostgreSQL (and most relational databases) that you gotta put all the non-aggregating fields in the group by list if you are gonna select them. So "SELECT td.tid AS tid, td.vid AS vid" is generated by the DB abstraction layer classes and thus td.tid and td.vid must be in the group by clause (or vice versa). So my patch removes the td.vid from the select clause. Dunno if that's the right solution.

CommentFileSizeAuthor
#1 1334848-taxonomy_access_user_term_grants-postgresql.patch396 bytesalphasupremicus

Comments

alphasupremicus’s picture

alphasupremicus commented
Title: Can't create nodes with non-admin account due to PostgreSQL SQL exception » Attaching my patch
StatusFileSize
new 1334848-taxonomy_access_user_term_grants-postgresql.patch396 bytes
alphasupremicus’s picture

alphasupremicus commented
Title: Attaching my patch » Can't create nodes with non-admin account due to PostgreSQL SQL exception

Adjusting the issue title back to a sensible one...geesh - been a while since I used this site.

xjm’s picture

xjm
she/her
Primary language English
commented

Thanks for the report. Looks like this is just an error in porting to the new DB API. I'm not sure we can just remove the condition, but I'll look into it.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Active » Needs review

Sending to testbot, just out of curiosity.

alphasupremicus’s picture

alphasupremicus commented
Status: Needs review » Active

The part I removed was related to a field in the SELECT clause - which your group by clause didn't use anyways. The change seems benign - removing a field that was seemingly unused from the SQL. But it's not my call. Maybe there are test cases that would pass or fail based upon this change?

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Active » Needs review

Right, that's why I sent it to the bot. :) Seems to pass, though. The fact that the field isn't used in that particular query doesn't mean it's never used, though--those query builders are used for pretty much every query the module does. So it bears more investigation.

The test coverage is incomplete, as well. It mostly tests form functionality at present.

alphasupremicus’s picture

alphasupremicus commented

Ok - thanks for the info. Well, in the end, strictly speaking, it's illegal SQL to not include all non-aggregated columns in the GROUP BY clause.

i.e. SELECT a, b, sum(c) must have GROUP BY a, b per the ANSI standard.

I've seen this behavior in other Drupal modules too and MySQL must run the SQL anyways and PostgreSQL does not (since I presume PostgreSQL strives for standards compliance more than MySQL - never researched this specifically, but that's my observation).

Anyways, so I wonder if this is a "usage" problem of the new DB API for Drupal 7 or a bug in the DB API that needs reported to the Drupal core team? Thoughts?

xjm’s picture

xjm
she/her
Primary language English
commented

It's most likely that I messed up the GROUP BY clause when I ported the query. :) I'll try to find out if there's an existing core issue, though, and get some advice from a core dev.

TAC is also using the addExpression() method for the BIT_OR(COALESCE()) here, which takes things out of the DB API's hands somewhat. BIT_OR() is supported by Postgres and MySQL, but is not ANSI. (See #1266674: Support other databases.)

xjm’s picture

xjm
she/her
Primary language English
commented

Of note: #1491542: testHavingCountQuery() fails on pgsql and sqlite

scratchfury’s picture

scratchfury commented

Just wanted to check in to see if this issue will be patched at any point. An alternative approach to the patch supplied above would be to add td.vid to the groupBy clause, instead of removing the field from the selection. Any thoughts from the maintainer as to which is the more appropriate solution? Here's the change I propose, starting at line 1529:

OLD:

  // Filter by the indicated vids, if any.
  if (!empty($vids)) {
    $query
      ->fields('td', array('vid'))
      ->condition('td.vid', $vids, 'IN')
      ;
  }

NEW:

  // Filter by the indicated vids, if any.
  if (!empty($vids)) {
    $query
      ->fields('td', array('vid'))
      ->groupBy('td.vid')
      ->condition('td.vid', $vids, 'IN')
      ;
  }

Note that all I did was add td.vid to the groupBy clause, which seems the appropriate solution since td.vid is only included in the query if any vids exist. This seemed to fix the issue for me on Drupal 7.15 with TAC 7.x-1.x-dev. Hope that helps!

xjm’s picture

xjm
she/her
Primary language English
commented

That seems like a reasonable fix. Want to roll a patch so others can test it? :)