Make WordPress Core

Opened 8 years ago

Last modified 11 days ago

#44349 new defect (bug)

Posts show up multiple times in backend when they have the exact same date

Reported by: wzshop Owned by:
Priority: normal Milestone: Awaiting Review
Component: Query Version: 4.9.6
Severity: normal Keywords: reporter-feedback 2nd-opinion has-patch has-unit-tests
Cc: Focuses:

Description

When I import for example 200 posts with the Import WordPress tool and then go to wp-admin>posts, the posts show up in the query multiple times (for instance on page 1 and page 2). Seems like others are left out.

Attachments (1)

post export demo.xml​ (194.4 KB ) - added by wzshop 8 years ago.

Download all attachments as: .zip

Change History (36)

#1 follow-up: @pbiron
8 years ago

  • Component General → Import
  • Keywords reporter-feedback added

Welcome to trac!

Can you please let us know which version of the WordPress Importer plugin you are using?

#2 in reply to: ↑ 1 @wzshop
8 years ago

Replying to pbiron:

Welcome to trac!

Can you please let us know which version of the WordPress Importer plugin you are using?

Version 0.6.4

#3 follow-up: @pbiron
8 years ago

Thank you for the version # and for attaching a sample export to test with.

I'm not seeing the behavior you report when I import post export demo.xml into a fresh install (no plugins other than WordPress Importer v 0.6.4 and twentyseventeen theme).

