Skip to content

fix: Count does not work with MasterKey on Postgres #9220

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.

Sign up for GitHub

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

Closed
wants to merge 9 commits into from
Closed

fix: Count does not work with MasterKey on Postgres #9220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c65278c
fix: Count does not work with MasterKey on Postgres
dplewis Jul 18, 2024
1b1da89
Merge branch 'alpha' into estimated-count-pg
dplewis Jul 18, 2024
976a4ea
Merge branch 'alpha' into estimated-count-pg
dplewis Jul 18, 2024
2e91df5
Merge branch 'alpha' into estimated-count-pg
mtrezza Jul 18, 2024
0cf57da
add missing await
dplewis Jul 18, 2024
5876999
Merge branch 'alpha' into estimated-count-pg
mtrezza Jul 19, 2024
f913b32
Merge branch 'alpha' into estimated-count-pg
mtrezza Jul 20, 2024
bdbff07
move estimated count
dplewis Jul 20, 2024
b62f7e7
handle analyze error
dplewis Jul 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion spec/AudienceRouter.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ describe('AudiencesRouter', () => {
});
});

it_exclude_dbs(['postgres'])('query installations with count = 1', done => {
it('query installations with count = 1 on multiple audiences', done => {
const config = Config.get('test');
const androidAudienceRequest = {
name: 'Android Users',
Expand Down
39 changes: 1 addition & 38 deletions spec/InstallationsRouter.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ describe('InstallationsRouter', () => {
});
});

it_exclude_dbs(['postgres'])('query installations with count = 1', done => {
it('query installations with count = 1 on multiple devices', done => {
const config = Config.get('test');
const androidDeviceRequest = {
installationId: '12345678-abcd-abcd-abcd-123456789abc',
Expand Down Expand Up @@ -166,43 +166,6 @@ describe('InstallationsRouter', () => {
});
});

it_only_db('postgres')('query installations with count = 1', async () => {
const config = Config.get('test');
const androidDeviceRequest = {
installationId: '12345678-abcd-abcd-abcd-123456789abc',
deviceType: 'android',
};
const iosDeviceRequest = {
installationId: '12345678-abcd-abcd-abcd-123456789abd',
deviceType: 'ios',
};
const request = {
config: config,
auth: auth.master(config),
body: {},
query: {
count: 1,
},
info: {},
};

const router = new InstallationsRouter();
await rest.create(config, auth.nobody(config), '_Installation', androidDeviceRequest);
await rest.create(config, auth.nobody(config), '_Installation', iosDeviceRequest);
let res = await router.handleFind(request);
let response = res.response;
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);
});

it_exclude_dbs(['postgres'])('query installations with limit = 0 and count = 1', done => {
const config = Config.get('test');
const androidDeviceRequest = {
Expand Down
9 changes: 9 additions & 0 deletions spec/ParseQuery.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5275,4 +5275,13 @@ describe('Parse.Query testing', () => {
// Validate
expect(result.executionStats).not.toBeUndefined();
});

it('count objects with master key', async () => {
const obj = new Parse.Object('TestObject');
const obj2 = new Parse.Object('TestObject');
await Parse.Object.saveAll([obj, obj2]);
const query = new Parse.Query('TestObject');
const count = await query.count({ useMasterKey: true });
expect(count).toBe(2);
});
});
6 changes: 3 additions & 3 deletions src/Adapters/Storage/Postgres/PostgresStorageAdapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2048,9 +2048,9 @@
if (where.pattern.length > 0 || !estimate) {
qs = `SELECT count(*) FROM $1:name ${wherePattern}`;
} else {
await this.updateEstimatedCount(className);
Copy link
Contributor

@cbaker6 cbaker6 Jul 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

qs = 'SELECT reltuples AS approximate_row_count FROM pg_class WHERE relname = $1';
}

return this._client
.one(qs, values, a => {
if (a.approximate_row_count == null || a.approximate_row_count == -1) {
Expand Down Expand Up @@ -2425,9 +2425,9 @@
return Promise.resolve();
}

// Used for testing purposes
async updateEstimatedCount(className: string) {
return this._client.none('ANALYZE $1:name', [className]);
return this._client.none('ANALYZE $1:name', [className])
.catch((e) => console.error(`Error: Failed to ANALYZE ${className}:`, e));

Check warning on line 2430 in src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

View check run for this annotation

Codecov / codecov/patch

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js#L2430 

Added line #L2430 was not covered by tests
}

async createTransactionalSession(): Promise<any> {
Expand Down
Loading