Skip to content
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

chore: Optimize postgres - prepared statements in select #2182

Merged
merged 8 commits into from
Nov 7, 2023

Conversation

Ivansete-status
Copy link
Collaborator

Description

The prepared statements is a mechanism that allows saving time for common queries.

We are applying that approach for the "select" queries, i.e. queries that come from the Store protocol requests. Notice that we were already applying this in "insert" operations.

With that, we enhance the performance of "select" queries. However, bear in mind that there is an underlying time consumption when performing concurrent queries. I've added a long comment about that in dbconn.nim.

Changes

  • Use of prepared statements in select queries.

Issue

#1842

Copy link

github-actions bot commented Nov 3, 2023

This PR may contain changes to database schema of one of the drivers.

If you are introducing any changes to the schema, make sure the upgrade from the latest release to this change passes without any errors/issues.

Please make sure the label release-notes is added to make sure upgrade instructions properly highlight this change.

Copy link

github-actions bot commented Nov 3, 2023

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2182

Built from 22d53a8

@Ivansete-status Ivansete-status self-assigned this Nov 3, 2023
@Ivansete-status Ivansete-status changed the title chore: Optimize postgres chore: Optimize postgres - prepared statements in select Nov 3, 2023
@Ivansete-status Ivansete-status mentioned this pull request Nov 3, 2023
8 tasks
@Ivansete-status Ivansete-status marked this pull request as ready for review November 6, 2023 06:57
Copy link
Contributor

@SionoiS SionoiS left a comment

Choose a reason for hiding this comment

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

LGTM

Minor style comments.

waku/common/databases/db_postgres/dbconn.nim Show resolved Hide resolved
waku/common/databases/db_postgres/dbconn.nim Outdated Show resolved Hide resolved
waku/common/databases/db_postgres/dbconn.nim Outdated Show resolved Hide resolved
waku/common/databases/db_postgres/dbconn.nim Outdated Show resolved Hide resolved
waku/common/databases/db_postgres/dbconn.nim Outdated Show resolved Hide resolved
waku/common/databases/db_postgres/pgasyncpool.nim Outdated Show resolved Hide resolved
waku/common/databases/db_postgres/pgasyncpool.nim Outdated Show resolved Hide resolved
waku/common/databases/db_postgres/pgasyncpool.nim Outdated Show resolved Hide resolved
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

LGTM! :)

Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

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

All good! Special thanks that this approach prevents sql injection attacks.

## but, on the other hand, the connection remains in "db.pqisBusy() == 1" for 100ms more.
## I think this is because `nwaku` is single-threaded and it has to handle many connections (20)
## simultaneously. Therefore, there is an underlying resource sharing (cpu) that makes this
## to happen. Notice that the _Postgres_ database spawns one process per each connection.
Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate this great, explanatory comment here!!!


return ok(rows)

method getMessages*(s: PostgresDriver,
Copy link
Contributor

Choose a reason for hiding this comment

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

How come PostgresDriver reference named as "s"? :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the comments! I'll review that in upcoming refactoring PRs :)

@Ivansete-status Ivansete-status merged commit 6da1aee into master Nov 7, 2023
9 of 10 checks passed
@Ivansete-status Ivansete-status deleted the optimize-postgres branch November 7, 2023 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants