[com_fields] Fetch all custom field values in one query #14558

laoneo · 2017-03-13T06:59:54Z

Pull Request for Issue #14473.

Summary of Changes

Right now when the values of the fields are fetched, for every field a query is done. This pr changes the behavior to one query for all fields.

Integer indexes are faster to lookup than string indexes, the context column got removed from the field_values table, we gain a bit of performance here as well.

Ping @Bakual and @mbabker for review.
Ping @alikon and @csthomas did I change the installer and upgrade scripts correctly?

Testing Instructions

Create some fields.
Edit an article.
Set some values for the fields.
Save the article.
View the article on the front.

Expected result

All of these should work as before, but with less queries. When the debugging is on, then you should see less queries logged.

Actual result

It works, but with more queries.

Documentation Changes Required

None.

zero-24 · 2017-03-13T07:41:24Z

administrator/components/com_fields/models/field.php

+		$fieldsQuery = $this->getDbo()->getQuery(true);
+		$fieldsQuery->select($fieldsQuery->qn('id'))
+			->from($fieldsQuery->qn('#__fields'))
+			->where($fieldsQuery->qn('context') . ' = ' . $fieldsQuery->q($context));


This looks wrong to me as you have removed context from the db table ;)

That query is against #__fields and not #__fields_values. It is correct.

Ah thanks i got confused. 👍

Do you know if we use in the core a delete with an inner join as subselects are slower? I couldn't find a way to do it.

I don't know that we use it. But that does not mean that we don't use it. 😄

I'm not that concerned about performance for deleting fields. It's not done multiple times per second usually. Much more important that it works reliably 😄

@laoneo in com_finder there is something similar https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_finder/helpers/indexer/taxonomy.php#L345

I saw that, but also doesn't use an inner join.

csthomas · 2017-03-13T07:58:28Z

administrator/components/com_fields/helpers/fields.php

 				}

-				if ($field->value === '' || $field->value === null)
+				if (!isset($field->value) || $field->value === '' || $field->value === null)


$field->value === null this will never happen. isset() will check that.

laoneo · 2017-03-13T08:01:04Z

Thanks @csthomas removed the double null check.

csthomas · 2017-03-13T08:07:28Z

administrator/components/com_admin/sql/updates/mysql/3.7.0-2016-08-29.sql

