chore: Postgres migrations #2477

Ivansete-status · 2024-02-24T11:09:08Z

Description

Adding the needed logic so that we can safely change the Postgres database schema.
The approach is different from the current migration logic in SQLite. In this case, we avoid the use of external resources and instead, use a compiled consts that contains the migration scripts.
Now I'm setting the current schema version to 1. Notice that this PR includes a migration script aimed at converting the current table into a partitioned table, and this will happen in the upcoming PR, where the partition logic is implemented. Therefore, even though we have the migration script for version 2, we are still at version 1.

Changes

New breakIntoStatements proc in postgres_driver/migrations.nim to support handling PL/SQL scripts.
Refactor Postgres unit tests. Now the table is created by the migration procedures.
Starting to use a new "VERSION" table, where we simply store the current schema version.

How to test

Start node A with ./build/wakunode2 --config-file=cfg_node_a.txt
cfg_node_a.txt
In a separate terminal, start a Postgres database locally, with docker compose -f postgres-docker-compose.yml up:
Content of postgres-docker-compose.yml:

version: "3.8"

services:
  db:
    image: postgres:15.4-alpine3.18
    #image: hartmutcouk/pg-repack-docker:1.4.8 # https://github.com/hartmut-co-uk/pg-repack-docker/blob/master/README.md
    ## if we use the 'pg-repack-docker' image, we will need to call the next command externally:
    ## PGPASSWORD=test123 ~/utils/pg_repack-1.4.8/bin/pg_repack -U postgres -h 127.0.0.1 -p 5432 -d postgres --table messages
    ## With that, the database size gets reduced.
    restart: always
    volumes:
      - ./postgres_cfg/postgresql.conf:/etc/postgresql/postgresql.conf
      - ./postgres_cfg/db.sql:/docker-entrypoint-initdb.d/db.sql
      - ./postgres-data:/var/lib/postgresql/data:Z
    command: postgres -c config_file=/etc/postgresql/postgresql.conf
    environment:
      POSTGRES_PASSWORD: test123
    ports:
      - "5432:5432"

Notice that the following lines might not be needed. I cannot push the files. You can use the default settings:
- ./postgres_cfg/postgresql.conf:/etc/postgresql/postgresql.conf
- ./postgres_cfg/db.sql:/docker-entrypoint-initdb.d/db.sql

Start node B with ./build/wakunode2 --config-file=cfg_node_b.txt
cfg_node_b.txt
Publish a few messages during some time with bash msg_publisher.sh

Being msg_publisher.sh:

#!/bin/sh

while true
do
  payload_size=76800 
  payload=$(openssl rand -hex ${payload_size})
  
  # Example on how to publish to autosharding endpoint
  echo '{"payload":"'${payload}'","contentTopic":"my-ctopic-1", "timestamp":'$(date +%s%N)'}' > msg.txt
  curl -X POST "http://127.0.0.1:8645/relay/v1/messages/%2Fwaku%2F2%2Fdefault-waku%2Fproto"  -H "content-type: application/json"  -d @msg.txt
  
  sleep 0.1
done

Stop script started in 3 after few minutes
Stop node B
Change the target version from 1 to 2 in waku/waku_archive/driver/postgres_driver/migrations.nim
Recompile wakunode2 and run B again.
Without sending any message (the partition logic will come soon,) access directly the database (with Postbird f.e.) and there you should see that the messages table is a partitioned table and it exists a partition that contains all the messages stored in the previous raw messages table.

Issue

closes #2249

github-actions · 2024-02-24T11:09:20Z

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.

github-actions · 2024-02-24T11:13:44Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2477

Built from e9e35bb

… well

… wakunode binary

SionoiS

I'm not familiar with migrations but LGTM.

SionoiS · 2024-02-27T12:26:03Z

apps/wakunode2/app.nim

