The current code for drupal 5.18 (encountered the bug on) and 6.12 adds a ranking clause like so by default:

%d * POW(2, (GREATEST(n.created, n.changed, c.last_comment_timestamp) - %d) * 6.43e-8)

This is then used to calculate the final relevance of all nodes that were brought up in the search results. However, if there is no entry in node_comment_statistics for a given node, c.last_comment_timestamp will = NULL. In both the versions of mysql I tested with GREATEST(any number, NULL) will return NULL; subtraction with NULL returns NULL; multiplication with NULL returns NULL. Finally, the relevance is returned as NULL.

The attached patch is against drupal 5. Drupal 6 has MAX(c.last_comment_timestamp), and MAX(NULL) is also NULL. So, the patch to drupal 6 could simply be MAX(c.last_comment_timestamp) || 1 (or some such low number). Something simily might need to be applied in the 'status' switch in node_search in drupal 5, but I'll leave that up to someone who knows the code better.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

brianV’s picture

Version: 5.18 » 6.x-dev
Status: Needs review » Needs work

+1 subscribe.

Should be fixed in 5.x and 6.x. This is not an issue in D7 as last_comment_timestamp isn't used.

jhutton’s picture

Version: 6.x-dev » 6.12
Status: Needs work » Needs review
FileSize
729 bytes

Bumped to 6.12 since the bug also appears to be present there. It looks as if Drupal 7 does not contain the bug, however, since it does
'score' => 'POW(2.0, (GREATEST(n.created, n.changed) - %d) * 6.43e-8)'
leaving out last_comment_timestamp entirely.

Attached is a patch against drupal 6.12.

brianV’s picture

Version: 6.12 » 6.x-dev
Status: Needs review » Reviewed & tested by the community

Looks good.

Bumping back to 6,x-dev since that is where it would be fixed.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

I'd like to request a line of comment above this code. I see that since 1 will be lower then the other MAX-es, you get some real value, but that would be good to document. Otherwise it could easily get people puzzled on what the hell happens there.

brianV’s picture

Status: Needs work » Needs review
FileSize
1.6 KB

Updated, with a comment explaining things. The comment is sort of long, but as short as I could make it while adequately explaining the issue.

petercasier’s picture

Problem is still there in 6.16

patch should now be:

$ranking[] = '%d * POW(2, (GREATEST(MAX(n.created), MAX(n.changed), MAX(c.last_comment_timestamp) || 1) - %d) * 6.43e-8)';

EmanueleQuinto’s picture

Here attached patch for 6.16

petercasier’s picture

.. and still there in 6.17 too

EmanueleQuinto’s picture

... the patch works for 6.17 and 6.x-dev too (as for node.module,v 1.947.2.24 2010/06/02 12:11:07 goba), it only seems to need review.

Jim Kirkpatrick’s picture

Status: Needs review » Reviewed & tested by the community

Patch applies, search now works properly - thanks!

Marking as tested.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Great, thanks for the testing. Committed.

Damien Tournoud’s picture

Priority: Normal » Critical
Status: Fixed » Active

Sorry to reopen, but this is very broken:

  • Dealing with NULL values is precisely what the COALESCE() function is for
  • || is not a portable operator for OR, it's the ANSI operator for string concatenation
  • || 1 doesn't really make any sense :p

The search module is now broken on PostgreSQL. Yay, fun.

What you wanted here is: GREATEST(MAX(n.created), MAX(n.changed), COALESCE(MAX(c.last_comment_timestamp), 0)).

It would also probably makes more sense to move the MAX() out of the GREATEST(), but the result would be unaffected and it's another issue anyway.

Damien Tournoud’s picture

jhodgdon’s picture

Damien meant to say that #882194: PostgreSQL query error when searching was marked as a duplicate, I think.

Damien Tournoud’s picture

Thanks Jennifer, that somewhat makes more sense.

jpulles’s picture

Title: Search relavence calculation fails if last_comment_timestamp is NULL » Search relevance calculation fails if last_comment_timestamp is NULL

It seems solved by removing the || 1 part. A 'coalesce' function is not needed because the timestamp is NOT NULL; just the greatest value of 3 timestamps is needed.

GREATEST(MAX(n.created), MAX(n.changed), MAX(c.last_comment_timestamp))

Damien Tournoud’s picture

@jpulles: have you seen the title of the issue? :)

EmanueleQuinto’s picture

This patch should be at least SQL-92 conformant. It uses COALESCE now.

I don't have a PostgreSQL environment, apologize for this, then should be tested there...

jhodgdon’s picture

Status: Active » Reviewed & tested by the community
FileSize
1.26 KB

Just as a note: When supplying patches for Drupal, it's best to (if possible) roll them using cvs diff, and at a minimum, roll them from the Drupal root, rather than at the level of the particular file you are patching.

That said, I have set up a PostgreSQL environment, reproduced the SQL error that is happening now, then applied the patch above. The errors went away, and my searching tests (on contrib module Porter Stemmer) now pass.

So here is the same patch, rerolled from Drupal head.

jpulles’s picture

@Damien: Yes, I read the title, but thought that the last_comment_timestamp could never be null since it is defined as 'NOT NULL' in the database schema.

jhodgdon’s picture

Many people are reporting this -- WE NEED TO FIX IT SOON!!!!
#888164: After update search has ceased to work
http://drupal.org/node/885026
etc.

lifepillar’s picture

Subscribe.

jhodgdon’s picture

Anyone having this problem using Postgres and Drupal 6.19 can apply the patch in #19 and likely fix the problem (if someone besides me could verify, that might help).

If you are not familiar with how to apply patches, take a look at the attached patch file. Basically the lines starting with - need to be removed and replaced with the lines starting with +. You can do this by hand in a text editor, since this patch is so small. Be sure to remove the + sign at the beginning of the two lines you are adding!!

kecsi’s picture

Yes. I had same issue here using 6.19, Postgres. Fixed manually. Thanks.

mcm.drupal’s picture

I'm not familiar with the testing process here... and am confused by:

  • article status says, "reviewed & tested by the community", but
  • attachment status is "Test request sent", while Test Result is "None".

I don't know how to contribute my two cents worth, which is that the patch fixed my problem on 6.19 with PG 8.3.7.

gionn’s picture

#19 Works for me. (Debian Lenny apache2+postgres)

andrea.vivaldi’s picture

subscribe

gmh04’s picture

#19 works for me postgres 8.4.4

PMunn’s picture

Works for me on PostgreSQL 8.4.4! Woohoo! Thank you jhodgdon for making this patch!

bendiy’s picture

OMG! patching now...

jorbot’s picture

Patch worked for me. THANK YOU!

christoph’s picture

For those who don't want to patch their production site I found the following worked as a temporary measure.
Go to Admin>Settings>Search and set the Factor 'Recently Posted' to 0. (Will have to remember to set back again on the next release).

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 488166-fix-reroll.patch, failed testing.

marlatt’s picture

Thanks christoph for this note, as no one has solved this problem yet and some of us with postgres don't want to be in the wild with core security vulnerabilities.

jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community

D6 patches are failing with the same error. There's no problem with this patch.

marlatt’s picture

Status: Needs work » Reviewed & tested by the community

I wasn't aware of that, thank you.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 488166-fix-reroll.patch, failed testing.

jhodgdon’s picture

re-testing is not going to help.
#961172: All D6 Core patches are failing

diaxpro’s picture

This is my error:

user warning: Operand should contain 1 column(s) query: SELECT COUNT(*) FROM (SELECT i.type, i.sid, 5 * (3,6765835972501 * SUM(i.score * t.count)) + 5 * POW(2, (GREATEST(MAX(n.created), MAX(n.changed), COALESCE(MAX(c.last_comment_timestamp), 0)) - 1288716585) * 6.43e-8) + 5 * (2.0 - 2.0 / (1.0 + MAX(c.comment_count) * 0,14285714285714)) + 5 * (2.0 - 2.0 / (1.0 + MAX(nc.totalcount) * 0,0050251256281407)) AS score FROM search_index i INNER JOIN search_total t ON i.word = t.word INNER JOIN node n ON n.nid = i.sid INNER JOIN active_translation ON n.nid = active_translation.es LEFT JOIN node_comment_statistics c ON c.nid = i.sid LEFT JOIN node_counter nc ON nc.nid = i.sid WHERE n.status = 1 AND (i.word = 'parroquia') AND i.type = 'node' GROUP BY i.type, i.sid HAVING COUNT(*) >= 1) n1 in ../search/search.module on line 958.

My partner show that in sql change the "," for "." work i find the solution...

jhodgdon’s picture

diaxpro: is this the same problem as has been reported above, and does the patch in #19 above fix it?

If it is a separate problem, please file a new issue. Thanks.

diaxpro’s picture

I'm sorry but #19 don't work for me and the solution that i find is in http://drupal.org/node/389452 on coment #2
Thx.

jhodgdon’s picture

Your problem is different from this error report. Your problem is that SQL doesn't support commas as decimal points, but for some reason, your system is creating queries with commas as decimal points. The solution you found is telling PHP not to do that. So it's unrelated to this issue.

quiptime’s picture

Patch #19 works on Ubuntu 8, PG 8.3.12 and D 6.19.

I'm very surprised. Many users report that the patch #19 works.

Why is not taken over the patch?

vevhlos’s picture

Thanks @#32! brilliant thought!

jhodgdon’s picture

#19: 488166-fix-reroll.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 488166-fix-reroll.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community

I had thought that #961172: All D6 Core patches are failing was fixed, and so I asked for a retest of this patch, but it's not fixed.

dsarch’s picture

Hi,

Please release a new version with this patch (#45) as it totally breaks things in postgresql here. I did exactly this fix (coalesce) and it works here.

Is it possible to generate another release at least with this correction ? This is a critical bug and it is fixed, so there is not holding it.

Thanks for the work.

morthylla’s picture

#32 Thank you! A simple and elegant solution. I only have to remember to modify it later ^_^

atavio’s picture

#19 & #32 worked for me.

Drupal 6.19
Postgres 8.4

Thanks! ;)

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, committed.

Status: Fixed » Closed (fixed)

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

pvpaneque’s picture

thanks my friend for that configuration , i was looking for the solution of this problem a long time ago and this setting works perfectly , thanks so much