[Postgres] GROUP BY uses field names instead of aliases

tagging for next Postgres Code Spring

Subscribing

Hi two questions:

1. Does anyone know when the next code sprint for postgres will be? This bug is breaking a large portion of my views, so i'd like to make some plans around what to do. :-)

2. Does anyone know if this bug is also present in 3.x / 2.5 ? I could just upgrade / downgrade if not.

Thanks!

Patrick

@phayes the New Zealand based team will likely have another mini-code sprint a couple weeks after Drupalcon.

I can confirm that this error also exists for 2.7 ... i assume its also in the current 2.x-dev version.

With the help of Damz (via irc) and fuerst we where able to fix the problem. Patch attached!

#6: That patch only works if the field is a real field. If it is a formula, I think that patch will cause errors. Try a date field, for example.

I extended the above view with a date field (field_event_date) and got the below listed query. At least there is no error. Is there a way to check if it's a formula or a real field? Shouldn't be $field['table'] (line 969) and $this->base_table (line 907) be empty, if it is a formula?


SELECT node.nid AS nid,
SUM(search_index.score * search_total.count) AS score,
node_revisions.body AS node_revisions_body,
node_revisions.format AS node_revisions_format,
node_data_field_event_date.field_event_date_value AS node_data_field_event_date_field_event_date_value,
node.type AS node_type,
node.vid AS node_vid
FROM node node
LEFT JOIN search_index search_index ON node.nid = search_index.sid
LEFT JOIN search_total search_total ON search_index.word = search_total.word
LEFT JOIN node_revisions node_revisions ON node.vid = node_revisions.vid
LEFT JOIN content_type_event node_data_field_event_date ON node.vid = node_data_field_event_date.vid
WHERE (search_index.word = 'lorem') AND (search_index.type = 'node')
GROUP BY search_index.sid, node.nid, node_revisions.body, node_revisions.format, node_data_field_event_date.field_event_date_value, node.type, node.vid
HAVING COUNT(*) >= 1


DamZ said " the first hunk is fine" and he suggested to check $field['table'] like:
$non_aggregates[] = (!empty($field['table']) ? $field['table'] . '.' : '') . $field['field'];

Patch changed.

does #607418: Column 'nid' in field list is ambiguous query apply to this situation? if so, see the patch I attached over there.

Ok, here is what I want to do:

In Views 2.x, I'm ok with patch #9.

In Views 3.x, I'd like to fix the real problem. The real problem is that we add the base field without its table, so instead of 'node_nid' it's 'nid'. This is really just a holdover from Views 1, where it was acceptable to refer to the base field directly in the query. In Views 3 that is actually not acceptable, thanks to the addition of group by. Therefore, a patch for Views 3 should remove the special handling for the base field in the query object and make sure any references to the base field directly (i.e, ->cid ->nid etc) are caught and fixed. Those are all wrong anyway.

So I need 2 patches for this.

If i remove

// We check for this specifically because it gets a special alias.
if ($table == $this->base_table && $field == $this->base_field && empty($alias)) {
$alias = $this->base_field;
}

The code does brake the groupby support. I don't know why, yet

because what you're grouping by is an ambiguous name. My guess is nid which is so very present in many tables.

Subscribing

Subscribing - anyway, the patch in #9 solves my particular problem, which has been described above already.

subscribe

Patch in #9 works for me.

Patch #9 works for me too. Can this issue marked as fixed and committed?

Patch #9 works nicely for me.

Without a 3.x patch this is not RTBC.

Is there any progress on a 3.x patch for this?

I wanted to confirm that I am having the same problem as reported in this issue before opening a new issue.

I am using 3.x Views. I have my fields setup but when I add the Search Terms filter, expose it I receive the follow error:

warning: pg_query() [function.pg-query]: Query failed: ERROR: column reference "nid" is ambiguous LINE 16: ...ld_image_cache_field_image_cache_data, node_type, nid, node_... ^ in /var/www/includes/database.pgsql.inc on line 139.
user warning: query: SELECT COUNT(*) FROM (SELECT DISTINCT SUM(search_index.score * search_total.count) AS score, node_data_field_image_cache.field_image_cache_fid AS node_data_field_image_cache_field_image_cache_fid, node_data_field_image_cache.field_image_cache_list AS node_data_field_image_cache_field_image_cache_list, node_data_field_image_cache.field_image_cache_data AS node_data_field_image_cache_field_image_cache_data, node.type AS node_type, node.nid AS nid, node.vid AS node_vid, node.title AS node_title, node_data_field_image_cache.field_intro_value AS node_data_field_image_cache_field_intro_value, node_data_field_image_cache.field_intro_format AS node_data_field_image_cache_field_intro_format FROM node node LEFT JOIN search_index search_index ON node.nid = search_index.sid LEFT JOIN search_total search_total ON search_index.word = search_total.word LEFT JOIN content_type_product_publication node_data_field_image_cache ON node.vid = node_data_field_image_cache.vid WHERE (node.type in ('product_publication')) AND (search_index.word = 'directory') AND (search_index.type = 'node') GROUP BY search_index.sid, node_data_field_image_cache_field_image_cache_fid, node_data_field_image_cache_field_image_cache_list, node_data_field_image_cache_field_image_cache_data, node_type, nid, node_vid, node_title, node_data_field_image_cache_field_intro_value, node_data_field_image_cache_field_intro_format HAVING COUNT(*) >= 1 ) count_alias in /var/www/sites/all/modules/contrib/views/plugins/views_plugin_pager.inc on line 134.
warning: pg_query() [function.pg-query]: Query failed: ERROR: column reference "nid" is ambiguous LINE 16: ...ld_image_cache_field_image_cache_data, node_type, nid, node_... ^ in /var/www/includes/database.pgsql.inc on line 139.
user warning: query: SELECT DISTINCT SUM(search_index.score * search_total.count) AS score, node_data_field_image_cache.field_image_cache_fid AS node_data_field_image_cache_field_image_cache_fid, node_data_field_image_cache.field_image_cache_list AS node_data_field_image_cache_field_image_cache_list, node_data_field_image_cache.field_image_cache_data AS node_data_field_image_cache_field_image_cache_data, node.type AS node_type, node.nid AS nid, node.vid AS node_vid, node.title AS node_title, node_data_field_image_cache.field_intro_value AS node_data_field_image_cache_field_intro_value, node_data_field_image_cache.field_intro_format AS node_data_field_image_cache_field_intro_format FROM node node LEFT JOIN search_index search_index ON node.nid = search_index.sid LEFT JOIN search_total search_total ON search_index.word = search_total.word LEFT JOIN content_type_product_publication node_data_field_image_cache ON node.vid = node_data_field_image_cache.vid WHERE (node.type in ('product_publication')) AND (search_index.word = 'directory') AND (search_index.type = 'node') GROUP BY search_index.sid, node_data_field_image_cache_field_image_cache_fid, node_data_field_image_cache_field_image_cache_list, node_data_field_image_cache_field_image_cache_data, node_type, nid, node_vid, node_title, node_data_field_image_cache_field_intro_value, node_data_field_image_cache_field_intro_format HAVING COUNT(*) >= 1 LIMIT 10 OFFSET 0 in /var/www/sites/all/modules/contrib/views/plugins/views_plugin_query_default.inc on line 1093.

Still having this issue in 6.x-3.x-dev

I hope the changes to the project options are correct. I changed this to version 3.x because the patch in #9 seems acceptable for those using 2.x. I consider this a major issue considering it halts the query from working at all.

I've tried altering the query and it does fix the error (in a very, very hacked sort of way) but it doesn't pass the nid value to the results so all titles are linked to "/node/" without the nid, etc.

$query->count_field['alias'] = 'node_nid';
$query->fields = array(
'node_title' => array("field" => "title", "table" => "node", "alias" => "node_title"),
 'node_nid' => array("field" => "nid", "table" => "node", "alias" => "node_nid"));

EDIT

I was able to get the nid working for the results links by doing this

$view->display['YOUR_DISPLAY']->handler->handlers['field']['title']->aliases['nid'] = 'node_nid';

I'm new to Drupal, and full of good intentions, I wanted it run on top of Postgres. Next, I decided to install Open Atrium, but this and related issues stopped it from running without errors. So I hacked on it for several hours and by the end of the day, I managed to get Open Atrium running without any warnings.

It is not perfect nor complete, but at least, it appears to be good enough for a start.

So here is the patch to query.inc (version 1.50.2.4 2009/12/02).

The issues:

1. I think this patch is (currently) only needed for Postgres, so I test if the DB is indeed Postgres, and if it isn't, just revert to the old behavior.
2. No longer use aliases, but use the original column names instead:

   That is, in principle, a matter of eliminating use of $field['alias'] in the code on line 947. However, that is not enough to solve every problem.