@@ -498,7 +498,7 @@ proc setupProtocols(node: WakuNode,

  if conf.store:
    # Archive setup
-    let archiveDriverRes = ArchiveDriver.new(conf.storeMessageDbUrl,
+    let archiveDriverRes = waitFor ArchiveDriver.new(conf.storeMessageDbUrl,


I don't think you can waitFor in a async proc. That would prevent the runtime from progressing no?

I think you can, it just means you are intended to wait till future become avail...
although I'm not pretty sure if it would not be better to wait with timeout?

Thanks for the comments!
@SionoiS - yes this is feasible. In this case we are blocking because this is a needed condition to be satisfied.
@NagyZoltanPeter - I will submit a separate PR where we apply that great proposal of using withTimeout

tests/waku_archive/test_driver_postgres.nim

SionoiS · 2024-02-27T12:40:43Z

waku/waku_archive/driver/builder.nim

@@ -63,17 +64,25 @@ proc new*(T: type ArchiveDriver,
    let db = dbRes.get()

    # SQLite vacuum
-    let (pageSize, pageCount, freelistCount) = ? db.gatherSqlitePageStats()
+    let sqliteStatsRes = db.gatherSqlitePageStats()


maybe .valueOr:or.isOkOr:? Same elsewhere.

Very good point. I'll submit a separate PR later on

SionoiS · 2024-02-27T12:45:29Z

waku/waku_archive/driver/postgres_driver/migrations.nim

Is there any need to rollback if a migration step fail?

Yes, very good point! I'd add this in the future as an enhancement

SionoiS · 2024-02-27T12:47:22Z

waku/waku_archive/driver/postgres_driver/postgres_driver.nim

-  let createMsgRes = await s.createMessageTable()
-  if createMsgRes.isErr():
-    return err("createMsgRes.isErr in init: " & createMsgRes.error)
+  let execRes = await s.writeConnPool.pgQuery(dropVersionTableQuery())


.isOkOr:?

gabrielmer

Amaaaazing PR! Thanks so much for it!
Adding some small questions

migrations/message_store_postgres/content_script_version_2.nim

tests/testlib/postgres.nim

tests/waku_archive/test_driver_postgres_query.nim

tests/waku_archive/test_driver_postgres.nim

jm-clius

Thanks! Without going into the details of the migration, approach generally LGTM. As mentioned elsewhere, I don't think we want to support endless migrations in the compiled code. At some point, migration scripts can be removed from the binary and provided as a standalone, consolidated script for older versions.

NagyZoltanPeter

Approve it within mind it will be followed by next step. Left some questions though.

NagyZoltanPeter · 2024-02-29T07:55:45Z

apps/wakunode2/app.nim

@@ -498,7 +498,7 @@ proc setupProtocols(node: WakuNode,

  if conf.store:
    # Archive setup
-    let archiveDriverRes = ArchiveDriver.new(conf.storeMessageDbUrl,
+    let archiveDriverRes = waitFor ArchiveDriver.new(conf.storeMessageDbUrl,


I think you can, it just means you are intended to wait till future become avail...
although I'm not pretty sure if it would not be better to wait with timeout?

NagyZoltanPeter · 2024-02-29T08:13:07Z

migrations/message_store_postgres/content_script_version_2.nim

+   id VARCHAR NOT NULL,
+   messageHash VARCHAR NOT NULL,
+   storedAt BIGINT NOT NULL,
+   CONSTRAINT messageIndex PRIMARY KEY (messageHash, storedAt)


For this migration I think it is better to add this index later after data inserted its time saving as more efficient to do it once for the db engine.

Ok good point , I will cover that in a separate PR.

tests/testlib/postgres.nim

waku/waku_archive/driver/builder.nim

waku/waku_archive/driver/postgres_driver/migrations.nim

…hiveDriver

Ivansete-status changed the title ~~chore: Feat pg migrations~~ chore: Postgres migrations Feb 24, 2024

Ivansete-status self-assigned this Feb 24, 2024

Ivansete-status added 6 commits February 26, 2024 14:05

step fwd in migration implementation and refactor sqlite migration as…

9d933f1

… well

Add postgres_driver/migrations.nim

26d7cf6

Postgres and archive logic adaptation to the migration implementation

afe5223

Add postgres migration scripts and migration utils

0ff9711

libwaku: adapt node_lifecycle_request.nim to migration refactoring

fa492fe

test_app.nim: add more detail for test that only fails in CI

cc6e377

Ivansete-status force-pushed the feat-pg-migrations branch from f230cc4 to cc6e377 Compare February 26, 2024 13:05

Ivansete-status added 10 commits February 26, 2024 20:55

postgres migrations: store the migration scripts inside the resulting…

a2b07a8

… wakunode binary

db_sqlite: revert getUserVersion rename

850cabc

db_sqlite.nim: revert too many changes

545cae4

Rm uneeded sql files for postgres migration logic

b9c3a6b

partially revert migration-logic changes in sql

c45bc69

postgres_driver/migrations.nim: remove unused variables

f5848dc

databases/db_sqlite: reverting completely the sqlite.nim changes

72b62fd

Remove migration_utils module

fd7828d

postgres_driver/migrations.nim: add breakInfoStatements proc

e7df8bb

postgres_driver/migrations.nim: remove unused import

d1ac066

Ivansete-status marked this pull request as ready for review February 27, 2024 10:07

Ivansete-status requested review from jm-clius, SionoiS, gabrielmer and NagyZoltanPeter February 27, 2024 10:07

SionoiS approved these changes Feb 27, 2024

View reviewed changes

gabrielmer reviewed Feb 27, 2024

View reviewed changes

jm-clius approved these changes Feb 28, 2024

View reviewed changes

NagyZoltanPeter approved these changes Feb 29, 2024

View reviewed changes

Ivansete-status added 4 commits February 29, 2024 18:55

test_driver_postgres_query.nim: more generic suite title

1723133

testlib/postgres.nim: add error log in case of error initializing Arc…

6de70a5

…hiveDriver

testlib/postgres.nim: better error logs

8bb6813

test postgres: avoid use of cast[PostgresDriver](..)

4b8d76e

Ivansete-status force-pushed the feat-pg-migrations branch from b0e3c38 to 4b8d76e Compare February 29, 2024 22:06

Ivansete-status merged commit 560f949 into master Mar 1, 2024
8 of 10 checks passed

Ivansete-status deleted the feat-pg-migrations branch March 1, 2024 11:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Postgres migrations #2477

chore: Postgres migrations #2477

Ivansete-status commented Feb 24, 2024 •

edited

Loading

github-actions bot commented Feb 24, 2024

github-actions bot commented Feb 24, 2024 •

edited

Loading

SionoiS left a comment

SionoiS Feb 27, 2024

NagyZoltanPeter Feb 29, 2024

Ivansete-status Feb 29, 2024

SionoiS Feb 27, 2024

Ivansete-status Feb 29, 2024

SionoiS Feb 27, 2024

Ivansete-status Feb 29, 2024

SionoiS Feb 27, 2024

gabrielmer left a comment

jm-clius left a comment

NagyZoltanPeter left a comment

NagyZoltanPeter Feb 29, 2024

NagyZoltanPeter Feb 29, 2024

Ivansete-status Feb 29, 2024

chore: Postgres migrations #2477

chore: Postgres migrations #2477

Conversation

Ivansete-status commented Feb 24, 2024 • edited Loading

Description

Changes

How to test

Issue

github-actions bot commented Feb 24, 2024

github-actions bot commented Feb 24, 2024 • edited Loading

SionoiS 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

gabrielmer left a comment

Choose a reason for hiding this comment

jm-clius left a comment

Choose a reason for hiding this comment

NagyZoltanPeter 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

Ivansete-status commented Feb 24, 2024 •

edited

Loading

github-actions bot commented Feb 24, 2024 •

edited

Loading