feat(postgres): Adding a postgres async pool to make the db interactions asynchronous #1779

Ivansete-status · 2023-06-02T12:54:38Z

Summary

Allowing asynchronous calls

Further details

This PR:

continues the postgres asynchronous adoption started by @LNSD in feat(common): added postgress async pool wrapper #1631. I submit a different PR in order to adapt it better to the current code state and respect the Lorenzo's PR.
makes the postgres asynchronous but is not yet integrated in the wakunode app. So far is only used by the tests.
pending to implement the enhancement mentioned by @Menduist - feat(common): added postgress async pool wrapper #1631 (comment)

How to test

Start postgres database locally

sudo docker compose -f tests/docker-compose.yml up

Run tests locally

. env.sh

nim c -r -d:chronicles_log_level=WARN --verbosity=0 --hints=off --outdir=build ./tests/v2/waku_archive/test_driver_postgres.nim

Issue

#1604

…iver/'

vpavlin · 2023-06-02T14:45:43Z

waku/v2/waku_archive/driver/postgres_driver/asyncpool.nim

+  # blocking the async runtime
+  if pool.conns.anyIt(it.busy):
+    while pool.conns.anyIt(it.busy):
+      await sleepAsync(0.milliseconds)


Why is the non-sleeping sleep here?

Or maybe a better question - what does it do?

Hi @vpavlin ! The main purpose of this await is to bring the Nim's asynchornous runtime the opportunity to attend other futures that are being "awaited". However, we might need to follow a better approach in further PR's (this comment is considered as a ToDo in the offending issue:)

#1631 (comment)

rymnc

Great PR, left a few comments for style and couple queries, thanks :)

rymnc · 2023-06-03T07:27:15Z

tests/v2/waku_archive/test_driver_postgres.nim

-    require:
-      driverRes.isOk()
+    let driver = driverRes.value
+    let _ = await driver.reset()


Suggested change

let _ = await driver.reset()

discard await driver.reset()

maybe?

rymnc · 2023-06-03T07:29:28Z

tests/v2/waku_archive/test_driver_postgres.nim


-    require:
-      driverRes.isOk()
+    let driver = driverRes.value


Suggested change

let driver = driverRes.value

let driver = driverRes.get()

rymnc · 2023-06-03T07:31:51Z

tests/v2/waku_archive/test_driver_postgres.nim

-    require:
-      not driver.isNil()
+    let initRes = await driver.init()
+    assert initRes.isOk(), initRes.error


Maybe we should follow this pattern in other test files too so we can see why the result is an error!
non-blocker

Yes, I'll set a separate PR asap with this

Morning @rymnc ! After checking the code to apply this change I see that the change would be quite wide but we won't get very great investment return by doing so, as the tests don't tend to fail very often.

Therefore, I think is better to just apply this technique from now on in the upcoming tests.

Of course, if you consider it worth the effort, then I will make it for the current tests :)

rymnc · 2023-06-03T07:34:00Z

waku/v2/waku_archive/driver/postgres_driver/asyncpool.nim

+  chronos
+import
+  ../../driver,
+  connection


should this import be with the std lib import group?

Actually this refers to a module set in the same folder. I'll make it more explicit with ./connection

rymnc · 2023-06-03T07:36:16Z

waku/v2/waku_archive/driver/postgres_driver/asyncpool.nim

+  let conn = pool.conns[connIndexRes.value].dbConn
+  defer: pool.releaseConn(conn)
+
+  return await conn.rows(sql(query), args)


Maybe we should try-except here to bubble the error as a result instead of a failed future?

rymnc · 2023-06-03T07:37:03Z

waku/v2/waku_archive/driver/postgres_driver/asyncpool.nim

+    # Each session/connection should have its own prepared statements.
+    try:
+      pool.conns[connIndexRes.value].insertStmt =
+                                    conn.prepare("insertRow", sql(baseStmt), 7)


I'm probably missing something here, what is 7?

7 is the number of parameters to be handled.
In this case, we are only setting an unique "insertRow" prepared statement and the baseStmt has the next value:

INSERT INTO messages (id, storedAt, contentTopic, payload, pubsubTopic, version, timestamp) VALUES ($1, $2, $3, $4, $5, $6, $7);

The concept of "prepared statement" is interesting from a performance point of view.

I see! thanks, its much clearer now
Can we make this value a constant near the "prepared statement", in case the value changes in the future?

rymnc · 2023-06-03T07:37:29Z

waku/v2/waku_archive/driver/postgres_driver/connection.nim

+proc open*(connString: string):
+           Result[DbConn, string] =
+  ## Opens a new connection.
+  var conn:DbConn = nil


Suggested change

var conn:DbConn = nil

var conn: DbConn = nil

rymnc · 2023-06-03T07:39:21Z

waku/v2/waku_archive/driver/postgres_driver/postgres_driver.nim

