feat(postgresql): align Andrea's PR#1590 changes into master #1764

Ivansete-status · 2023-05-26T16:10:38Z

Summary

This PR is aimed to align the great progress made by @cammellos in terms of PostgreSQL adoption, into our master branch. With this, we add a postres driver that allows to interact with a PostgreSQL database.

Details

The important changes were introduced in the next PR.
#1590

Apart from the adaptation I add a tiny test aimed to explicitly test the asynchronicity, which should work once we add the Lorenzo's changes.

My idea was to align Andrea's branch first and then work on the PR#1590 but I rebased it from origin/master as ususal so I need to merge into master. Sorry for that Andrea.

How to test it

Start a PostgreSQL instance by mean of the included docker compose file
```
sudo docker compose -f tests/docker-compose.yml up
```

Run the postgresql tests:

. env.sh

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

Pending tasks

Extend the postgresql driver to cover all the "archive" features.
Include @LNSD changes, feat(common): added postgress async pool wrapper #1631, so that we can make asynchronous calls to the postgres driver.
Allow the postgres unit tests to run without a running local PostgreSQL database.
Move the included docker compose file to a more appropriate location.

Issue

#1604

…-ness

…uture

jm-clius

Afaict this looks good. I imagine a tricky step will be integrating this with configuration and the rest of the node :)

jm-clius · 2023-05-29T15:37:23Z

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

+method sleep*(s: PostgresDriver, seconds: int):
+              Future[ArchiveDriverResult[void]] {.base, async.} =


If this is not overridden/inherited anywhere else, it should not be annotated base and be a proc rather than a method.

vpavlin · 2023-05-30T06:19:23Z

tests/all_tests_v2.nim

@@ -19,6 +19,10 @@ import
  ./v2/waku_archive/test_retention_policy,
  ./v2/waku_archive/test_waku_archive

+# TODO: add the postgres test when we can mock the database.


In case you wanted to have tests enabled in CI, you could just exec a docker run command from tests to setup DB prior execution the tests themselves - it should work fine in CI

Hey @vpavlin ! In the end I will tackle that in a separate PR because I need to tweak it in a way that the postgres tests are only run on a Linux platform. This is because the GH Actions only allow starting a docker container within Linux runners.
Thanks for the hint !

vpavlin · 2023-05-30T06:30:04Z

waku/v2/waku_archive/driver/postgres_driver.nim

+
+import ./postgres_driver/postgres_driver
+
+export postgres_driver


missing new line:) Would you like to add it?

alrevuelta

lgtm! thanks! left a comment that might be important, let me know otherwise :)

alrevuelta · 2023-05-30T06:32:46Z

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

