Data providers perform unnecessary COUNT queries that negatively affect performance

[sleptor]

What steps will reproduce the problem?

Check the debug panel on any page containing GridView.

What is the expected result?

Single COUNT query

What do you get instead?

Multiple identical COUNT queries

Additional info

Q	A
Yii version	2.0.51-dev
PHP version	8.1
Operating system	Debian Buster

The problem is in this function: https://github.com/yiisoft/yii2/blob/master/framework/data/BaseDataProvider.php#L165 and related to the changes made in this commit https://github.com/lav45/yii2/commit/bcc499bfaf01124e730b90748eb32d817424a0c1

It is expected to implement caching, but due to a bug/typo it always calls $this->prepareTotalCount().

The correct code has to be the following:

  public function getTotalCount()
    {
        if ($this->_pagination === false) {
            return $this->getCount();
        }
        if ($this->_totalCount !== null) {
            return (int)$this->_totalCount;
        }
        return $this->_totalCount = $this->prepareTotalCount();  // <---
    }

[lav45]

@sleptor

So, the cache definitely does not work, which affects performance notably.

Usually getTotalCount() is called once when creating a paginated navigation. If several identical queries are running in your case, then use caching.

User::getDb()->queryCache = new \yii\caching\ArrayCache();

[sleptor]

@lav45 getTotalCount() is being called multiple times during GridView rendering. It's normal. To avoid duplicated COUNT queries, its result is cached in $this->_totalCount.

However, due to the bug, $this->_totalCount is never initialized, so it's always null.

[sleptor]

@lav45, it's regression. In 2.0.49, the caching logic is correct. $this->_totalCount is initialized when it's null

   public function getTotalCount()
    {
        if ($this->getPagination() === false) {
            return $this->getCount();
        } elseif ($this->_totalCount === null) {
            $this->_totalCount = $this->prepareTotalCount();
        }

        return $this->_totalCount;
    }

[lav45]

This caching method breaks this test https://github.com/yiisoft/yii2/blob/master/tests/framework/data/ActiveDataProviderTest.php#L215

[rob006]

@lav45 This is a new test, which is not even released and changes behavior that existed in framework since the beginning (and probably breaks BC in some cases) - I think it is fine to drop it if it creates any problems.

In general problems started with #20070 (preceded by #20007) and each attempt to fix them created even more problems and regressions. I suggest to revert all changes to state before #20070 or #20007 and start again from there. Messing with pagination and data provider cache looks like a dead end to me.

[sleptor]

@lav45 You added this test, and I think it's wrong because it's artificial. What would a real-life situation be when you need to change the provider's query after a pagination call?

2.0.49 can't pass this test also.

It breaks BC. It ruins performance.

I support the idea of reverting all your changes.

[marcovtwout]

What is needed to solve the immediate issue here? Can we revert all the mensioned changes so we can release 2.0.50.1? 2.0.50 is now very much broken in this regard and has been for a few weeks. It would be good to solve the release first, and then re-address the solution for this ticket.

If I understand correctly that means the following PR's should be reverted:

• https://github.com/yiisoft/yii2/pull/20007
• https://github.com/yiisoft/yii2/pull/20070
• https://github.com/yiisoft/yii2/pull/20176

[sleptor]

@marcovtwout I'm afraid the only immediate solution is to downgrade to 2.0.49.4.

[samdark]

@terabytesoftw is on rolling back the changes.

[My6UoT9]

This surely breaks even more stuff, totalcount is cached at runtime forever.

https://github.com/yiisoft/yii2/pull/20176/files#diff-097176e54cade8343dd0f3a488dc8269b8c5ea275a06abad5060183b4cf252f5R164

[terabytesoftw]

• https://github.com/yiisoft/yii2/pull/20206

I think it should be added.

[terabytesoftw]

I also think that testing needs to be improved.

[terabytesoftw]

@lav45 This is a new test, which is not even released and changes behavior that existed in framework since the beginning (and probably breaks BC in some cases) - I think it is fine to drop it if it creates any problems.

In general problems started with #20070 (preceded by #20007) and each attempt to fix them created even more problems and regressions. I suggest to revert all changes to state before #20070 or #20007 and start again from there. Messing with pagination and data provider cache looks like a dead end to me.

I think we should also revert this PR: https://github.com/yiisoft/yii2/issues/20002 @rob006 @samdark @sleptor

[samdark]

Alright.

[terabytesoftw]

All changes were reverted:

• https://github.com/yiisoft/yii2/pull/20226

Thks.

[rob006]

@terabytesoftw It does not look like full revert, there are still changes comparing to 2.0.49:

https://github.com/yiisoft/yii2/blob/54702a8443b12c1e6b6e5ea627cfbd17e09180d9/framework/data/ActiveDataProvider.php#L164-L176

vs

https://github.com/yiisoft/yii2/blob/64a13931cf60ec0677974640f5bdbd340f93ff6f/framework/data/ActiveDataProvider.php#L162-L169

[terabytesoftw]

@rob006 Fixed, thks.

[schmunk42]

Hi guys, sorry for being late to the party :)

JFTR, we were also affected by this bug. Maybe next time we should move such a PR to the 2.2 branch.