-      return err("failed to reset database")
-  except DbError:
-    return err("failed to reset database")
+  let ret = await s.connPool.exec(createTableQuery(), newSeq[string](0))


try-except here and bubble the error within the result?

rymnc · 2023-06-03T07:41:27Z

waku/v2/waku_archive/driver/postgres_driver/postgres_driver.nim

@@ -282,9 +280,9 @@ proc sleep*(s: PostgresDriver, seconds: int):
  # database for the amount of seconds given as a parameter.
  try:
    let params = @[$seconds]
-    s.connection.exec(sql"SELECT pg_sleep(?)", params)
+    let _ = await s.connPool.query("SELECT pg_sleep(?)", params)


Suggested change

let _ = await s.connPool.query("SELECT pg_sleep(?)", params)

asyncCheck await s.connPool.query("SELECT pg_sleep(?)", params)

https://forum.nim-lang.org/t/5279#33208

discard is OK for tests imo (in some situations)

Thanks for the recommendation! However, the test got stuck with that.
Therefore, in the end I'll suggest sth like:

let sleepRes = await s.connPool.query("SELECT pg_sleep(?)", params) if sleepRes.isErr(): return err("error in postgres_driver sleep: " & sleepRes.error)

…e location

jm-clius

LGTM and thanks for taking this over!

rymnc

LGTM, awesome PR!

Lorenzo Delgado and others added 5 commits May 31, 2023 12:50

feat(common): added postgress async pool wrapper

75e8f0b

Simplification of the code a little and starting new bare unit tests

5c8a19d

Moving all postgres logic to 'waku/v2/waku_archive/driver/postgres_dr…

e7c4e60

…iver/'

Integrating postgres asynchronous calls into the current implementation

3a6b4d9

better use 'proc' instead of 'method'

6f3c9f6

Ivansete-status marked this pull request as ready for review June 2, 2023 13:36

Ivansete-status requested review from alrevuelta, jm-clius, vpavlin, Menduist and rymnc June 2, 2023 13:36

Ivansete-status mentioned this pull request Jun 2, 2023

feat(common): added postgress async pool wrapper #1631

Closed

Ivansete-status self-assigned this Jun 2, 2023

vpavlin reviewed Jun 2, 2023

View reviewed changes

rymnc reviewed Jun 3, 2023

View reviewed changes

Ivansete-status added 5 commits June 6, 2023 09:53

test_driver_postgres.nim: use of future 'discard' instead of 'let _'

92f9af2

asyncpool.nim: Make import more explicit to indicate the actual modul…

9df5b93

…e location

asyncpool.nim: making explicit num params in prepared statement

d2884a2

postgres_driver/connection: better spacing style

0b4b13f

postgres_driver.nim, asyncpool.nim: better error handling

2f52cbf

Ivansete-status requested review from rymnc and vpavlin June 6, 2023 08:40

jm-clius approved these changes Jun 6, 2023

View reviewed changes

alrevuelta approved these changes Jun 7, 2023

View reviewed changes

vpavlin approved these changes Jun 7, 2023

View reviewed changes

rymnc approved these changes Jun 7, 2023

View reviewed changes

Ivansete-status merged commit cb2e3d8 into master Jun 7, 2023
13 checks passed

Ivansete-status deleted the feat-1604-pgconn-async-pool branch June 7, 2023 08:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(postgres): Adding a postgres async pool to make the db interactions asynchronous #1779

feat(postgres): Adding a postgres async pool to make the db interactions asynchronous #1779

Ivansete-status commented Jun 2, 2023

vpavlin Jun 2, 2023

vpavlin Jun 2, 2023

Ivansete-status Jun 5, 2023

rymnc left a comment

rymnc Jun 3, 2023

rymnc Jun 3, 2023

rymnc Jun 3, 2023

Ivansete-status Jun 5, 2023

Ivansete-status Jun 7, 2023

rymnc Jun 3, 2023

Ivansete-status Jun 5, 2023

rymnc Jun 3, 2023

rymnc Jun 3, 2023

Ivansete-status Jun 5, 2023

rymnc Jun 5, 2023

rymnc Jun 3, 2023

rymnc Jun 3, 2023

rymnc Jun 3, 2023

rymnc Jun 3, 2023

Ivansete-status Jun 6, 2023

jm-clius left a comment

rymnc left a comment

	let _ = await s.connPool.query("SELECT pg_sleep(?)", params)
	asyncCheck await s.connPool.query("SELECT pg_sleep(?)", params)

feat(postgres): Adding a postgres async pool to make the db interactions asynchronous #1779

feat(postgres): Adding a postgres async pool to make the db interactions asynchronous #1779

Conversation

Ivansete-status commented Jun 2, 2023

Summary

Further details

How to test

Issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rymnc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jm-clius left a comment

Choose a reason for hiding this comment

rymnc left a comment

Choose a reason for hiding this comment