3. Error message: column reference "nid" is ambiguous

   I like the idea of using the original column names as the alias for the columns from the base table. Because, if you are going to use fieldnames with the table name as a prefix for the base table too, as has been proposed, then to be consistent, you also have to do it if your query uses only one table. And that would seem to be rather silly, at least to me.

   So what I decided on is to simply qualify (= prepend name of base table plus dot) the field names that are not:

   • already qualified names: names containing a "."
   • column name aliases: names most likely containing "_" (though apparently some column names for some tables also contain an underscore)
   • special literals, most notable "NULL", though probably that could be numbers or string literals too
   • something that would seem to be a magical placeholder (or template variable): names formatted like "***name***", for example "***CURRENT_USER***" or "***ATRIUM_ACTIVITY_TIMESTAMP***"

   That's a lot of exceptions (although those latter cases would probably never end up in the "group by" list), so instead of looking at the format of the field, I decided instead to look up if the name is actually a column name for that particular base table, thus: look if the string as is, is in the list of column names. That simply eliminates every exception from the above list, and quite efficiently too.

   Though one could actually retrieve the column names from the DB at runtime, it would be extremely inefficient (and silly) to do it for every single query, perhaps thousands of times an hour, because that list of column names will not just change overnight.

   So the wiser thing to do was to fetch the list of column names for the problematic tables and stuff it into a PHP array, in source code. I used an associative array where the keys are the table names and the values are the arrays with column names.

   For all other tables you ought to do the same, but as Open Atrium didn't require it, I decided against doing it for now.

   I just took a few column names that would seem to be fairly common, for any other tables.

   I think the proper solution would be to fetch the column names for all tables once, at installation time, and generate a PHP source file from them, and load that file from there on.

   That way, the array of column names could be gotten without any conditionals.

   A future enhancement could be that this list could be returned as a method from the query object.

4. $this->groupby

   The user supplied fields in $non_aggregates are not the only problem. The same issue arises for the list of field names supplied through $this->groupby. So I apply the same treatment to that list, using a temporary array in the variable $groupby.

   BTW I know that some people might object to my reuse of the same variable as a temporary array, first, and then as the final string later. But I first had used a different variable name at first and the code didn't work, because that variable apparently was already used elsewhere. $groupby is safe, simply because it gets set to a new value a few lines down.

   I guess it it ought to be possible to already qualify the names before the method groupby returns them.

5. Error message: could not determine anyarray/anyelement type because input has type "unknown"

   That problem is caused by use of FIRST(NULL), where plain and simple NULL will do — and do better.

   So I test if $string is "NULL", and if it is, not make it "FIRST($string)".

   Probably other literals (including the magic placeholders) could handle the same treatment, however, Postgres doesn't seem to object for those.

6. Error message: UNION types integer and text cannot be matched

   That problem only seems to arise in UNION queries, where NULL is used in one branch as a column value, and the same column is a number in other branches. It would seem Postgress automatically treats a plain NULL as a string, and merging it with a column of numbers poses a problem. NULL ought to be treated as a number.

   The solution is to typecast the NULL to a number (or more specifically, an integer), for the fields of which I assume to represent numbers, using "NULL::integer", or the more SQL-Standard compliant "CAST(NULL AS integer)" instead of plain "NULL". My rule for that guess is that if an alias name ends with "id", then it's a number. It seems to be good enough for Open Atrium.

I think that about wraps it up.

Well, there's still the same problem with NULL as in issue#6 in my previous post ("UNION types integer and text cannot be matched") for queries that do not use "group by", so I moved the code around a little and now always check if $string is "NULL".

That solves my immediate problem.

This patch replaces the patch from yesterday, and thus, is against the original file.

+    if($GLOBALS['db_type'] == 'pgsql') {
+      # common tables: list of column names
+      $_columns = array(
+        'node' => array('nid', 'vid', 'type', 'language', 'title', 'uid', 'status', 'created', 'changed',
+          'comment', 'promote', 'moderate', 'sticky', 'tnid', 'translate'),
+        'comments' => array('cid', 'pid', 'nid', 'uid', 'subject', 'comment', 'hostname', 'timestamp',
+          'status', 'format', 'thread', 'name', 'mail', 'homepage'),
+        'user' => array('uid', 'name', 'pass', 'mail', 'mode', 'sort', 'threshold', 'theme', 'signature',
+          'signature_format', 'created', 'access', 'login', 'status', 'timezone', 'language',
+          'picture', 'init', 'data', 'timezone_name'));
+      $columns = $_columns[$this->base_table];
+      if(!$columns) {
+        # other tables: guess column names
+        $columns = array('nid', 'uid', 'cid', 'vid', 'pid', 'status');
+      }
+    } else {
+      # not Pg
+      $columns = array();
+    }

Isn't there a more abstract way of fixing it? Views builds query against any kind of table schema not just only node and users.

To quote myself:

I think the proper solution would be to fetch the column names for all tables once, at installation time, and generate a PHP source file from them, and load that file from there on.

By that I mean execute a query like this once, and generate a PHP source file from it -- most definitely not write it by hand:

SELECT relname as table_name, attname as column_name
FROM pg_catalog.pg_class c
INNER JOIN pg_catalog.pg_namespace n ON c.relnamespace = n.oid
INNER JOIN pg_catalog.pg_attribute a ON a.attrelid = c.oid
WHERE a.attnum > 0
  and a.attisdropped = false
  and relkind = 'r'
  and relowner = (select oid from pg_roles where rolname=user)
order by 1, 2, attnum;

There is no reliable way to determine if a field name is a actually a column name, based on the format alone. Checking it against the list of column names of a table is as reliable as it gets, and fast.

There's only one problem: it's also big. There are currently 95 tables and a total of 577 column names in the database. That could be a rather large file. Although... not that large, to current standards...

Still, most of those tables are never used in views so it's a bit unnecessary.

It is possible to easily get the names of the columns from a single table, straight from the db, but I'm still not a fan of doing this every single time. The following code works for me (you'll have to change the db parameters) independently from Drupal:

$dbconn = pg_connect("host=localhost user=drupal");
$dbresult = pg_query($dbconn, "select * from node where 1 = 0");
$num = pg_num_fields($dbresult);
$columns = array();
for($i = 0; $i < $num; $i++) {
    $columns[] = pg_field_name($dbresult, $i);
}
pg_close($dbconn);

echo '<plaintext>';
var_export($columns);

which produces

array (
  0 => 'nid',
  1 => 'vid',
  2 => 'type',
  3 => 'language',
  4 => 'title',
  5 => 'uid',
  6 => 'status',
  7 => 'created',
  8 => 'changed',
  9 => 'comment',
  10 => 'promote',
  11 => 'moderate',
  12 => 'sticky',
  13 => 'tnid',
  14 => 'translate',
)

Note that the query itself returns no rows, but that doesn't matter, you can still fetch the column names.

You could fetch this once the first time a base table is used when a view is run, and cache it (the text is valid PHP code), be it in one common file for all tables, or one tiny file per table.

And when using Drupal's database access functions, you can use its built in features to replace the table name with the actual table name, in case people use a table name prefix: "select * from {node} where 1 = 0"

subscribe

Subscribing.
I'm using D7 (Views 7.x-3.0-beta3) but have the same issue--will leave version of issue as-is since i'm late to the party.
I'm using postgresql. If I add search term filter, it adds a group-by clause and I get the error:
'Exception: SQLSTATE[42702]: Ambiguous column: 7 ERROR: column reference "nid" is ambiguous at character 1200' in views_plugin_query_default->execute()

Rerolled #9 to get rid of offsets; it's still against 6.x-2.12.

Patch #32 works with 6.x-2.14

I'm not really sure whether this is RTBC as it's a) not tested by really a lot of people and b) whether it should be added to 6.x-2.x that late.

Same issue still applies to 7.x-3.x.

Hi. I would like to confirm that Patch #32 works for me too, also with version 6.x-2.16. Thank you very much for the fix!

I have to re-apply the patch everytime I update my version of Views 6.x-2.x. Any chance that it could still be added to 6.x-2.x in a future release?

For what it is worth, I manually made the changes in patch #32 to views 7.x-3.3 (changes went into file plugins/views_plugin_query_default.inc) , and it solved the problem for me. Hoping this gets added in.

...of course, I had to manually add new group_by clauses in order to make my query actually work, but that's another issue.

I have had the same problem. Here a patch.

#38 works for me, thanks. could this please be put into future releases?

Hm, since this is a problem in 7.x-3.x, should we fix it in 8.x-3.x first? I don't know.

But I'm going to call back to merlinofchaos's original plan back in #11, where the base table's primary field needs a better alias. I believe that is all that is necessary to fix the original issue. However, merlin wanted to get rid of extraneous references to ->nid and ->cid, etc., as well. Does anyone know if that has been done by now?

I tried changing the base field's alias, but it seems there is a lot of 3rd-party code that relies on it too much. I don't think anyone wants to mess with that in version 3.

So here's a different approach. The real problem is just the base field having an ambiguous alias in the GROUP BY clause. These other patches try to change how the alias is built, but I propose just changing the way the base field is added to the GROUP BY clause of the query itself.

The patch #42 worked fine for me.

The approach seems sensible, and the code is quite straightforward, so I move it to RTBC.

#42 worked for me too ...

Oh! Testbot is adding 'sites/default/modules/views/tests/templates/views-view--frontpage.tpl.php' to the end of the list of test files. I bet the double-dash there is messing up the argument parser. Is there an issue about this for the testbot? Why is it doing that?

"Scanning sites/default/files/checkout/sites/default/modules/views for /Tests/*.php files.", but it's not doing it in a case-sensitive manner. Guess I'll track down some Testbot admins.

I found the issue queue instead. #2415991: Scan for D8 tests breaks Views tests in D7, or fix path case-sensitivity and argument parsing