+  ");"
+
+proc insertRow(): string =
+ """INSERT INTO messages (id, storedAt, contentTopic, payload, pubsubTopic,


will this insert work if we try to insert a duplicated message? I mean, insert a message twice. Perhaps we need ON CONFLICT?

mind adding a testcase if its the case?

Thanks for the comment @alrevuelta
I think it should be added correctly as the primary key would be different, given different timestamps (nanosecconds precision)
The expected behavior is that the duplicate messages are properly added right ?

I think this is a good point. True duplicate messages (i.e. the exact same message from the same source with same timestamp) should not be inserted twice. I think it will correctly fail because of the resulting primary key clash, but since we plan to have multiple store nodes all write to the same (shared) postgresql instance many duplicates will be expected. May be worth understanding how reliably (and quickly) this "expected failure on duplicate" works.

In case of completely equal messages, the put call will return an error. I will add a little test with the next:

putRes = await driver.put(DefaultPubsubTopic, msg2, computeDigest(msg2), msg2.timestamp) ## Then require: not putRes.isOk()

vpavlin · 2023-05-30T06:36:15Z

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

+  try:
+    let res = s.connection.tryExec(sql(createTableQuery()))
+    if not res:
+      return err("failed to migrate database")


would an error saying failed to initialize make more sense here? Or is this a prep for adding migrations in the future?

You are absolutely right! I'll set a that more meaningful error message straightaway.

vpavlin · 2023-05-30T06:37:36Z

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

+  for r in rows:
+    let rowRes = extractRow(r)
+    if rowRes.isErr():
+      return err("failed to extract row")


Will this also print/pass the actual error from extractRow?

Good catch! I'll add it :)

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

vpavlin

Few questions/comments mainly to feed my curiosity:) Otherwise, LGTM!

rymnc · 2023-05-30T08:35:37Z

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

+proc dropTableQuery(): string =
+  "DROP TABLE messages"
+
+proc createTableQuery(): string =


Curious if we can manage migrations in plain SQL, so that other implementations can use the same migrations (not a blocker, just want to know your thoughts)

Good point @rymnc ! I'll set that as a pending action item in the issue

cammellos

Code looks good to me, I won't approve since I have co-written it, thank you @Ivansete-status for taking care of it!!

Ivansete-status · 2023-05-31T08:41:49Z

Special kudos to @cammellos for submiting the original PR!
Btw Andrea, kindly close your original PR, if you consider it appropriate, as we included your changes in that one.

If you consider that we are missing any important change we can create a separate issue to tackle it.
Thanks!

cammellos and others added 5 commits May 26, 2023 15:40

Add postgres basic support

1b5397f

[WIP] Address feedback

19e7172

test_driver_postgres.nim: adding simple test to validate asynchronous…

272e913

…-ness

Aligning the changes made by Andrea to the current master state

5e494d6

Disabling postgresql tests temporarily

2a56ba4

Ivansete-status changed the title ~~Feature/postgres align to master~~ feat(postgresql) align Andrea's PR#1590 changes into to master May 26, 2023

Ivansete-status changed the title ~~feat(postgresql) align Andrea's PR#1590 changes into to master~~ feat(postgresql) align Andrea's PR-1590 changes into to master May 26, 2023

Deleting file which was part of a refactoring. We'll do that in the f…

74f636f

…uture

Ivansete-status changed the title ~~feat(postgresql) align Andrea's PR-1590 changes into to master~~ feat(postgresql): align Andrea's PR#-1590 changes into to master May 26, 2023

Ivansete-status changed the title ~~feat(postgresql): align Andrea's PR#-1590 changes into to master~~ feat(postgresql): align Andrea's PR#1590 changes into to master May 26, 2023

Deleting file and unused lines in waku_node.nim

b773d6e

Ivansete-status marked this pull request as ready for review May 26, 2023 17:27

Ivansete-status requested review from cammellos, vpavlin, alrevuelta and jm-clius May 26, 2023 17:27

Ivansete-status changed the title ~~feat(postgresql): align Andrea's PR#1590 changes into to master~~ feat(postgresql): align Andrea's PR#1590 changes into master May 26, 2023

jm-clius approved these changes May 29, 2023

View reviewed changes

vpavlin reviewed May 30, 2023

View reviewed changes

alrevuelta approved these changes May 30, 2023

View reviewed changes

vpavlin reviewed May 30, 2023

View reviewed changes

waku/v2/waku_archive/driver/postgres_driver/postgres_driver.nim Show resolved Hide resolved

vpavlin approved these changes May 30, 2023

View reviewed changes

Ivansete-status self-assigned this May 30, 2023

driver/postres_driver.nim: adding new empty line at the end of the file

cead9a8

rymnc reviewed May 30, 2023

View reviewed changes

cammellos reviewed May 30, 2023

View reviewed changes

Ivansete-status mentioned this pull request May 30, 2023

chore(archive): allow request concurrency towards PostgreSQL #1604

Closed

5 tasks

Ivansete-status added 2 commits May 30, 2023 13:09

postgres_driver/postgres_driver.nim: method sleep -> proc sleep

4483549

postgres_driver/postgres_driver.nim: standardizing how to return values

2a5960c

Ivansete-status force-pushed the feature/postgres-align-to-master branch 2 times, most recently from 90b5549 to 3bc28fb Compare May 31, 2023 07:41

test_driver_postgres.nim: new unit test (checking duplicate insertion)

8e4be4e

Ivansete-status force-pushed the feature/postgres-align-to-master branch from 3bc28fb to 8e4be4e Compare May 31, 2023 07:47

Ivansete-status merged commit 7df6f4c into master May 31, 2023

Ivansete-status deleted the feature/postgres-align-to-master branch May 31, 2023 08:28

Ivansete-status mentioned this pull request Jun 2, 2023

Add postgres basic support #1590

Closed

Ivansete-status mentioned this pull request Jul 5, 2023

chore(postgres): keep the Postgres queries in separate raw files. #1841

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(postgresql): align Andrea's PR#1590 changes into master #1764

feat(postgresql): align Andrea's PR#1590 changes into master #1764

Ivansete-status commented May 26, 2023 •

edited

Loading

jm-clius left a comment

jm-clius May 29, 2023

vpavlin May 30, 2023

Ivansete-status May 31, 2023 •

edited

Loading

vpavlin May 30, 2023

alrevuelta left a comment

alrevuelta May 30, 2023

Ivansete-status May 30, 2023

jm-clius May 30, 2023

Ivansete-status May 30, 2023

vpavlin May 30, 2023

Ivansete-status May 30, 2023

vpavlin May 30, 2023

Ivansete-status May 30, 2023

vpavlin left a comment

rymnc May 30, 2023

Ivansete-status May 30, 2023

cammellos left a comment

Ivansete-status commented May 31, 2023 •

edited

Loading

		method sleep*(s: PostgresDriver, seconds: int):
		Future[ArchiveDriverResult[void]] {.base, async.} =


		import ./postgres_driver/postgres_driver

		export postgres_driver

feat(postgresql): align Andrea's PR#1590 changes into master #1764

feat(postgresql): align Andrea's PR#1590 changes into master #1764

Conversation

Ivansete-status commented May 26, 2023 • edited Loading

Summary

Details

How to test it

Pending tasks

Issue

jm-clius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivansete-status May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrevuelta 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

vpavlin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cammellos left a comment

Choose a reason for hiding this comment

Ivansete-status commented May 31, 2023 • edited Loading

Ivansete-status commented May 26, 2023 •

edited

Loading

Ivansete-status May 31, 2023 •

edited

Loading

Ivansete-status commented May 31, 2023 •

edited

Loading