Also, post export demo.xml only has 149 posts (confirmed by evaluating the xpath expression //input on it, as well as the number of posts that get imported in my test site) and not the 200 you mention in this ticket's description. Could that be why "Seems like others are left out"?

#4 in reply to: ↑ 3 @wzshop
8 years ago

Sorry, meant 149 posts.
See the vid here: ​https://youtu.be/IK84U3ERCi4

#5 @pbiron
8 years ago

  • Component Import → Query
  • Keywords 2nd-opinion added

Thanx for the video! Now that I understand what you are reporting, I can confirm that I'm seeing what you are seeing. At first sight this appears to be a bug in the importer, but it is not. The explanation is a little complicated, but I'll do my best.

First, you can confirm that "Post 129" was only imported once by searching for "Post 129" and seeing that it only appears once in the search results.

The reason it appears on page 1 & page 2 is the result of 2 factors:

  1. more than one post in the WXR file being imported have the same value in the <wp:post_date> element, thus more than one of the imported posts have the same value in the post_date field.
  2. the pagination on /wp-admin/edit.php is accomplished via LIMIT and ORDER BY clauses in the SQL request. MySQL performs certain optimizations on queries that include a LIMIT and those optimizations are what is producing the seemingly buggy behavior you're seeing.

As explained in MySQL's ​LIMIT Query Optimization,

If you combine LIMIT row_count with ORDER BY, MySQL stops sorting as soon as it has found the first row_count rows of the sorted result, rather than sorting the entire result. If ordering is done by using an index, this is very fast. If a filesort must be done, all rows that match the query without the LIMIT clause are selected, and most or all of them are sorted, before the first row_count are found. After the initial rows have been found, MySQL does not sort any remainder of the result set.

One manifestation of this behavior is that an ORDER BY query with and without LIMIT may return rows in different order, as described later in this section.

and more importantly

If multiple rows have identical values in the ORDER BY columns, the server is free to return those rows in any order, and may do so differently depending on the overall execution plan. In other words, the sort order of those rows is nondeterministic with respect to the nonordered columns.

So, with the posts imported from post export demo.xml, the 1st page of posts when Number of items per page = 20 (in Screen Options) contains some posts with post_date = 2018-06-11 15:51:54 and some with post_date = 2018-06-11 15:51:53 (the later including "Post 129") with the order of posts within each "group" being "random". The 2nd page contains some posts with post_date = 2018-06-11 15:51:53 (including "Post 129") and some with post_date = 2018-06-11 15:51:52.

If you change Number of items per page = 30 all of the posts with post_date = 2018-06-11 15:51:53 will appear on the 1st page (again in a "random" order) and thus "Post 129" will only appear on that 1st page.

Alternatively, if the posts in the site you exported (to produce post export demo.xml) all had unique post_date's, then when you import them into another site you will not see this behavior.

Yes, this is behavior is confusing, but it is a result of MySQL's "Limit Query Optimization" and is not a bug in the Importer nor in WP_Query.

Thus, I believe this ticket should be closed as "invalid", but I will leave it to one of the SQL gurus on the team to confirm my analysis and close it accordingly.

This ticket was mentioned in ​Slack in #core by pbiron. ​View the logs.


8 years ago

#7 @wzshop
8 years ago

Thank you very much for the broad explanation.
Is there any way to 'fix' this seemingly buggy behavior or a workaround, since it also has its effect on the frontend, when browsing the posts per category for instance. This is not something I would like to present to my visitors.

I can however comfirm that this problem won't occur when setting different dates.
Thanks again.

#8 @pbiron
8 years ago

Isn't non-determinisum fun :-)

Is there any way to 'fix' this seemingly buggy behavior

As trac is not the place for support questions, I'd suggest you ask that that question on ​https://wordpress.stackexchange.com/ (feel free to reference this ticket when you do).

#9 @wzshop
8 years ago

Ok, 1 more question to understand the MySQL's LIMIT Query Optimization;
It seems like posts are showing up multiple times, because of the posts are randomly ordered when browsing the pagination, but they don't actually show up multiple times?

Thanks

#10 @pbiron
8 years ago

In pure-SQL terms, any query of the form:

SELECT column FROM table ORDER BY order_by_column LIMIT x,y

where

  1. order_by_columnis indexed, and
  2. there are more than y rows with the same value in order_by_column that span the "pages" x-1 and x

could result in some rows from "page" x-1 also appearing on "page" x.

In WP_Query terms, the above translates into any query of the form:

<?php
$args = array(
    // the following are the indexed columns in the wp_posts table
    'orderby' => any of 'type', 'date', 'id', 'parent', 'author' or 'name'
    // each of the below "pagination" args results in a LIMIT clause
    'posts_per_page|showposts|posts_per_archive_page' => any value other than -1
);
$query = new WP_Query( $args );// including get_posts( $args )


There are other WP_Query corner cases that will exhibit the non-deterministic behavior as well, including but probably not limited to, hooking into any of posts_orderby, posts_clauses, posts_orderby_request, posts_clauses_request, posts_request to add any of the above (plus wp_posts.post_status, which is not directly legal in the args to new WP_Query()) to the "ORDER BY" clause of the SQL query that WP_Query ultimately executes.

#11 @SergeyBiryukov
8 years ago

#46929 was marked as a duplicate.

#12 @SergeyBiryukov
8 years ago

#39818 was marked as a duplicate.

#13 @terraGirl
7 years ago

Another vote for adding a second default "order by" for example the post ID.

My problem: we migrated data from an old osCommerce store over into WP. During the migration the timestamp for all media files defaulted to "1970-01-01 00:00:00". This meant we had over a thousand attachments with the same timestamp.

Result: In the media library, it was impossible to scroll through the images as the pages kept showing the same content.

Our solution: we looped through the posts & added 1 second to the post_date and post_date_gmt to make it unique. The media library can now be navigated with prev / next without any issues.

Preferred solution: The assumption that post_date is unique is flawed as data may have been imported externally. To avoid issues with paginated scrolling the default "order by" should be supplemented by "id" which is always unique.

#14 @danielbachhuber
6 years ago

If it helps, here's a little script I wrote to increment all of the post times by a second to "fix" the issue:

add_action(
	'init',
	function() {
		if ( isset( $_GET['dbx_increment_dates'] ) ) {
			$query = new WP_Query(
				[
					'post_type'      => $_GET['dbx_increment_dates'],
					'posts_per_page' => -1,
					'post_status'    => 'publish',
				]
			);
			$seconds = 1;
			foreach( $query->posts as $post ) {
				// Time is already non-standard, so we can assume it's fine.
				if ( false === stripos( $post->post_date, ' 00:00:00' ) ) {
					continue;
				}
				$new_date = gmdate( 'Y-m-d H:i:s', strtotime( $post->post_date ) + $seconds );
				wp_update_post(
					[
						'ID'            => $post->ID,
						'post_date'     => $new_date,
						'post_date_gmt' => $new_date,
					]
				);
				$seconds++;
			}
			echo 'process complete';
			exit;
		}
	}
);

#15 @danielbachhuber
6 years ago

  • Summary Posts show up multiple times in backend, when imported with Import WordPress tool → Posts show up multiple times in backend when they have the exact same date

#16 @johnbillion
6 years ago

I've seen this indeterminate sort order bug appear in several places in core when working on applications that ingest or generate a large amount data in bulk, therefore resulting in multiple posts with the exact same date.

The correct solution is to add secondary field to the orderby clause, as suggested above, eg post_date, ID, but this would need careful consideration.

#17 @TimothyBlynJacobs
6 years ago

I think this is related to #46294 as well?

#18 @johnbillion
6 years ago

Yep looks like it's the same underlying issue.

#19 @ocean90
6 years ago

#40376 was marked as a duplicate.

#20 follow-up: @joyously
6 years ago

I saw this also in the Theme Unit Test data, when I had duplicated some posts, they ended up with the exact same time (which could happen on a multi-author blog). But the output was always different between the list of latest posts and the links to Next/Prev once you were on a single post. So make sure the fix is not just for pagination, but also that the navigation links on single posts get the same order that the archive page has.
Would there be a case when the orderby should not have ID?

#21 in reply to: ↑ 20 @johnbillion
6 years ago

Replying to joyously:

Would there be a case when the orderby should not have ID?

I don't believe so, no. This change does have potential to break custom filters that are doing string replacements on the resulting SQL though.

This ticket was mentioned in ​Slack in #core by pbiron. ​View the logs.


6 years ago

This ticket was mentioned in ​Slack in #core by helen. ​View the logs.


6 years ago

#25 @hellofromTonya
6 years ago

Per core scrub today, this ticket is related to #47642 and #42936. Helen proposed:

Should probably lump those together and see if somebody is interested in targeting a 5.7-early fix

#27 @SergeyBiryukov
6 years ago

#52907 was marked as a duplicate.

This ticket was mentioned in ​Slack in #core-restapi by timothybjacobs. ​View the logs.


6 years ago

#29 @pbiron
5 years ago

#53201 was marked as a duplicate.

This ticket was mentioned in ​Slack in #core by peterwilsoncc. ​View the logs.


5 years ago

#31 @ramon fincken
5 years ago

Added new patch in #47642

#32 @ramonopoly
13 months ago

This bug came up when I was trying to filter attachment posts by media_type

https://core.trac.wordpress.org/ticket/64042

My workaround was, when multiple attachments have the same post_date, to add ID as secondary sort to guarantee consistent ordering.

`
$query_argsorderby = array( 'date' => $query_argsorder ?? 'DESC', 'ID' => 'DESC' );
`

Version 0, edited 13 months ago by ramonopoly (next)

#33 @ramonopoly
12 months ago

I've taken stab at fixing this over in ​https://github.com/WordPress/wordpress-develop/pull/10262

It's the first attempt, and probably the worst attempt, but it builds on ideas already tested by @ramon-fincken and @pbearne in https://core.trac.wordpress.org/ticket/47642

My main objective is to re-ignite this bug as it's very obvious when paginating in ​DataViews.

For example, bulk uploading media will give that media (usually) the same post_date. Paginating across pages in the media library, you will see the same attachment id on multiple pages.

Plus all the other examples mentioned in related tickets.

This ticket was mentioned in ​PR #10262 on ​WordPress/wordpress-develop by ​@ramonopoly.


11 days ago
#34

  • Keywords has-patch has-unit-tests added

I'm testing out an approach to fix a long-standing issue:

https://core.trac.wordpress.org/ticket/44349
https://core.trac.wordpress.org/ticket/47642
https://core.trac.wordpress.org/ticket/46294
https://core.trac.wordpress.org/ticket/52907 (closed as a duplicate of #44349)

Possibly related too:

https://core.trac.wordpress.org/ticket/52626
https://core.trac.wordpress.org/ticket/64042 (closed)

https://core.trac.wordpress.org/ticket/8107 was fixed separately in 6.9 ([61066]) in get_adjacent_post(), which does not use WP_Query. This PR does not change that path.

This change addresses potential duplicate records across pages when multiple posts share the same value for a field.

It updates WP_Query ordering to ensure deterministic results by adding ID as a secondary sort field.

Reproducing on trunk

Earlier attempts to write a failing test used orderby=date on a single post status. That never fails: the type_status_date index ends in ID, so the database supplies the tie-breaker by accident. Ordering on a column with no such index does fail.

Paging 10 at a time through a media library of 52 unattached uploads, ordered by parent (all of them share post_parent = 0):

trunk this branch
orderby=parent 24 of 52 returned, 28 duplicates 52 of 52, no duplicates
orderby=menu_order (pages) 40 of 84 returned, 44 duplicates 84 of 84, no duplicates

The parent case is what the media library's DataViews table hits when sorting on "Uploaded to", and what the block editor reaches through core-data:

( async () => {
        const q = { orderby: 'parent', order: 'asc', per_page: 10 };
        const s = wp.data.resolveSelect( 'core' );
        const p1 = await s.getEntityRecords( 'postType', 'attachment', { ...q, page: 1 } );
        const p2 = await s.getEntityRecords( 'postType', 'attachment', { ...q, page: 2 } );
        const ids1 = p1.map( i => i.id ), ids2 = p2.map( i => i.id );
        console.log( 'overlap:', ids1.filter( id => ids2.includes( id ) ) );
} )();

On trunk page 2 comes back identical to page 1. On this branch the overlap is empty.

Filters

The tie-breaker is built into the ORDER BY before posts_orderby, posts_clauses and their _request counterparts run, matching how WP_Comment_Query has added its own comment_ID tie-breaker since 4.4. Filters receive the ORDER BY that will actually run, and a filtered clause is used exactly as returned.

Ordering that is already unique gets no tie-breaker: ID, post__in, rand, and seeded RAND(n). post_parent__in and post_name__in still get one: several posts can share a parent, or a slug across post types.

Tests

npm run test:php -- --filter Tests_Query_DeterministicOrdering
npm run test:php -- --filter test_get_items_paged_by_parent_returns_each_attachment_once
npm run test:php -- --filter test_get_items_paged_by_menu_order_returns_each_page_once

On MariaDB 11.4, 21 of the 42 tests in Tests_Query_DeterministicOrdering fail without the fix, as do the REST media and pages tests. The assertions on the generated SQL fail on any engine; whether the paging assertions fail depends on how the database happens to sort ties.

​@ramonopoly commented on ​PR #10262:


11 days ago
#35

Given the scope and ambition of this PR, maybe it's safer to wait until after 7.2, and come up with an education/dev note plan.

There are some very slight risks in relation to what filters _might_ see.

See the collapsed section below

<details>

<summary>Status of this PR</summary>

Rebased on trunk (7.2-alpha) and reviewed. Five follow-up commits: `posts_clauses_request` once again blanks the ORDER BY when a callback returns no `orderby`, matching `posts_clauses` (the fallback was a leftover of the abandoned append-after-filters design); the redundant `array( 'none' )` check is gone; the four ORDER BY filter docblocks carry `@since 7.2.0` notes; tests now cover `comment_count` (the #47642 case) and `post_name__in`; `is_orderby_id()` is renamed `is_orderby_deterministic()`.

The fix appends `{$wpdb->posts}.ID` to the ORDER BY inside `WP_Query::get_posts()`, in the direction of the last column, before `posts_orderby` / `posts_clauses` and their `_request` variants run. It is skipped for `ID`, `post__in`, `rand` and seeded `RAND(n)`. Same placement as the `comment_ID` tie-breaker `WP_Comment_Query` has had since 4.4.

**Proof**

With trunk's `class-wp-query.php` swapped in, 21 of the 42 tests in `Tests_Query_DeterministicOrdering` fail, plus both new REST tests. The behavioural proof is `test_paging_by_date_returns_each_post_once`: 20 posts sharing one date across two statuses, paged 10 at a time, come back with a post on both pages on trunk. `test_paging_by_menu_order_returns_each_post_once`, `test_paging_by_post_parent__in_returns_each_post_once`, REST `test_get_items_paged_by_parent_returns_each_attachment_once` and `test_get_items_paged_by_menu_order_returns_each_page_once` reproduce the same way. The engine-independent guard is `test_id_clause_follows_the_sort_direction`, which asserts the generated SQL.

On the branch (MariaDB 11.4, Docker): the `query` group (1094 tests), REST posts/pages/media controllers (281/35/201), `Tests_Post_GetPages`, `Tests_Post_Query`, `Tests_Query_CacheResults` and `Tests_Admin_wpPrivacyRequestsTable` all pass. CI is green on the old base; the rebased branch has not been through CI yet.

**Risks**

- Filters see a longer clause. 736 plugins/themes on .org hook `posts_orderby` or `posts_clauses`; anything comparing the clause exactly or regexing on its end changes behaviour. Plugins that append their own ID clause produce a repeated ORDER BY column, which MariaDB and SQLite accept.
- The behavioural tests depend on the engine. On MariaDB 11.4 the title and meta-value paging tests pass on trunk because ties happened to sort stably; only the SQL assertions catch those cases everywhere.
- Ties now resolve by ID in the sort's direction. Sites that relied on the accidental order of same-date posts (imports, bulk publishing) get a stable but possibly different order.
- Every `WP_Query` cache key changes on upgrade because the SQL changed: one round of cache misses.
- Performance: EXPLAIN plans were identical with and without the ID clause for the default loop, attachments by parent, pages by menu_order and posts by title, and the single-status default query keeps its `type_status_date` index scan. Measured on a tiny table only.
- Out of scope: `WP_Term_Query` and `WP_User_Query`; `get_adjacent_post()` was fixed separately in 6.9 (#8107, with follow-up #64390).

</details>

Note: See TracTickets for help on using tickets.