fix: Count does not work with MasterKey on Postgres #9220
Conversation
Thanks for opening this pull request! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #9220 +/- ##
==========================================
- Coverage 93.75% 93.74% -0.01%
==========================================
Files 184 184
Lines 14675 14677 +2
==========================================
+ Hits 13758 13759 +1
- Misses 917 918 +1 ☔ View full report in Codecov by Sentry. |
|
This PR fixes estimated counts on Postgres, which happens when you useMasterKey with count. |
|
So this counts issue only occurs when using master key? |
| async updateEstimatedCount(className: string) { | ||
| return this._client.none('ANALYZE $1:name', [className]); | ||
| return this._client.none('ANALYZE $1:name', [className]).catch(() => {}); |
There was a problem hiding this comment.
This looks like you are swallowing the error, do you think it should be logged as an error or warning? Especially since this method isn’t just used for tests anymore. This will help track down if the method stops working and can’t update the estimate in the future
There was a problem hiding this comment.
I updated it. Maybe a user doesn't have permission to run ANALYZE
| @@ -2048,9 +2048,9 @@ export class PostgresStorageAdapter implements StorageAdapter { | |||
| if (where.pattern.length > 0 || !estimate) { | |||
| qs = `SELECT count(*) FROM $1:name ${wherePattern}`; | |||
| } else { | |||
| await this.updateEstimatedCount(className); | |||
There was a problem hiding this comment.
If I remember correctly, this else statement usually gets hit by the ParseDashboard and maybe the use of the masterKey. Since the dashboard heavily relies on count, do you anticipate a noticeable performance hit whenever a database manager (or multiple managers) are logged into the Dashboard?
There was a problem hiding this comment.
Since the dashboard heavily relies on count
The dashboard relies heavily on count on the initial browser load. I've never personally used ANALYZE on a large dataset before so I'm not sure of the performance.
For the dashboard, I think we can add some documentation to recommend users to enable autovalcuum and its threshold if they are using Postgres.
There was a problem hiding this comment.
Continuing from #9220 (comment).
The question of the perf impact still remains. If an app is currently calling count many times, even though that may not be not efficient, we run ANALYZE more often than needed. We may even need to consider this a breaking change, due to its possible performance impact.
More generally, I think that queries and DB maintenance are two different realms and therefore I'm still not sure we should fuse them together like this.
I'm beginning to suspect that the solution to this is to advise developers to properly set up and maintain the DB, maybe add a docs mention regarding auto vacuum.
I agree |
|
@dplewis if there is any other change in this PR that should be merged, please feel free to re-open. |
Pull Request
Issue
Count operations are expensive so for Postgres we use estimated counts. The method to updated these estimated counts is called valcuuming which can happen at any time. If it doesn't happen you end up with 0 for your count. This has been a problem specifically for the Parse Dashboard.
https://www.postgresql.org/docs/current/routine-vacuuming.html
Closes: #8286
Closes: #8502
Approach
Tasks