Skip to content

Count Objects not working in some classes #8502

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
4 tasks done
MrMartinR opened this issue Jun 4, 2022 · 25 comments · Fixed by #8511
Open
4 tasks done

Count Objects not working in some classes #8502

MrMartinR opened this issue Jun 4, 2022 · 25 comments · Fixed by #8511
Labels
bounty:$10 Bounty applies for fixing this issue (Parse Bounty Program) state:released Released as stable version state:released-alpha Released as alpha version state:released-beta Released as beta version type:bug Impaired feature or lacking behavior that is likely assumed

Comments

@MrMartinR
Copy link

MrMartinR commented Jun 4, 2022

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

  • Parse Dashboard version: 4.1.1
  • Browser: Safari, Safari Technology Preview Release, Firefox, Chrome
  • Browser version: Safari 15.5, Safari Technology Preview Release 146, Firefox 100.0.2, Chrome 102.0.5005.61.

Server

  • Parse Server version: 5.2.1
  • Operating system: macOS Monterrey
  • Local or remote host (AWS, Azure, Google Cloud, Heroku, Digital Ocean, etc): AWS Ubuntu EC2

Database

  • System (MongoDB or Postgres): Postgres
  • Database version: 14.2
  • Local or remote host (MongoDB Atlas, mLab, AWS, Azure, Google Cloud, etc): AWS

Logs

Screen Shot 2022-06-04 at 19 15 17

@parse-github-assistant
Copy link

parse-github-assistant bot commented Jun 4, 2022

Thanks for opening this issue!

  • 🚀 You can help us to fix this issue faster by opening a pull request with a failing test. See our Contribution Guide for how to make a pull request, or read our New Contributor's Guide if this is your first time contributing.

@mtrezza mtrezza added the type:bug Impaired feature or lacking behavior that is likely assumed label Jun 4, 2022
@mtrezza
Copy link
Member

mtrezza commented Jun 4, 2022

Probably related: parse-community/parse-dashboard#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.

@mtrezza mtrezza added the bounty:$10 Bounty applies for fixing this issue (Parse Bounty Program) label Jun 7, 2022
@patelmilanun
Copy link
Member

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

@patelmilanun
Copy link
Member

patelmilanun commented Apr 5, 2023

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
Copy link
Member

mtrezza commented Apr 9, 2023

Would this be a Parse Server or Parse Dashboard PR?

@patelmilanun
Copy link
Member

Parse server it is

@parse-github-assistant

This comment was marked as outdated.

@mtrezza mtrezza transferred this issue from parse-community/parse-dashboard Apr 10, 2023
@parse-github-assistant

This comment was marked as outdated.

@mtrezza
Copy link
Member

mtrezza commented Apr 10, 2023

Moving the issue to Parse Server.

@parseplatformorg
Copy link
Contributor

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

@mtrezza
Copy link
Member

mtrezza commented May 28, 2023

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 #8588. @patelmilanun could you try again?

@patelmilanun
Copy link
Member

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
Copy link
Member

mtrezza commented May 29, 2023

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
Copy link
Contributor

cbaker6 commented May 29, 2023

@patelmilanun I made the request in #8586 (comment) to revert your PR due to it not passing the test suite. I recommend reading #8586 (comment) 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
Copy link
Member

mtrezza commented May 29, 2023

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
Copy link
Member

patelmilanun commented May 29, 2023

@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
Copy link
Contributor

cbaker6 commented May 29, 2023

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.

async count(
className: string,
schema: SchemaType,
query: QueryType,
readPreference?: string,
estimate?: boolean = true
) {
debug('count');
const values = [className];
const where = buildWhereClause({
schema,
query,
index: 2,
caseInsensitive: false,
});
values.push(...where.values);
const wherePattern = where.pattern.length > 0 ? `WHERE ${where.pattern}` : '';
let qs = '';
if (where.pattern.length > 0 || !estimate) {
qs = `SELECT count(*) FROM $1:name ${wherePattern}`;
} else {
qs = 'SELECT reltuples AS approximate_row_count FROM pg_class WHERE relname = $1';
}

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
Copy link
Contributor

cbaker6 commented May 29, 2023

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):

expect(response.results.length).toEqual(2);
expect(response.count).toEqual(0); // estimate count is zero
const pgAdapter = config.database.adapter;
await pgAdapter.updateEstimatedCount('_Installation');
res = await router.handleFind(request);
response = res.response;
expect(response.results.length).toEqual(2);
expect(response.count).toEqual(2);

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
Copy link
Member

mtrezza commented May 29, 2023

@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
Copy link
Member

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
Copy link
Member

mtrezza commented May 30, 2023

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

@patelmilanun
Copy link
Member

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
Copy link
Contributor

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

@parseplatformorg parseplatformorg added the state:released-beta Released as beta version label Jun 10, 2023
@parseplatformorg
Copy link
Contributor

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

@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 6.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bounty:$10 Bounty applies for fixing this issue (Parse Bounty Program) state:released Released as stable version state:released-alpha Released as alpha version state:released-beta Released as beta version type:bug Impaired feature or lacking behavior that is likely assumed
Projects
None yet
5 participants