search

parse-community / parse-server

Parse Server for Node.js / Express
https://parseplatform.org
Apache License 2.0
20.81k stars 4.77k forks source link

Count Objects not working in some classes #8502

Open MrMartinR opened 2 years ago

MrMartinR commented 2 years ago

New Issue Checklist

Issue Description

Dashboard doesn't count object in most of the classes

Steps to reproduce

Since the first installation I always had this issue...

Actual Outcome

Only showing '2' in the _Session class, the other classes show '0'

Expected Outcome

To show the total of objects in each class

Environment

Parse dashboard hosted on AWS Ubuntu EC2

Dashboard

Server

Database

Logs

Screen Shot 2022-06-04 at 19 15 17
parse-github-assistant[bot] commented 2 years ago

Thanks for opening this issue!

parse-github-assistant[bot] commented 1 year ago

Thanks for opening this issue!

mtrezza commented 2 years ago

Probably related: https://github.com/parse-community/parse-dashboard/issues/1637

The issue above was reported to only happen on Postgres, not on MongoDB; same as this new issue. May be DB specific, and therefore possibly a Parse Server issue.

patelmilanun commented 1 year ago

it indeed is parse server issue as the count is coming from parse server only.

patelmilanun commented 1 year ago

i found the solution should i raise MR

it can be done using qs = `SELECT n_live_tup AS approximate_row_count FROM pg_stat_all_tables WHERE relname = $1; instead of qs = 'SELECT reltuples AS approximate_row_count FROM pg_class WHERE relname = $1'; when doing count inside parse-server\src\Adapters\Storage\Postgres\PostgresStorageAdapter.js

mtrezza commented 1 year ago

Would this be a Parse Server or Parse Dashboard PR?

patelmilanun commented 1 year ago

Parse server it is

parse-github-assistant[bot] commented 1 year ago

Thanks for opening this issue!

mtrezza commented 1 year ago

Moving the issue to Parse Server.

parseplatformorg commented 1 year ago

🎉 This change has been released in version 6.1.0-alpha.15

mtrezza commented 1 year ago

Reopening, the fix caused other issues, see failing CI: https://github.com/parse-community/parse-server/actions/runs/5105446713/jobs/9177489496?pr=8586. Fix has been reverted with https://github.com/parse-community/parse-server/pull/8588. @patelmilanun could you try again?

patelmilanun commented 1 year ago

Can you please help me on this. Based on what I see, I can conclude that my changes are not working for other versions of PostgreSQL like 11 to 14 with different PostGIS and only work with PostgreSQL version 15 with PostGIS 3.3.

So is that mean my code isn't backward compatible and I need a workaround for older versions.

mtrezza commented 1 year ago

We have all the postres tests in the CI so you can see for which versions it's failing in a PR. You could start the PR with adding a tests that demonstrates that the count is incorrect, to see which versions need a fix. Then you could apply a fix depending on the version.

cbaker6 commented 1 year ago

@patelmilanun I made the request in https://github.com/parse-community/parse-server/pull/8586#issuecomment-1566205298 to revert your PR due to it not passing the test suite. I recommend reading https://github.com/parse-community/parse-server/pull/8586#issuecomment-1566221232 as it has more insight about why the count is only an estimate and is not intended to be exact. The comment also points to the broken test case if you are attempting to look at it in the future.

So is that mean my code isn't backward compatible and I need a workaround for older versions.

It's possible your changes are not backwards compatible. If your version of an estimate count is not available in older versions of Postgres, IMO, it may not be worth the workaround since both are only estimates. Of course, you can look into ways to detect the postgres version and use your code over the original way if you believe your estimate is always more accurate in newer versions of postgres.

mtrezza commented 1 year ago

Only showing '2' in the _Session class, the other classes show '0'

The original issue was that classes show a count of zero. If that is due to being an estimate we can change this from "bug" to "feature request". If there is any improvement possible (such as exact count if table doesn't have many rows, and only estimate if it has many rows), then we can keep this open, otherwise we can close as designed. In any case, a count of zero while there are 4 rows looks rather unexpected.

patelmilanun commented 1 year ago

@cbaker6 @mtrezza My changes are related to the count being wrong so how it's affecting the test case line expect(response.results.length).toEqual(2);. I mean we are counting length of result and not checking the count. I mean if it's failing at line expect(response.count).toEqual(0); than I get it as it's the actual line checking for count right?

image

Apart from this I followed this guide to test it https://github.com/parse-community/parse-server/blob/alpha/CONTRIBUTING.md#postgres-with-docker which starts a docker with PostgreSQL 13.8 and PostGIS 3.2.2 and it's not failing for me.

cbaker6 commented 1 year ago

The original issue was that classes show a count of zero. If that is due to being an estimate we can change this from "bug" to "feature request".

A feature request makes sense. The count method defaults to estimating most likely due to a count being expensive, so a good design decision IMO. The dashboard uses the estimate. When a direct query asks for count it uses the expensive count. https://github.com/parse-community/parse-server/blob/505dd6bcfe2ce787a85d380b60b0d4dc5656fea1/src/Adapters/Storage/Postgres/PostgresStorageAdapter.js#L2020-L2044

In any case, a count of zero while there are 4 rows looks rather unexpected.

Agreed... I do believe an estimate is okay as most deployed apps will have multiple classes/tables with multiple rows. The dashboard shouldn't cause an unnecessary burden on the DB. I've seen some posts about count being expensive in MongoDB as well, but haven't looked into how the parse mongo adapter is handling this. IMO the display of the amount of rows is merely a convenience and shouldn't be depended on for an exact number. It should probably be noted somewhere in the docs so postgres users who also use the Parse Dashboard don't think it's a bug.

cbaker6 commented 1 year ago

I mean if it's failing at line expect(response.count).toEqual(0); than I get it as it's the actual line checking for count right?

@patelmilanun there are multiple tests in the same function. You should verify which one is failing, my guess is it's line 203, not the one you pointed to, but I'm guessing. If you feel your adjustment improved the results, you still have to update the test cases and justify your change of the test (this would seem odd to me because you mentioned it passes for Postgres 15, but not earlier versions): https://github.com/parse-community/parse-server/blob/6c5f89a56bf21609818b8aac7c55137101f8c62f/spec/InstallationsRouter.spec.js#L194-L203

One thing to remember is the CI is there to test all supported versions as developers use many different versions in their active deployments. Just because it passes one (or locally on your system), doesn't mean it will pass all others in the CI.

mtrezza commented 1 year ago

@patelmilanun You may want to look online for possible solutions, row counting being a common issue, see for example https://stackoverflow.com/questions/7943233/fast-way-to-discover-the-row-count-of-a-table-in-postgresql.

Off the top of my head, a possible solution could be to get the estimate first, and if the estimate is a low number (like <1k), then do an exact count. That would require 2 DB requests, but if this is the only solution due to the design limitations of the DB then it could be an option in Parse Server. I could imagine that estimated vs. exact counting is something that is also an interesting feature for normal queries.

patelmilanun commented 1 year ago

But I had the same PostgreSQL as well as PostGIS version as the pipeline which is failing does have. But for me its running perfectly. I don't know how to reproduce it and as such I can't even propose a solution.

mtrezza commented 1 year ago

Did you run the full tests (/spec) locally?

patelmilanun commented 1 year ago

No I didn't but after ur reply I did and i'm attaching the result of PARSE_SERVER_TEST_DB=postgres PARSE_SERVER_TEST_DATABASE_URI=postgres://postgres:password@localhost:5432/parse_server_postgres_adapter_test_database npm run coverage while there was a docker container running with same version as failed versions which was started using docker run -d --name parse-postgres -p 5432:5432 -e POSTGRES_PASSWORD=password --rm postgis/postgis:11-3.0-alpine && sleep 20 && docker exec -it parse-postgres psql -U postgres -c 'CREATE DATABASE parse_server_postgres_adapter_test_database;' && docker exec -it parse-postgres psql -U postgres -c 'CREATE EXTENSION pgcrypto; CREATE EXTENSION postgis;' -d parse_server_postgres_adapter_test_database && docker exec -it parse-postgres psql -U postgres -c 'CREATE EXTENSION postgis_topology;' -d parse_server_postgres_adapter_test_database.

U can see that there wasn't any case which are failing for me which are failing for CI with the same exact version.

test coverage log.txt

parseplatformorg commented 1 year ago

🎉 This change has been released in version 6.3.0-beta.1

parseplatformorg commented 1 year ago

🎉 This change has been released in version 6.3.0-alpha.1

parseplatformorg commented 11 months ago

🎉 This change has been released in version 6.3.0