WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 months ago

Last modified 5 months ago

#22196 closed enhancement (maybelater)

add $wpdb->upsert function

Reported by: thomask Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.4.2
Component: Database Keywords: has-patch needs-testing needs-refresh needs-unit-tests
Focuses: Cc:

Description

If i read http://codex.wordpress.org/Class_Reference/wpdb right, we have only insert and update functions, but for many occations, if would be lovely, if we would have upsert function as well (using INSERT ... ON DUPLICATE KEY UPDATE http://dev.mysql.com/doc/refman/5.1/en/insert-on-duplicate.html).

So when i am inserting e.g. posts using database function, i do not have to check if post with that id exist (good for some migration / importing tools etc.)

Attachments (1)

upsert.diff (3.9 KB) - added by donpark 5 months ago.
UPSERT Patch

Download all attachments as: .zip

Change History (12)

#1 @wonderboymusic
5 years ago

This one would be tricky:

Inserts and Updates also take formatting as an argument. If you pass the primary key as an arbitrary value, the primary key and its formatting would have to be removed from the values AND formatting arrays, regardless of what order they were passed.

The resulting SQL needs to look like:

INSERT INTO wp_posts (post_title, post_date, ID, post_content, post_type) 
VALUES ('Title', '2012-10-07 07:00:00', 3, 'This is content', 'event') 
ON DUPLICATE KEY UPDATE 
post_title = 'Title',
post_date ='2012-10-07 07:00:00',  
post_content = 'This is content', 
post_type = 'event'

But that can be passed to $wpdb->insert like so:

$wpdb->insert( 
	'wp_posts' 
	array( 
		'post_title' 	=> 'Title', 
		'post_date' 	=> '2012-10-07 07:00:00', 
		'ID' 		=> 3, 
		'post_content' 	=> 'This is content', 
		'post_type' 	=> 'event' 
	),
	array( '%s', '%s', '%d', '%s', '%s' )
);

The insert method takes arbitrary arrays as arguments, but to remove the *specific* primary key for any passed table, it would have to be intercepted and altered just in time to create the proper update SQL.

I'm not sure wpdb should get this fancy. Semi-related: #19019

Last edited 5 years ago by SergeyBiryukov (previous) (diff)

#2 @wonderboymusic
4 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to maybelater
  • Status changed from new to closed

I'm the only comment, and that was 20 months ago.

#3 @donpark
5 months ago

  • Keywords has-patch needs-testing added
  • Resolution maybelater deleted
  • Status changed from closed to reopened

Proposed patch adds new method upsert to wp-db.php and modifies private method _insert_replace_helper, adding a new $type value UPSERT.

public function upsert( $table, $data, $format = null, $keys = null )

Optional parameter $keys is used to filter out keys from ON DUPLICATE KEY UPDATE part of the query like this:

$wpdb->upsert( 'table', array('id' => 1234, 'column' => 'foo', 'field' => 1337 ),
    array('%d', '%s', '%d' ), array( 'id' ) );

@donpark
5 months ago

UPSERT Patch

#4 follow-up: @pento
5 months ago

  • Keywords reporter-feedback needs-refresh needs-unit-tests added
  • Milestone set to Future Release

Thanks for the patch, @donpark!

MySQL's INSERT ... ON DUPLICATE KEY UPDATE is... kind of weird, particularly on tables with multiple unique columns. If the INSERT clashes with multiple rows, only the first row encountered will be updated. I think the behaviour of which row will be updated is undefined. It's useful in certain circumstances, but can be horrifyingly hard to debug when used incorrectly. I'm not convinced that it's wise to be encouraging the use of upserts with a CRUD function, though I'm interested in your thoughts on the matter.

For the patch itself. I think that, instead of $keys, we'd want to pass $update_columns - the array of columns to be used in the UPDATE part of the query. This matches the behaviour of the actual query (you specify the columns to update, not the columns from the INSERT part to not update), and it's probably easier to maintain against table schema updates - adding a new unique index to a table would involve updating every upsert() call for that query.

I don't think there'd be any reason to change the mysqli_affected_rows() call to recognise upserts. It already has well defined behaviour for when the INSERT vs. the UPDATE part of the query was run.

There are also a couple of places in Core where we use upserts, updating them so we're using the functionality in Core would be good.

Also, this will need many unit tests. :-)

#5 in reply to: ↑ 4 @donpark
5 months ago

  • Resolution set to invalid
  • Status changed from reopened to closed

Replying to pento:

MySQL's INSERT ... ON DUPLICATE KEY UPDATE is... kind of weird, ...
though I'm interested in your thoughts on the matter.

I wasn't aware of MySQL's UPSERT issues. I'll need do some research before I can share anything.

Also, this will need many unit tests. :-)

I agree. :-)

#6 @donpark
5 months ago

  • Keywords reporter-feedback removed
  • Resolution invalid deleted
  • Status changed from closed to reopened

Urgh. Trac UX leaves a lot to be desired.

Undoing unintended change to Resolution and Status.

#7 @donpark
5 months ago

  • Resolution set to maybelater
  • Status changed from reopened to closed

Details on UPSERT problem @pento mentioned can be found here:
https://stackoverflow.com/questions/16377932/mysql-behavior-of-on-duplicate-key-update-for-multiple-unique-fields
Duplicate keys are matched with OR, not AND, which makes no sense to me.
Undefined behavior stems from undefined order of duplicate key matching rows.

In short, UPSERT on MySQL needs to be used with care. Frankly I think this may be too much to ask for from typical WP hackers, even with flashing warning message.

Changing status to maybelater again to be reconsidered when and if demand for upsert warrants reconsideration.

Thx for the quick response @pento.

Last edited 5 months ago by donpark (previous) (diff)

#8 @thomask
5 months ago

I am not an expert in this area, but wouldn't be a solution to that problem be allowing upsert for single ID? This is anyway the most typical scenario. But i am fine if it would be maybelater and wait for some mysql change, as i have reported this 5 years ago and reaction was minimal

#9 @donpark
5 months ago

@pento, would @thomask's one-key-upsert help w/upsert needs in the core you mentioned?

It doesn't help me but I can assist if the need is there.

#10 @pento
5 months ago

I think we're stuck between a rock and a hard place, here.

On the one hand, MySQL's upsert behaviour is unpredictable under some circumstances, so making it easier to use seems unwise.

On the other hand, having WPDB's upsert behave differently to MySQL seems like a recipe for people running into edge cases that they didn't expect. There'd also need to be a tradeoff between doing a complete set of pre-flight checks on upsert queries (for example, running a SELECT query first, based on any columns being updated that are part of unique keys) while also maintaining compatibility with non-transactional DB engines, like MyISAM.

I don't really have a good answer here, unfortunately. I think the best method for now is to require upsert queries to be written manually - people who are aware of the query can use it, but we're not encouraging wider use of something that is often not entirely understood.

#11 @netweb
5 months ago

  • Milestone Future Release deleted
Note: See TracTickets for help on using tickets.