feat(postgres): integration of postgres in wakunode2

[Ivansete-status]

Summary

This PR is a combination of:

1. Integration of Postgres in wakunode2.
2. refactoring of Waku Archive, database, etc.

Details

First, apologies for that big PR:S. I started the integration of Postgres into wakinode2 but during the process, I ended up doing a more generic refactoring, sorry about that. I can explain everything until we are all happy with it.

The main purpose of this PR is to allow a Nwaku to mount the Waku Archive protocol by using Postgres as the underlying database. For that, run the WakuNode with:

./build/wakunode2 --store-message-db-url="postgres://postgres:test123@localhost:5432/postgres" ...

How to test

As usual, we can run two Nwaku nodes, a, and b.
b will act as the store node and will connect to the Postgres database.

1. Start local Postgres database. Go to the repo's root folder and run the next:

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

   where the postgres-docker-compose.yml contains:

   version: "3.8"
   
   services:
     db:
       image: postgres:9.6-alpine
       restart: always
       environment:
         POSTGRES_PASSWORD: test123
       ports:
         - "5432:5432"
   

2. Start a node that will act as a client

   ./build/wakunode2 --config-file=cfg_node_a.txt
   

cfg_node_a.txt

3. Start a node that will act as the "store", which will connect to the Postgres database.

      ./build/wakunode2 --config-file=cfg_node_b.txt
   

cfg_node_b.txt

4. Send a couple of requests to store two messages.
   The next command sends requests to node b (started in 3.)

   curl -d "{\"jsonrpc\":\"2.0\",\"id\":"$(date +%s%N)",\"method\":\"post_waku_v2_relay_v1_message\", \"params\":[\"/waku/2/default-waku/proto\", {\"timestamp\":"$(date +%s%N)", \"payload\":\"VGhpcyBpcyBhIG1lc3NhZ2UK\"}]}" --header "Content-Type: application/json" http://localhost:8546
   

5. Finally, send a "get_waku_v2_store_v1_messages" request to node a (started in 2.)

   curl -d '{"jsonrpc":"2.0","id":"id","method":"get_waku_v2_store_v1_messages"}' --header "Content-Type: application/json" http://localhost:8545
   

   And this should bring the two messages stored in point 4.

Issue

#1604

[jm-clius]

I guess before I could review, I'd need to understand why it was necessary to move all the archive setup, migration and maintenance logic into the archive module itself? This was specifically removed from the node modules before as the node should be unaware of which archive drivers the application choose to set up and how it manages these. According to me this all belongs to the application. Of course, if it becomes too large a part of the app module, it could be split into a separate module specifically aimed at how the application sets up its archive drivers. I'd still not extend the node's archive driver modules, though, as it's designed to be agnostic.

[SionoiS]

Architecturally there's are thing I like and other not so much. I like that app have less mentions of driver or DB but then the code end up in node non! non! non!

Archive drivers seams to be DB interfaces and Store is the protocol.
Was is the purpose of archive? Is it only for policy purpose?
Should it not be application specific how policies are applied?

Maybe we should talk about refactoring in general at some meeting? I got some words.

Nitpick: I don't like when lines are added or removed outside of scope. I am myself guilty of this.

Otherwise looks good so far!

[Ivansete-status]

Thanks for the comments @jm-clius !

Now, in the app.nim module, from master, we have the next procs:

proc setupDatabaseConnection(dbUrl: string): AppResult[Option[SqliteDatabase]] =

proc gatherSqlitePageStats(db: SqliteDatabase): AppResult[(int64, int64, int64)] =

proc performSqliteVacuum(db: SqliteDatabase): AppResult[void] =

Those procs refer to SQLite, which is very specific, so it needs to be moved into a "common/databases" folder. And I'm not keen on just adding other procs for Postgres in app.nim.

IMO, the only places that should refer to "SQLite" are the archive.nim module and the logic around the peer storage feature.

---

why it was necessary to move all the archive setup, migration and maintenance logic into the archive module itself?

We currently have the next in the App object (master:)

  App* = object
    version: string
    conf: WakuNodeConf

    rng: ref HmacDrbgContext
    peerStore: Option[WakuPeerStorage]
    archiveDriver: Option[ArchiveDriver]
    archiveRetentionPolicy: Option[RetentionPolicy]
    dynamicBootstrapNodes: seq[RemotePeerInfo]

    node: WakuNode

    rpcServer: Option[RpcHttpServer]
    restServer: Option[RestServerRef]
    metricsServer: Option[MetricsHttpServerRef]

Looking at it, we have an archive/store-protocol concept, archiveDriver and archiveRetentionPolicy, at the same level as node. This IMHO, is not correct and especially considering that the node itself already has a reference to wakuArchive -> driver, as shown below:

type
  WakuNode* = ref object
...
    wakuRelay*: WakuRelay
    wakuArchive*: WakuArchive
    wakuStore*: WakuStore

... many other protocols ...

type
  WakuArchive* = ref object
    driver*: ArchiveDriver  # TODO: Make this field private. Remove asterisk
    validator: MessageValidator
    retentionPolicy: RetentionPolicy

---

This was specifically removed from the node modules before as the node should be unaware of which archive drivers the application choose to set up and how it manages these. According to me this all belongs to the application.

I absolutely agree about that. Within the feature branch, the node is still unaware of the selected driver with the next considerations:

• The decision about which driver to create comes from the app.
• The WakuNode only has a reference to wakuArchive and therefore doesn't care if the underlying driver is SQLite or Postgres.
• The responsibility to create the appropriate ArchiveDriver falls under the archive module because this module is the one that knows about drivers. This module is the father/mother of the drivers. The app knows about config parameters but doesn't speak the "driver language", so it delegates that factory-driver responsibility to that archive module.

---

Of course, if it becomes too large a part of the app module, it could be split into a separate module specifically aimed at how the application sets up its archive drivers. I'd still not extend the node's archive driver modules, though, as it's designed to be agnostic.

IMO, there should be only one place where the drivers are considered. The only module that has interest on drivers is the archive one so any driver management outside it is not correct for me.

IMHO, the app module itself should be as simple as possible and it should be just the root of the "tree of responsibility".

I hope that justifies the changes :)

Thanks again for the comment and let's have a quick call if there is sth I'm missing.

[Ivansete-status]

As per our recent meeting. I'll set this on standby and I will submit a separate PR's in order to suggest the refactoring that is being suggested in this.

So ignore this PR for now :)

[Ivansete-status]

The original PR that I raised was too generic. Now, this is simpler as we already could make a proper refactor in previous PRs.

Thanks for your patience!

[alrevuelta]

the table should exist but perhaps its still valuable to check isErr?

[Ivansete-status]

I thought that was not important but now that you mention I'll check it :)
Thanks !