Make WordPress Core

Opened 8 months ago

Closed 8 months ago

Last modified 7 months ago

#58673 closed defect (bug) (fixed)

WordPress database error when installing PHPUnit tests

Reported by: chouby's profile Chouby Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.3 Priority: high
Severity: normal Version: 6.3
Component: Editor Keywords: has-patch needs-testing commit
Focuses: performance Cc:

Description

It is reported for example in this job

WordPress database error:</strong> [Table &#039;wordpress_develop_tests.wptests_posts&#039; doesn&#039;t exist]<br /><code>
					SELECT   wptests_posts.ID
					FROM wptests_posts 
					WHERE 1=1  AND ( 
  0 = 1
) AND wptests_posts.post_type = &#039;wp_global_styles&#039; AND ((wptests_posts.post_status = &#039;publish&#039;))
					GROUP BY wptests_posts.ID
					ORDER BY wptests_posts.post_date DESC
					LIMIT 0, 1
					SELECT   wptests_posts.ID
					FROM wptests_posts 
					WHERE 1=1  AND ( 
  0 = 1
) AND wptests_posts.post_type = 'wp_global_styles' AND ((wptests_posts.post_status = 'publish'))
					GROUP BY wptests_posts.ID
					ORDER BY wptests_posts.post_date DESC
					LIMIT 0, 1
				 made by require_once('wp-settings.php'), do_action('wp_loaded'), WP_Hook->do_action, WP_Hook->apply_filters, call_user_func_array, WP_Duotone::set_global_style_block_names, WP_Theme_JSON_Resolver::get_merged_data, WP_Theme_JSON_Resolver::get_user_data, WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles, WP_Query->query, WP_Query->get_posts

I noticed it in WP 6.3-beta2-56110. It is not reported in WP 6.3-beta2

Attachments (1)

58673.diff (683 bytes) - added by SergeyBiryukov 8 months ago.

Download all attachments as: .zip

Change History (29)

#1 @Chouby
8 months ago

  • Component changed from General to Editor

It seems that this error occurs since [56101]

Last edited 8 months ago by Chouby (previous) (diff)

#2 @SergeyBiryukov
8 months ago

  • Milestone changed from Awaiting Review to 6.3

#3 @swissspidy
8 months ago

  • Keywords needs-patch added

This is still happening for me on trunk.

It happens the first time when installing & running the PHPUnit tests from a plugin.

After that, all the database tables exist, so on the second run it's not failing.

From what I can see:

  1. phpunit/includes/bootstrap.php calls phpunit/includes/install.php to install WordPress
  2. phpunit/includes/install.php loads wp-settings.php
  3. wp-settings.php fires wp_loaded
  4. WP_Duotone::set_global_style_block_names() is hooked to wp_loaded and triggers a WP_Query
  5. Now you have the database error

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


8 months ago

#5 @costdev
8 months ago

If this only occurs in the test suite and we want to avoid changes to src, could we just include wp-includes/plugin.php, unhook the action near the start of tests/phpunit/includes/install.php and hook it again at the end? Or do similar in tests/includes/bootstrap.php around lines 261-269?

Last edited 8 months ago by costdev (previous) (diff)

#6 @swissspidy
8 months ago

That'd just paper over the issue, we should rather fix this at the root.

There is no point in doing a WP_Query on every single page load just for duotone. wp_loaded is really not the right way for this.

#7 @costdev
8 months ago

Agreed, as it's also happening when installing Beta 4.

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


8 months ago

#9 @SergeyBiryukov
8 months ago

  • Keywords has-patch added; needs-patch removed

This seems similar to a few earlier tickets: #53830, #54634, #54800, #54849, #55632.

In line with [53306] / #55632, perhaps WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles() could bail early if wp_installing(), see 58673.diff.

Last edited 8 months ago by SergeyBiryukov (previous) (diff)

#10 @spacedmonkey
8 months ago

I strongly recommend removing these lines

add_action( 'wp_loaded', array( 'WP_Duotone', 'set_global_styles_presets' ), 10 );
add_action( 'wp_loaded', array( 'WP_Duotone', 'set_global_style_block_names' ), 10 );

Loading these method on every requests is wasteful, as duo tone is not used on all pages. REST API calls for example.

#11 follow-up: @joemcgill
8 months ago

  • Focuses performance added
  • Priority changed from normal to high

@spacedmonkey noticed that this same WP_Duotone::set_global_styles_presets method being called on every request is causing a large unnecessary performance regression in trunk (7–9% of total server response time in my profiling).

While ensuring this isn't run during wp_installing() would be sufficient to fix this issue, it would be nice to fix the performance regression at the same time, by only calling that method when it's needed.

I'm adding the performance focus and increasing the priority on this to "high" so we can investigate and resolve this prior to RC1. @SergeyBiryukov if you prefer to commit your patch in the mean time, we can open a new ticket for the performance regression.

cc: @isabel_brison and @onemaggie, since you were both involved in the original commit that introduced this functionality.

#13 @spacedmonkey
8 months ago

@joemcgill @flixos90 I put together a little POC of lazily loading the expensive properties in the Duotone class.

Take a look at the PR #4831.

#14 in reply to: ↑ 11 @SergeyBiryukov
8 months ago

Replying to joemcgill:

While ensuring this isn't run during wp_installing() would be sufficient to fix this issue, it would be nice to fix the performance regression at the same time, by only calling that method when it's needed.

Yeah, removing this from wp_loaded and calling conditionally when needed would be ideal.

PR #4831 looks good to me at a glance.

#15 @flixos90
8 months ago

Cross-posting here from my Slack comment: Beta 4 benchmarks show a small regression in server-side performance compared to Beta 2 benchmarks. Given the commits from #58555 happened after Beta 2, together with what @joemcgill reported above, they may be partially responsible for that regression.

#16 @flixos90
8 months ago

I conducted server-side performance benchmarks for https://github.com/WordPress/wordpress-develop/pull/4831 (see the results): While it looks like the PR is improving performance overall, it is only a small overall difference (in the wp-total metric) of less than 1%. Interestingly, the PR makes the wp-before-template metric faster but the wp-template metric slower. It's almost as if the performance overhead is just being moved elsewhere.

I agree with the previous comments and with the approach of the PR. That said, the benchmark difference is too small to reliably confirm this as a regression. So I doubt that this change is what caused the overall regression that I shared in 15.

Maybe profiling can help clarify. @spacedmonkey Can you please do a profiling before & after comparison on your end?

Last edited 8 months ago by flixos90 (previous) (diff)

#17 @spacedmonkey
8 months ago

Thanks for doing the benchmarking here @flixos90 .

I beleive the biggest performance issue here is this line. Parsing and merging all this data, when it was not doing so before. I expected to see better results than 1%. I will do some profiling in the morning and get back. My PR just defers this later / when it is needed. I saw REST API calls being much faster for example.

it worth noting, you need to test on a classic theme without a theme.json, with a theme.json and a block theme.


#18 @spacedmonkey
8 months ago

@flixos90 @swissspidy @joemcgill

I have spent some time improving #4831, it now ready for review. Details of the change are detailed in the description.

This change, only defers the loading of theme.json data until it needed. This fixes the PHPUnit issue in this ticket and means Duo tone only loads if needed. Without a larger refactor ( out of scope of this stage of release ), we can not get rid of this performance regression completely. Calling WP_Theme_JSON_Resolver::get_merged_data() is expensive and there is no getting around that for now.

In my testing, I compared the following. Block / classic theme, post with and without a image block ( that support duo tone) and a REST API request with no blocks.

These were run on 1000 runs, PHP 8.2, using test data.

URLSuccess RateResponse Time (median)wp-load-alloptions-query (median)wp-before-template (median)wp-before-template-db-queries (median)branchtheme
/2023/01/13/media-category-blocks/100%69.450.8930.42.13trunktwentytwentyone
/2023/01/13/media-category-blocks/100%85.330.7738.312.89trunktwentytwentythree
/2023/01/13/media-category-blocks/100%59.370.7621.631.77fix/fix-duo-tonetwentytwentyone
/2023/01/13/media-category-blocks/100%82.330.7535.692.7fix/fix-duo-tonetwentytwentythree
/2023/07/10/hello-world/100%64.940.9131.252.11trunktwentytwentyone
/2023/07/10/hello-world/100%97.630.946.663.46trunktwentytwentythree
/2023/07/10/hello-world/100%52.870.7721.321.71fix/fix-duo-tonetwentytwentyone
/2023/07/10/hello-world/100%79.40.7636.572.72fix/fix-duo-tonetwentytwentythree
/wp-json/wp/v2/types100%40.19N/AN/AN/Atrunktwentytwentyone
/wp-json/wp/v2/types100%41.33N/AN/AN/Atrunktwentytwentythree
/wp-json/wp/v2/types100%28.12N/AN/AN/Afix/fix-duo-tonetwentytwentyone
/wp-json/wp/v2/types100%40.37N/AN/AN/Afix/fix-duo-tonetwentytwentythree

#19 @nazmul111
8 months ago

I am Not find it when installing to 6.3beta4

#20 @mukesh27
8 months ago

  • Keywords needs-testing added

Thanks @spacedmonkey for the PR. It need manual testing.

This ticket was mentioned in Slack in #cli by swissspidy. View the logs.


8 months ago

@ajlende commented on PR #4831:


8 months ago
#22

However, there seems to be a bug caused by this based on https://github.com/WordPress/wordpress-develop/pull/4831#pullrequestreview-1526511182? From that perspective, I am slightly worried about the refactoring in this PR. While I'm onboard with the PR in its current state, we may want to be more cautious and go with the much simpler change based on the changes from yesterday, before you refactored further.

I've tested it and it is fixed by f10a53f.

@swissspidy commented on PR #4831:


8 months ago
#23

Can we rebase this now that 4839 has been merged?

#24 @swissspidy
8 months ago

@nazmul111 This only happens when installing the WordPress test suite when doing custom plugin/theme development, not when regularly installing WordPress itself.

#25 @spacedmonkey
8 months ago

  • Keywords commit added
  • Owner set to spacedmonkey
  • Status changed from new to assigned

#26 @spacedmonkey
8 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 56226:

Editor: Lazily load Duotone settings only when needed.

Introduced in [56101] the WP_Duotone class, hooks into the wp_loaded action to load duotone style data from global styles. Hooking in early in the bootstrap process caused a number of problems. This hook, triggered an error on installing, as this lookup for global styles, would result in a global post trying to be created, even before the table existed. Additionally, this implementation caused a severe performance regression, as duotone styling data was loaded unnecessarily for requests that did not require such data, such as REST API calls or actions within the wp-admin interface.

In this change, refactor the WP_Duotone to lazily load the global styles and theme.json data, only when a block that supports duotone is encountered. The method render_duotone_support was change to take a third parameter to reuse the existing WP_Block_Type object passed to the filter, to save it being looked up again. The code has also got improved type checking and the use of the util function block_has_support. Furthermore, the code's readability has been improved, along with enhancements to the documentation blocks.

Props Chouby, spacedmonkey, SergeyBiryukov, swissspidy, costdev, joemcgill, flixos90, mukesh27, nazmul111, ajlende, isabel_brison.
Fixes #58673.

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


7 months ago

Note: See TracTickets for help on using tickets.