@@ -66,11 +66,9 @@ CREATE TABLE IF NOT EXISTS `#__fields_groups` (

 CREATE TABLE IF NOT EXISTS `#__fields_values` (
  `field_id` int(10) unsigned NOT NULL,
-  `context` varchar(255) NOT NULL,
  `item_id` varchar(255) NOT NULL COMMENT 'Allow references to items which have strings as ids, eg. none db systems.',
  `value` text NOT NULL DEFAULT '',


You are not changed it now, but database field with type TEXT NOT NULL can not have default value on mysql.
https://dev.mysql.com/doc/refman/5.7/en/blob.html

There is a lot of such mistakes in joomla but please do not repeat it.
I see 2 solutions. Allow to NULL or always set default value in php code.

That sort of stuff should be done in a separate pr, basically on thing in one pr, otherwise it gets harder to review.

Yes, I want only mention about it.

No problem.

yvesh · 2017-03-14T07:51:17Z

administrator/components/com_fields/models/fields.php

@@ -109,7 +109,7 @@ protected function getStoreId($id = '')
 		$id .= ':' . serialize($this->getState('filter.assigned_cat_ids'));
 		$id .= ':' . $this->getState('filter.state');
 		$id .= ':' . $this->getState('filter.group_id');
-		$id .= ':' . print_r($this->getState('filter.language'), true);
+		$id .= ':' . serialize($this->getState('filter.language'));


serialize? :-)

Why not, it is an array and serialize is also used for the assigned cats.

joomdonation · 2017-03-14T11:52:34Z

administrator/components/com_fields/models/field.php

+
+		if (!key_exists($key, $this->valueCache))
+		{
+			$this->valueCache[$key] = null;


If I read the code correctly, this line is not needed. You set $this->valueCache[$key] = $data; later already

joomdonation · 2017-03-14T11:53:41Z

administrator/components/com_fields/models/field.php

+
+			$query->select(array($query->qn('field_id'), $query->qn('value')))
+				->from($query->qn('#__fields_values'))
+				->where($query->qn('field_id') . ' in (' . implode(',', ArrayHelper::toInteger($fieldIds)) . ')')


Should be IN instead of in. Am I correct that we always use upper case string for SQL keywords?

Bakual · 2017-03-14T12:04:48Z

After thinking about this a bit, the only question I have is if we really need both the "getFieldValue" and "getFieldValues" methods. They do quite the same just instead of using a "->where field_id IN (...)" instead of "->where field_id = ...".
That could also be done using the same method with a conditional based on the field_id parameter (multiple ids vs single integer).

mbabker · 2017-03-14T12:27:04Z

Please don't combine the two. That violates SRP.

Bakual · 2017-03-14T12:53:36Z

Stupid question, what is SRP?

rdeutz · 2017-03-14T12:55:52Z

single responsibility prinzip

Bakual · 2017-03-14T13:15:12Z

Makes sense 😄

So better to have a method to fetch the value(s) for a single field and one for fetching values for multiple fields allthough they do basically the same thing?
If that's preferred, then it is fine as it is. It was just something to consider 👍

mbabker · 2017-03-14T13:29:38Z

Right. Basically you want your methods to be responsible for one task and one task only. Fetching the data for a single field and fetching the data for multiple fields are two different tasks, will presumably have two different return types (one might be a value object and the other may be a collection of value objects), and may be post-processed by two different code paths (even if you're doing a foreach loop and it just calls the path for the single item, it still has to have that extra logic). So it's better from a structural standpoint to separate those out.

laoneo · 2017-03-15T07:19:36Z

Changed the getValue function to use getValues and removed the not needed code as pointed out by @joomdonation. Additionally added some inline comments to explain what the function is doing.

rdeutz · 2017-03-15T07:24:27Z

@laoneo when you on it could you fix the code style issues

laoneo · 2017-03-15T07:58:51Z

@rdeutz which ones? Travis is running trough.

laoneo · 2017-03-15T07:59:43Z

Ah sorry, didn't realize that drone is now doing the CS stuff. Give me a minute.

zero-24 · 2017-03-15T08:33:23Z

done @laoneo

laoneo · 2017-03-15T08:34:55Z

@rdeutz all good now.

laoneo · 2017-03-20T09:28:15Z

Should we also add here a release blocker label as it reduces the amount of SQL queries?

mbabker · 2017-03-20T12:20:21Z

Added due to the major performance issues this PR aims to address.

joomdonation · 2017-03-24T12:59:55Z

I have tested this item 🔴 unsuccessfully on c2af37b

There are SQL error causes by this PR:

I created two text fields
I created to articles
I created a menu item to link to category blog layout menu option

Access to that menu item, see this error

1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ') AND item_id = '2'' at line 3

The actual SQL command is

SELECT field_id,value FROM jos_fields_values WHERE field_id IN () AND item_id = '2'

Look like in some condition, field_id doesn't have any data for some reasons

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14558.}

joomdonation · 2017-03-24T13:04:53Z

BTW, in my test, look like the custom fields are not being displayed for Category Blog menu option. So in that case, could we stop querying the database to get data? (I haven't tested this feature much so far, so maybe I just mis-understand how it works)

mbabker · 2017-03-24T13:27:55Z

I don't think I would hardcode the plugin context to exclude category views. In theory someone could do a category layout using fields if desired.

joomdonation · 2017-03-24T14:01:07Z

OK. That makes sense (although with default usage like this, it is a waste of resource to query data without using it - It might has performance impact). Also, one thing about category view which I cannot test right now because above error. Before patch, for each item, an SQL query is is generated to get data for a field, so with 2 articles, 2 fields, 4 query total. I guess if this patch works, there will only one query for one article, so still 2 query. Do we want to merge it to one query to get data for all custom fields of all articles (not sure if it is possible)

Bakual · 2017-03-24T16:16:07Z

Do we want to merge it to one query to get data for all custom fields of all articles (not sure if it is possible)

Not possible with the current plugin events. You would need a new event for the whole list.

laoneo · 2017-03-25T14:24:00Z

@joomdonation I'v added a test if the field ids are empty. Now the error should be gone.

joomdonation · 2017-03-25T14:38:51Z

I have tested this item ✅ successfully on 23fac40

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14558.}

joomdonation · 2017-03-25T14:40:27Z

OK. It works now. Unrelated to this PR, on category blog page, still one query per article if you want to improve (in case it can be improved)

laoneo · 2017-03-25T15:29:07Z

Open an issue for it. But as @Bakual said, it needs a new event for it, probably not a bad idea, but I'm not sure if it is doable in the 3 series.

klas · 2017-03-26T22:01:09Z

I have tested this item ✅ successfully on 23fac40

with 100 items in list number of queries is reduced from 1000 to 100, there is also jsut a single query on single item.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14558.}

klas · 2017-03-26T22:14:38Z

This is much better but still not quite there (not the fault of this PR), we have the same problem with tags.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14558.}

