Line 77 of pathauto_node.inc uses the MySQL CONCAT function. This is not supported in PostgreSQL, which does support the a || b || ... ANSI syntax. (Note that, from what I found, MySQL has to be run in "ANSI" or "Strict" mode for "||" to be properly interpretted, but that's what is recommended for Drupal development anyway.)

CommentFileSizeAuthor
#6 pathauto_pgsql_compliance_concat_cast_257281_6.patch1.57 KBgreggles
#5 pathauto_pgsql_compliance_concat_cast_257281_5.patch0 bytesgreggles
#2 PathAuto.Concat.tar_.gz1.53 KBbj.drupal

Comments

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

When testing this under pgsql I asked for help and got some. I believe that the current version works under modern versions of pgsql. If not, patches to fix the problem are always welcome.

bj.drupal’s picture

bj.drupal commented
StatusFileSize
new PathAuto.Concat.tar_.gz1.53 KB

First time I've ever reported a bug, so not quite sure what you'd like. I changed line 77 of the indicated file to use ANSI "||" concatenation operator and problem went away. Haven't analyzed the code to understand what the query does exactly, so haven't figured out what specific testing I should do to assert its correctness. The changed file is attached so you can "diff" it or whatever.

Note also that, from what I read, this will only work in MySQL if it is running in "ANSI" mode. Otherwise MySQL interprets the "||" as an "OR" operator I gather (like PHP).

I've only just started playing with Drupal and PostgreSQL, but I'd be happy to help out with some testing where I can.

Hope this helps - B.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Active » Needs work

Well, when you provide code please do so as a http://drupal.org/patch.

If this only works when MySQL is in ANSI or Strict mode then it won't work for greater than 95% of Pathauto users. So...that won't work.

According to http://drupal.org/node/212327#comment-749830 this should work fine on pgsql 8.1+

Which version are you using?

bj.drupal’s picture

bj.drupal commented
Title: CONCAT Use Not Valid in ANSI SQL/Postgresql » Quick Update: Drupal's pgsql concat() only defined for 2 text parameters ...

... and you are passing a text and a numeric. Still investigating - as I said, I'm new to PostgreSQL and MySQL - but one option is to declare a "concat" overload for your combination. Looks like "num" in ANSI concatenation of txt || num will coerce the numeric to string. Alternative is a coerce of the numeric as part of the call, but don't know how to do that. (Yet?)

Trickiest part is finding something that works in both data base choices, of course.

Incidentally, after seeing the Drupal coding conventions recommending use of MySQL in "ANSI" mode, I naively assumed that that's what people used. I can understand it not being so easy to do in practice given the historic popularity of MySQL.

Thanks, also, for the pointers to providing a proper patch form.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Needs work » Needs review
StatusFileSize
new pathauto_pgsql_compliance_concat_cast_257281_5.patch0 bytes

Here's an alternate solution. From the work in #208860: Add per-language patterns for multilingual node types we lost the CONCAT('node/', CAST(nid AS CHAR)) which is why this stopped working for Postgres (it also has a bad performance consequence :(

So, in the attached patch I added that back in. Does this look good to you?

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
StatusFileSize
new pathauto_pgsql_compliance_concat_cast_257281_6.patch1.57 KB

Sorry about that. It was the old blank patch file trick...

Here's a slightly better version ;)

bj.drupal’s picture

bj.drupal commented

Sorry, been busy, hopefully try out the patch tomorrow. But if the CAST is a performance problem, perhaps a better solution would be to add a PostgreSQL concat() overload for a string and a number?

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

Sorry I wasn't more clear - the CAST fixes the performance problem because it allows the database to use the indexes on the url_alias table.

So, assuming that this works on pgsql (it should) we should be all set. Thanks for your help reviewing/testing this.

bj.drupal’s picture

bj.drupal commented

Patch looks very nice! Ran the following sequence:

- Ran [Automated alias settings] selecting "Bulk generate ..." on all of Blog, Node, Taxonomy, User, and Forum to ensure I still had the SQL error problem.

- Backed up the [pathauto_node.inc].

- Applied the patch per instructions to which you pointed me, and noted it said it patched the expected file.

- Diff'ed the backup and the revised file to ensure I got the changes shown in your patch file.

- Reran the [Automated alias settings] option as specified in the first step and: no SQL errors!

Thank you very much for your help.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Title: Quick Update: Drupal's pgsql concat() only defined for 2 text parameters ... » Use cast inside concat for better performance pgsql compliance
Status: Needs review » Fixed

Awesome! Thanks for the review. I have committed this to the 6.x branch.

If you haven't already I encourage you to join the Postgresql group at http://groups.drupal.org/postgresql There is a list of issues which need a review, and if you can keep providing reviews like this you will soon be famous ;)

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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