Router on postgres #389

Tetrino · 2023-08-22T11:12:24Z

The underlying functionality is the same, but we are moving away from
mongo to postgres to ensure we can continue to maintain the app.

We no longer keep soft copies of the data read from the database,
instead reading each row from memory.

We no longer rely on comparing OpTime from Mongo changes, instead relying
on PSQL's LISTEN/NOTIFY process to notify us of changes. However, in order
to avoid overloading with constant update requests, we still poll
the update every two seconds.

https://trello.com/c/YJro3BGj

pq is a standard library for interacting with postgresql databases in golang. It is currently in maintenance mode so is as stable as it can be for the foreseeable future.

The underlying functionality is the same, but we are moving away from mongo to postgres to ensure we can continue to maintain the app. We no longer keep soft copies of the data read from the database, instead reading each row from memory. We no longer rely on comparing OpTime from Mongo changes, instead relying on PSQL's LISTEN/NOTIFY process to notify us of changes. However, in order to avoid overloading with constant update requests, we still poll the update every two seconds.

sengi · 2023-09-20T13:56:57Z

Makefile

+			--health-start-period 5s \
+			postgres:14; \
+		echo Waiting for postgres to be up; \
+		until [ "`docker inspect -f '{{.State.Health.Status}}' router-postgres-test-db`" = "healthy" ]; do \


optional: if Docker or the container fails then this could get stuck for a long time (6h on GitHub Actions), so could so with for _ in $(seq 30) so we fail faster

sorry that was meant to be sent along with the review rather than as a one-off (I hit the wrong button)

sengi

Cool — I like the minimal approach.

sengi · 2023-09-20T14:04:03Z

Makefile

+			--health-start-period 5s \
+			postgres:14; \
+		echo Waiting for postgres to be up; \
+		until [ "`docker inspect -f '{{.State.Health.Status}}' router-postgres-test-db`" = "healthy" ]; do \


sorry that was meant to be sent along with the review rather than as a one-off (I hit the wrong button)

sengi · 2023-09-20T14:08:58Z

lib/router.go

-	RedirectType string `bson:"redirect_type"`
-	SegmentsMode string `bson:"segments_mode"`
-	Disabled     bool   `bson:"disabled"`
+	IncomingPath sql.NullString


optional: it'd be neat if we could use plain old strings here and deal with the nullability in loadRoutes or wherever, then we could delete this struct's evil twin in integration_tests/route_helpers.go. (Would also mean you wouldn't need to deal with nulls in all the places that use struct Route.)

sengi · 2023-09-20T14:11:13Z

lib/router.go

-	for _, instance := range replicaSet.Members {
-		if instance.Current {
-			currentInstance = append(currentInstance, instance)
+func (rt *Router) shouldReload(listener *pq.Listener) bool {


This is so much neater — I love it

sengi · 2023-09-20T14:15:54Z

localdb_init.sql

+FOR EACH ROW
+EXECUTE PROCEDURE notify_listeners();
+
+CREATE TRIGGER rotues_notify_trigger 


harmless typo

Suggested change

CREATE TRIGGER rotues_notify_trigger

CREATE TRIGGER routes_notify_trigger

sengi · 2023-09-20T14:19:37Z

localdb_init.sql

@@ -0,0 +1,67 @@
+DROP DATABASE IF EXISTS router_test;


(no action needed) It's kind of a shame we need this almost-but-not-quite-identical copy of the schema here, but I agree it's probably the right call in the interests of time rather than revamping the way the tests work to pull in the real schema from router-api 👍

Agree with @sengi

The very slight downside of having router-api being the source of the database is that it makes everything in govuk-docker that depends on router now also dependent on router-api. But the advantages of having it as an ordinary rails migration outweighs that (plus the fact that router/router-api doesn't really work on govuk-docker at the moment anyway, and converting to PG might unblock that problem).

That just sounds like a govuk-docker bug/flaw tbh :) I hope next quarter we'll be able to make govuk-helm-charts portable enough to replace at least some govuk-docker use cases in local development / testing.

sengi · 2023-09-20T14:27:08Z

my optionals are genuinely optional btw — totally fine if you prefer not to do them (and no need to justify!)

Very little of the tests themselves had to be rewritten to reflect the system being postgres, however a number of tests that relied on mocking Mongo are no longer valid in the Postgres implementation and have been removed. We now spin up a test PSQL database container using an initialiser sql file to test against.

Tetrino force-pushed the router-on-postgres branch 18 times, most recently from 4596254 to 268d251 Compare August 23, 2023 13:33

Tetrino force-pushed the router-on-postgres branch 7 times, most recently from 2b73fb8 to cff0771 Compare September 5, 2023 11:08

Tetrino force-pushed the router-on-postgres branch 3 times, most recently from e2a0e19 to 266ed28 Compare September 6, 2023 10:54

Tetrino added 2 commits September 6, 2023 12:22

Remove MGO mongo library and replace it with PQ

7932f2a

pq is a standard library for interacting with postgresql databases in golang. It is currently in maintenance mode so is as stable as it can be for the foreseeable future.

Tetrino force-pushed the router-on-postgres branch from 266ed28 to 16aeef6 Compare September 6, 2023 11:33

Tetrino requested a review from sengi September 7, 2023 09:46

sengi reviewed Sep 20, 2023

View reviewed changes

sengi approved these changes Sep 20, 2023

View reviewed changes

hannako mentioned this pull request Oct 6, 2023

Router migration to postgres #364

Closed

Tetrino force-pushed the router-on-postgres branch from 16aeef6 to 6e91739 Compare October 18, 2023 14:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Router on postgres #389

Router on postgres #389

Tetrino commented Aug 22, 2023 •

edited

sengi Sep 20, 2023 •

edited

sengi Sep 20, 2023

sengi left a comment

sengi Sep 20, 2023

sengi Sep 20, 2023

sengi Sep 20, 2023

sengi Sep 20, 2023

sengi Sep 20, 2023 •

edited

KludgeKML Sep 20, 2023

sengi Sep 20, 2023

sengi commented Sep 20, 2023

	CREATE TRIGGER rotues_notify_trigger
	CREATE TRIGGER routes_notify_trigger

Router on postgres #389

Are you sure you want to change the base?

Router on postgres #389

Conversation

Tetrino commented Aug 22, 2023 • edited

sengi Sep 20, 2023 • edited

Choose a reason for hiding this comment

sengi Sep 20, 2023

Choose a reason for hiding this comment

sengi left a comment

Choose a reason for hiding this comment

sengi Sep 20, 2023

Choose a reason for hiding this comment

sengi Sep 20, 2023

Choose a reason for hiding this comment

sengi Sep 20, 2023

Choose a reason for hiding this comment

sengi Sep 20, 2023

Choose a reason for hiding this comment

sengi Sep 20, 2023 • edited

Choose a reason for hiding this comment

KludgeKML Sep 20, 2023

Choose a reason for hiding this comment

sengi Sep 20, 2023

Choose a reason for hiding this comment

sengi commented Sep 20, 2023

Tetrino commented Aug 22, 2023 •

edited

sengi Sep 20, 2023 •

edited

sengi Sep 20, 2023 •

edited