DB colum type fixes (make PostgreSQL work)

Thank you, makes sense. I'm a bit surprised that there haven't been any complaints about this...

Do you have a reference/link for #3?

a reference for #3:
http://www.ianywhere.com/developer/product_manuals/sqlanywhere/0902/en/h...

I don't really know 'the authorative document for SQL92'. I just mention the term to give things extra weight ;-) (and I don't bother explaining more, or splitting it out into a separate patch, since this is a 'standard' bug report all over Drupal contrib.)

For more, google "sql92 group by" or "site:drupal.org postgresql group by".

For my own reference: http://www.db-staff.com/index.php/microsoft-sql-server/86-sql-group-by

Consequently, the INSERT statement in subscriptions_queue() that is generated for taxonomy subscriptions should also cause problems, because it includes SELECT ... GROUP BY tn.nid. Can you confirm or deny this?

Just remove the slash in front of

to see the generated statement.

Would it be better to use DISTINCT?

Yes, the insert statement generated for taxonomy subscriptions yields problems.

And well... I guess with your code it's somwewhat easier to use DISTINCT. But it won't help you as-is.

As a test, I subscribed to posts in term 3, and to posts in term 2 of a specific author. When I updated a post, this is part of what the query would return to me if I delete the GROUP BY:

In MySQL, your 'GROUP BY tn.nid' clause returns only one row, with 'quasi random' value & author_id fields. In PostgreSQL (and in MS-SQL & MS Access, afaik) this is illegal. You cannot implicitly say "oh, I don't care what values you exactly return for those fields, just throw one of the 2 rows away, whatever".
(And that is what you say when you group on only a subset of the fields. Hence the '#3' above.)
It is the same with DISTINCT queries -- I don't know MySQL enough to know whether you can make up some way to make that work. But in Postgres/MSSQL you will have to make those value & author_uid fields equal, before DISTINCT will ever merge those 2 rows into one.

Solution?
In subscriptions_taxonomy_subscriptions(), you can probably scrap the 'group_by' key of $params['node']['tid']. Instead, return 'ignore_fields' => array('value','author_id').
subscriptions_queue() will then need to:
* if you don't actually need those value & author_uid values: make those fields 0 instead of s.FIELDNAME in the SELECT part. Then you can make the query 'distinct'.
* if you do need one of the value & author_uid values" make those fields into Min(s.FIELDNAME) or Max(s.FIELDNAME) in the SELECT part, and group on all of the other fields in the SELECT list (which are all equal for all rows your query returns).

I haven't studied your mailing code so I don't know which one it is. In either case, sticking your INSERT/SELECT fields into an array and doing some array_filter() stuff with the 'ignore_fields' key in your return values, should get you far. (Or something like that.)

(Or... otherwise you will just have to insert 2 rows into subscriptions_queue.)

Thank you for your analysis and proposed solutions! Interestingly, this came up only a few days ago in #520230-1: Proposed Action plugin (e.g. for workflow).

This GROUP BY was an effort to reduce the number of records inserted into the {subscriptions_queue} table. However, there may still be multiple entries for the same node, and subscriptions_cron() needs to remove all of them after processing the first one. So, I guess we may just as well drop the GROUP BY and thereby improve the chances of delivering a list of all matching terms.

"oh, I don't care what values you exactly return for those fields, just throw one of the 2 rows away, whatever"

Yes, indeed, this is exactly what I meant when I introduced the GROUP BY as a partial response to #507898: Subscriptions_queue table containing duplicate entries two weeks ago. Maybe this used to work before SQL92, as it still does for MySQL...

Until #520230-1: Proposed Action plugin (e.g. for workflow) I didn't care about which tid (the value column) I got (but I do need one for !term_name!), but now it seems preferable to not GROUP at all and undo http://drupal.org/cvs?commit=232670. I'll look into this again.

"oh, I don't care what values you exactly return for those fields, just throw one of the 2 rows away, whatever"

Yes, indeed, this is exactly what I meant when I introduced the GROUP BY as a partial response to #507898

And you still can, in all database systems, kind of. But it needs to be explicitly specified, with First(FIELDNAME) or Last(FIELDNAME). To prevent unspotted mistakes, I guess.
(Which would also require the same code rewrite in this case. Yeah, as long as you have multiple rows in subscriptions_queue anyway, it does not seem worth the effort.)

(There's probably a Curch Of Good SQL that hates MySQL's guts for enabling sloppy aggregate functions, and joins across different types... and a Union Of Practical People that hates Postgres for being so strict. I'm not on either side really. I just notice that IF we want to reach portability, developing on the 'easiest' platform first costs extra time. But us Drupal devs will just have to live with that...)

Trying to turn SQL into a strongly-typed language is taking it a bit too far, IMO, but I agree that portability is a worthy goal.

I've reverted the GROUP BY as discussed in #5.

I've committed your patch and also fixed subscriptions_taxonomy_update_1() — however, it turned out that my MySQL doesn't like CAST(... AS VARCHAR), even though it accepted CAST(... AS CHAR) before, so I had to special-case for pgsql again. See http://bugs.mysql.com/bug.php?id=34564 — so far for portability...

Committed to 6.x-1.x-dev as well as 5.x-2.x-dev. Please give it a try once it has been repackaged.

@roderik: Have you had a chance to verify that this works with pgsql, now?

It works.

Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.