laoneo · 2017-03-27T06:41:33Z

Guess RTC?

sbouey · 2017-03-27T11:48:24Z

work now with Falang

laoneo added 5 commits March 11, 2017 13:33

Add function to fetch all fields in one shot

3da7057

Remove the context from the field_values table as it is redundant

a695dcb

Fix notices errors

3cf4748

Cosmetic

85e5b53

Remove the context from the upgrade scripts

90bcfd0

joomla-cms-bot added the PR-staging label Mar 13, 2017

This was referenced Mar 13, 2017

Fields Performance - Value Queries #14473

Closed

Removing unneeded where clause from query and fixing a notice Digital-Peak/joomla-cms#4

Closed

Remove notice

2dccb74

zero-24 reviewed Mar 13, 2017

View reviewed changes

csthomas reviewed Mar 13, 2017

View reviewed changes

No need to double check of null

efacd96

csthomas reviewed Mar 13, 2017

View reviewed changes

yvesh reviewed Mar 14, 2017

View reviewed changes

yvesh approved these changes Mar 14, 2017

View reviewed changes

joomdonation reviewed Mar 14, 2017

View reviewed changes

laoneo added 2 commits March 15, 2017 07:52

Remove not needed code

2b47cf6

getValue of a single field is using the getValues function

0696ca0

GeraintEdwards mentioned this pull request Mar 15, 2017

[com_fields] Enable custom fields to be configured to output in different contexts #14641

Closed

mbabker added the Release Blocker label Mar 20, 2017

zero-24 added this to the Joomla 3.7.0 milestone Mar 24, 2017

laoneo added 2 commits March 25, 2017 15:20

Merge branch 'staging' into cf/performance

6ad999a

Check if fields ids is empty

23fac40

brianteeman mentioned this pull request Mar 26, 2017

Huge number of queries performed on lists due to fields #14912

Closed

laoneo mentioned this pull request Mar 27, 2017

[com_fields] Add field_id to getFieldValue query #14884

Closed

rdeutz merged commit 1c52ed3 into joomla:staging Mar 27, 2017

rdeutz removed the Release Blocker label Mar 27, 2017

normanmm mentioned this pull request Mar 27, 2017

[com_fields] Add support for max length for text and textarea #14458

Merged

laoneo deleted the cf/performance branch March 27, 2017 11:50

This comment was marked as abuse.

Sign in to view

laoneo mentioned this pull request Apr 2, 2017

[com_fields] values are saved without context #15048

Closed

[com_fields] Fetch all custom field values in one query #14558

[com_fields] Fetch all custom field values in one query #14558

Conversation

laoneo commented Mar 13, 2017

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

Choose a reason for hiding this comment

laoneo Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laoneo commented Mar 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bakual commented Mar 14, 2017

mbabker commented Mar 14, 2017

Bakual commented Mar 14, 2017

rdeutz commented Mar 14, 2017

Bakual commented Mar 14, 2017

mbabker commented Mar 14, 2017

laoneo commented Mar 15, 2017

rdeutz commented Mar 15, 2017

laoneo commented Mar 15, 2017

laoneo commented Mar 15, 2017

zero-24 commented Mar 15, 2017

laoneo commented Mar 15, 2017

laoneo commented Mar 20, 2017

mbabker commented Mar 20, 2017

joomdonation commented Mar 24, 2017

joomdonation commented Mar 24, 2017

mbabker commented Mar 24, 2017

joomdonation commented Mar 24, 2017 • edited Loading

Bakual commented Mar 24, 2017

laoneo commented Mar 25, 2017

joomdonation commented Mar 25, 2017

joomdonation commented Mar 25, 2017

laoneo commented Mar 25, 2017

klas commented Mar 26, 2017

klas commented Mar 26, 2017

laoneo commented Mar 27, 2017

sbouey commented Mar 27, 2017

This comment was marked as abuse.

laoneo Mar 13, 2017 •

edited

Loading

joomdonation commented Mar 24, 2017 •

edited

Loading