High number of queries on the schema form
Closed this issue · 15 comments
When I edit a library, I get 124 sql queries. There seems to be some kind of loop, I always see the 3 following queries :
SELECT t0.id AS id1, t0.value AS value2, t0.definition_id AS definition_id3 FROM mednum_portal_attribute t0 INNER JOIN mednum_portal_subscriber_attribute ON t0.id = mednum_portal_subscriber_attribute.attribute_id WHERE mednum_portal_subscriber_attribute.subscriber_id = 21613
SELECT
m0_.id AS id0,
m0_.salutation AS salutation1,
m0_.given_name AS given_name2,
m0_.family_name AS family_name3,
m0_.subscriber_id AS subscriber_id4,
m0_.email AS email5,
m0_.birth_date AS birth_date6,
m0_.nick_name AS nick_name7,
m0_.password AS password8,
m0_.subscription_end_date AS subscription_end_date9,
m0_.numeric_password_hash AS numeric_password_hash10,
m0_.created_at AS created_at11,
m0_.updated_at AS updated_at12,
m0_.comment AS comment13,
m0_.encrypted_password AS encrypted_password14,
m0_.newsletter AS newsletter15,
m0_.last_login AS last_login16,
m0_.offer_type_configuration_hash AS offer_type_configuration_hash17,
m0_.roles AS roles18,
m0_.tac_version AS tac_version19,
m0_.sso_id AS sso_id20,
m0_.pin AS pin21,
m0_.uuid AS uuid22,
m0_.active AS active23,
m0_.fake AS fake24,
m0_.library_id AS library_id25
FROM
mednum_portal_subscriber m0_
INNER JOIN mednum_portal_subscriber_attribute m2_ ON m0_.id = m2_.subscriber_id
INNER JOIN mednum_portal_attribute m1_ ON m1_.id = m2_.attribute_id
AND (m1_.definition_id = ?)
WHERE
m0_.id = ?
SELECT
m0_.id AS id0,
m0_.salutation AS salutation1,
m0_.given_name AS given_name2,
m0_.family_name AS family_name3,
m0_.subscriber_id AS subscriber_id4,
m0_.email AS email5,
m0_.birth_date AS birth_date6,
m0_.nick_name AS nick_name7,
m0_.password AS password8,
m0_.subscription_end_date AS subscription_end_date9,
m0_.numeric_password_hash AS numeric_password_hash10,
m0_.created_at AS created_at11,
m0_.updated_at AS updated_at12,
m0_.comment AS comment13,
m0_.encrypted_password AS encrypted_password14,
m0_.newsletter AS newsletter15,
m0_.last_login AS last_login16,
m0_.offer_type_configuration_hash AS offer_type_configuration_hash17,
m0_.roles AS roles18,
m0_.tac_version AS tac_version19,
m0_.sso_id AS sso_id20,
m0_.pin AS pin21,
m0_.uuid AS uuid22,
m0_.active AS active23,
m0_.fake AS fake24,
m0_.library_id AS library_id25
FROM
mednum_portal_subscriber m0_
INNER JOIN mednum_portal_subscriber_attribute m2_ ON m0_.id = m2_.subscriber_id
INNER JOIN mednum_portal_attribute m1_ ON m1_.id = m2_.attribute_id
AND (m1_.definition_id = ?)
WHERE
m0_.id = ?
Are you passing the subscriber to the form like this? (Or the ParamConverter equivalent?)
$subscriber = $em->find('...', $id);
If so, you should create a custom $qb in your doctrine Repository, with attributes and definitions already joined.
I am using sonata, so I do not doing it myself. I will try to see have to customize the query. What I don't understand though is why I get so many queries : I only have 2 attributes at the moment.
Oh and also please note : This is not on the subscriber form (I made a mistake when writing this bug report), but on the library form. Strange, isn't it ? I believe this has to do with one of the listeners.
I am going to get all the joins I need automatically done. I tried adding fetch: EAGER to the schema relation mapping, without success. Here is my mapping:
manyToMany:
schema:
targetEntity: Padam87\AttributeBundle\Entity\Definition
cascade: [persist, remove]
It really is a problem with the AttributeCreatorListener. I end up outside the foreach 37 times (the number of subscribers in the library), and inside the foreach 74 times (because I have 2 definitions).
EDIT: It happens because I'm trying to get subscribers with a special role in my library. I'm getting all the subscriber and I need to check an array property on the subscriber to know if he is a contributor or not, so I get is has nothing to do with the bundle, sorry for the noise.
Reopening the issue : maybe the attributeCreatorListener should be less aggressive ? I think the piece of code inside it should be triggered in a lazier way, typically, when you need to access the $attributes array. This could be done by injecting a service in the entity on postLoad(). The listener would only do that, and in the trait, when the attributes property is accessed, it would be lazily hydrated. What do you think ?
I got it to work, I can contribute it if you are interested. Here is how it looks :
<?php
namespace Padam87\AttributeBundle\Listener;
use Doctrine\ORM\Event\LifecycleEventArgs;
use Doctrine\Common\Annotations\AnnotationReader;
use Doctrine\ORM\UnitOfWork;
use Padam87\AttributeBundle\Entity\Attribute;
use JMS\DiExtraBundle\Annotation as DI;
use Doctrine\DBAL\DBALException;
/**
* @DI\Service("attribute.attribute_creator")
* @DI\Tag("doctrine.event_listener", attributes = {"event" = "postLoad"})
*/
class AttributeCreatorListener
{
private $loader;
/**
* @DI\InjectParams({"loader" = @DI\Inject("attribute.attribute_loader")})
*/
public function __construct($loader)
{
$this->loader = $loader;
}
public function postLoad(LifecycleEventArgs $eventArgs)
{
$em = $eventArgs->getEntityManager();
$this->loader->setEntityManager($em);
$entity = $eventArgs->getEntity();
$refl = new \ReflectionClass($entity);
$reader = new AnnotationReader();
if ($reader->getClassAnnotation($refl, 'Padam87\AttributeBundle\Annotation\Entity') != null) {
$entity->setAttributeLoader($this->loader);
}
}
}
namespace Padam87\AttributeBundle\Entity;
use Doctrine\ORM\Mapping as ORM;
trait AttributedEntityTrait
{
/**
* @var \Padam87\AttributeBundle\Entity\Attribute
*
* @ORM\ManyToMany(targetEntity="\Padam87\AttributeBundle\Entity\Attribute", fetch="EAGER", cascade={"persist", "remove"})
*/
private $attributes;
private $areAttributesLoaded = false;
/**
* Add attributes
*
* @param \Padam87\AttributeBundle\Entity\Attribute $attributes
*/
public function addAttribute(\Padam87\AttributeBundle\Entity\Attribute $attributes)
{
$this->attributes[] = $attributes;
return $this;
}
/**
* Remove attributes
*
* @param \Padam87\AttributeBundle\Entity\Attribute $attributes
*/
public function removeAttribute(\Padam87\AttributeBundle\Entity\Attribute $attributes)
{
$this->attributes->removeElement($attributes);
}
/**
* Get attributes
*
* @return \Doctrine\Common\Collections\Collection
*/
public function getAttributes()
{
$this->loadAttributes();
return $this->attributes;
}
public function setAttributeLoader($loader)
{
$this->loader = $loader;
}
public function loadAttributes()
{
if (!$this->areAttributesLoaded) {
$this->loader->load($this);
}
$this->areAttributesLoaded = true;
}
}
namespace Padam87\AttributeBundle\Hydration;
use JMS\DiExtraBundle\Annotation as DI;
use Doctrine\Common\Persistence\ObjectManager;
namespace Padam87\AttributeBundle\Hydration;
use JMS\DiExtraBundle\Annotation as DI;
use Doctrine\Common\Persistence\ObjectManager;
use Padam87\AttributeBundle\Entity\Attribute;
use Doctrine\ORM\UnitOfWork;
/**
* Loads attributes in an entity. Should be called lazily.
*
* @DI\Service("attribute.attribute_loader")
*/
class AttributeLoader
{
/**
* For some reason, constructor injection gives a circular reference exception.
* Maybe the entity manager depends on the listeners ?
*/
public function setEntityManager(ObjectManager $em)
{
$this->em = $em;
}
public function load($entity)
{
$refl = new \ReflectionClass($entity);
$uow = $this->em->getUnitOfWork();
try {
/* $schema = $em->getRepository('Padam87AttributeBundle:Schema')->findOneBy(array( */
/* 'className' => $refl->getName() */
/* )); */
$schema = array(
$entity->getLibrary()->getDefinition1(),
$entity->getLibrary()->getDefinition2(),
);
if ($schema !== null) {
/* foreach ($schema->getDefinitions() as $definition) { */
foreach ($schema as $definition) {
$qb = $this->em->getRepository($refl->getName())->createQueryBuilder('main');
$qb->join('main.attributes', 'a', 'WITH', 'a.definition = :definition');
$qb->where('main = :main');
$qb->setParameter('definition', $definition);
$qb->setParameter('main', $entity);
$attribute = $qb->getQuery()->getOneOrNullResult();
if ($attribute === null) {
$attribute = new Attribute();
$attribute->setDefinition($definition);
$entity->addAttribute($attribute);
if ($uow->getEntityState($entity) == UnitOfWork::STATE_MANAGED) {
$this->em->persist($entity);
$this->em->flush($entity);
}
}
}
}
} catch (DBALException $e) {
// Discard DBAL exceptions in order for schema:update to work
}
}
}It does not work for the moment, I'll try to get there soon…
The problem here is the consistency check. Basically there are two options. Whenever a definition is changed, update all the options (this is bad, and could easily cause a timeout if there is enough data) OR as it's done now, when an object is fetched, check for changes.
I think your solution wold probably break the auto update.
I think I will add a changedAt field to the schema, and only rebuild attributes when necessary.
as it's done now, when an object is fetched, check for changes.
My solution doesn't change this that much. When an object is fetched, nothing is checked, but as soon as the attributes are accessed, then the check occurs. So the auto update wouldn't be broken it would be "delayed", if you will.
I also had to make the relationship not eager in order to decrease the number of requests.
My solution is not so good: since my Subscriber are also my user classes, I had to make sure the loader does not get serialized (PDO instances cannot be serialized, and the loader is not really part of what should stay a Plain Old PHP Object).
I resorted to this kind of hack, but it is does not look really good :
public function __sleep()
{
return array_diff(array_keys(get_object_vars($this)), array('loader'));
}
I reverted the change on my project, but this is a real blocker for me : whenever I access the library subscribers (to filter them for instance), this attribute creation is done. It should be reduced to the few instances where it makes sense. So, when does it make sense to make sure a subscriber has all attributes ?
- When the subscriber is created
- When the schema changes (at first, could be approximated to "when the library is saved")
I think the listeners should be made optional, and that is easier now that the DIEBundle has been removed. The code that does the attribute creation should be extracted from it so that it can be called manually.
I managed to solve my problem with this very simple workaround : not using the trait, and doing this in my class :
/**
* Get attributes
*
* @return \Doctrine\Common\Collections\Collection
*/
public function getAttributes()
{
foreach ($this->getLibrary()->getSchema() as $definition) {
if (!$this->attributes->exists(
function ($index, $attribute) use ($definition) {
return $attribute->getDefinition() == $definition;
}
)) {
$attribute = new Attribute();
$attribute->setDefinition($definition);
$this->addAttribute($attribute);
}
}
return $this->attributes;
}I don't think this could be applied to the bundle because this is very specific to my use case : the subscriber has access to the schema, I am not sure it is the case when using the bundle normally. Anyway, it is a good reason to make the listeners disablable I will disable the listeners by simply removing the annotation on my entity.
EDIT: I can see there is a Schema entity in the bundle. adding a relation to the schema in the trait would make this work I think. If you think it is possible, I can already tell you that the performance implications are huge : loading a library edition form took me 1.5 s, it is now down to 42 ms As a bonus, the database does not get filled with empty values unless the user is persisted.
This has been fixed in 3.0