Modern Vespa

Hi Jess. I thought that your system believed I was trying SQL injection because my search term used "replace" but I changed that to "change" as in "change brake pad" and still got an error like this one.

Sorry about that. I will investigate.

Were there any other parameters you used? Posts vs Threads? I tried with the same search string and it seemed to work, but I could very well be missing something.

Nope. I just put in "replace brake pad" as the search term and hit enter. Got the error and changed "replace brake pad" to "change brake pad" and tried again. Maybe it has resolved itself.

EDIT: Yup. Seems to be working now. I have no idea what happened.

My suspicion is that there was a record stuck in the search results table. I'm not totally certain why -- the search system is kind of a beast and I admit that I don't fully understand it in its entirety. If it happens again, let me know. And again, I appreciate the problem report.

That sounds about right. I'll let you know if I see it again.

The mystery deepens: I went and looked at the code that threw this error. The duplicate key that it's complaining about is generated -- immediately before the SQL operation -- by generating a random number. And it does appear to be properly seeded.

So I am more puzzled than ever as to how that could have happened.

It's not generating a GUID? That would be far less likely to ever generate a dupe than a random number generator.

Oh, agreed completely. But this code dates to 2002, so...

Might need to make some mods here.

I'm surprised it's not using an auto incrementing primary key field if it wants to use numbers for the primary key. No chance of duplication then.

Is there any way to change it to auto incrementing and remove the primary key from the INSERT command?

The original software was designed to run on a variety of database types, with a translation layer in between. And a number of those DB systems weren't very well developed at the time, so it looks like there was a lot of coding to the lowest common denominator. Auto-increment wasn't used much, if at all.

These days, of course it makes sense to use those capabilities, and they are widely available with virtually every database.

Okay, digging a little deeper: it appears the strategy goes something like this:

1. Generate pseudorandom number, seeded with microtime()
2. If there's already a set of search results associated with the session_id (i.e. that user's session cookie) then update the database with the new search results and set the search_id to the new random number
3. If there ISN'T a set of search results associated with the session, insert using the search_id field (which must be unique)

So that explains why auto-increment isn't used. While search_id must be unique, it is secondary to the session_id.

My question is: why have search_id at all? Probably because session_id historically wasn't very reliable at the dawn of web apps. But still. This seems like a bad strategy.

Got it. It's not a big problem, anyway. It might not happen again for years.

Yeah, but still. This feels off to me -- I think I'm still missing some piece of the puzzle.

I don't think you're missing anything. If so wouldn't it have shown up a long time ago or happen on a regular basis? Seems to me the random number generator simply generated a couple of numbers that had already been used.

That's what feels off. The search results are pruned automatically. At any given moment, there are only maybe 10 - 20 sets of search results in the database. With a sufficiently large random number, the chances of a collision are exceedingly low -- so low as to be highly unlikely. And you got it twice in a row.

That feels like a hole in the logic.

So you're saying the search results are held in a temp table of some sort? I thought they were INSERTed into a permanent table that has been holding all the search results since inception of the site.

OK, then, yeah. There's something off with the logic but given that it can't easily be duplicated is it worth taking a few years off your life trying to track it down? I wouldn't bother and would just chalk it up to one of those things that happens once every ten years or so.

This might have something to do with it. This is the test to see if the update worked: The first clause is the standard form used throughout this forum software: execute the SQL command and check the result.

The second clause is much less common -- and I'm not sure why the original author felt it necessary to second-guess the first clause. It seems to me, since this is structured as an OR, that if sql_affectedrows() returns 0 (for whatever reason), then the overall if statement would be true and the code it encloses (which is the INSERT statement, btw) would execute.

That's my theory.

Nope. Just a handful of them at any given moment. Their lifespan is defined by the session time, which I think is 30 minutes.

What comes after the if? That codes looks to me like if SQL command fails (in this case INSERT) OR affectedrows is false THEN (do something). I'm wondering what the do something is. Throw an error?

Here's the whole sequence: I'm thinking that the first clause (the sql_query() that performs the actual UPDATE SQL statement) succeeds, but (for reasons still unknown) sql_affectedrows() returns 0, causing the INSERT to be executed with the same key.

That message_die() function call is what produced the error message you saw.

Agreed. Seems odd though that any database engine would return an erroneous result on affected rows.

Agreed. Which is the only reason I haven't already refactored this code this morning. I am puzzled by how the conditions might actually present themselves.

Well, it does seem like it's returning a wrong result, either from the update or the rows affected. Weird.