Please, excuse me if I missed this in the new DB Layer code. I have just browsed the code from CVS, but it looks to me that a particular feature of MySQL ODKU clausule is not used. ie. field = VALUES(field)

INSERT INTO table (pkey, field_a, field_b) VALUES (1, 2, 3)
  ON DUPLICATE KEY UPDATE
    field_a = VALUES(field_a),
    field_b = VALUES(field_b);

With this syntax it is not necessary to duplicate INSERT values for the ODKU clausule. This may be specially important when big TEXT/BLOB field literals/expressions are invoved, such as db_merge() usage in cache_set().

It has impact on the size of the SQL statemente itself, max_allowed_packet, ...

CommentFileSizeAuthor
#8 query_odku.patch1.84 KBCrell
#6 query_odku.patch1.6 KBmarkus_petrux
#4 query_odku_values.patch2.42 KBmarkus_petrux
#3 query_odku_values.patch1.73 KBmarkus_petrux
#2 query_odku_values.patch1.1 KBmarkus_petrux

Comments

Crell’s picture

Crell commented

Hm, interesting. I wasn't aware of that syntax. So you're saying that cache_set, for instance, would become:

INSERT INTO cache (cid, data, expire, created, headers) VALUES ('foo', 'huge string', 0, 12345, NULL) ON DUPLICATE KEY UPDATE cid=values(cid), data=values(data), expire=values(expire), headers=values(headers);

That does seem like a nice optimization. We can probably use that for the default case (just fields()) and for updateExcept(), but not for update(). Can you roll a patch so we can see if it works?

markus_petrux’s picture

markus_petrux commented
Status: Active » Needs review
StatusFileSize
new query_odku_values.patch1.1 KB

Not exactly. Note that it is not needed to update the key. ie. this is not needed: cid=values(cid)

So the statement would look something like this:

INSERT INTO cache (cid, data, expire, created, headers) VALUES ('foo', 'huge string', 0, 12345, NULL) ON DUPLICATE KEY UPDATE data=values(data), expire=values(expire), headers=values(headers);

Rolling a patch created by looking at the code. It is not tested, but I think it may work, or at least you can get the idea. :)

markus_petrux’s picture

markus_petrux commented
StatusFileSize
new query_odku_values.patch1.73 KB

Rerolled. $update_fields is not really needed in execute() method.

markus_petrux’s picture

markus_petrux commented
StatusFileSize
new query_odku_values.patch2.42 KB

I'm very sorry I was not enterily familiar with all merge related methods and I figured I missed the fact that the caller can explicitly specify which fields need to be updated.

Patch re-rolled.

Crell’s picture

Crell commented
Status: Needs review » Needs work

#4 totally doesn't apply at all. :-( I suspect it's because of the whitespace changes that were fixed a week ago. Can you reroll please?

On visual inspection it looks reasonable (and small, which I like), although the comments need a spellcheck and some language cleanup.

markus_petrux’s picture

markus_petrux commented
Status: Needs work » Needs review
StatusFileSize
new query_odku.patch1.6 KB

There you go. This time I believe I have used correct english and technical words. :-/

I wasn't sure whether to include a link to the MySQL manual for this particular syntax. Here's the function VALUES(col_name), if that helps:

http://dev.mysql.com/doc/refman/5.0/en/miscellaneous-functions.html#func...

dries’s picture

dries commented

Looks like a nice optimization, but I'd like to see Crell approve it first.

Crell’s picture

Crell commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new query_odku.patch1.84 KB

Looks good to me, and all database tests pass. Sadly there are Windows linebreaks in #6. :-( Attached patch is identical but removes them. Please don't credit me.

This should really help with things like cache sets.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks! Welcome back, markus_petrux. :)

Anonymous’s picture

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

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