feat: create database tables and queries for notifications #13536

dannykopping · 2024-06-11T10:05:22Z

Creates two new tables:

notification_templates: templates are used to create notifications from, and specify a unique event in the system
- right now this will just store immutable system notification templates, but in the future users will be able to configure their own notification templates (e.g. notify when a workspace's disk is 90% full)
notification_messages: each record corresponds to a separate notification which must be delivered to a user
- this table is effectively a queue, and queue consumers can consume these records concurrently with safety (see AcquireNotificationMessages)

coderd/database/migrations/000219_notifications.up.sql inserts a single template ("Workspace Deleted"), for which we are creating this vertical slice in this initial stack of PRs.

dannykopping · 2024-06-11T10:05:36Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @dannykopping and the rest of your teammates on Graphite

coderd/database/migrations/000219_notifications.up.sql

coderd/database/queries/notifications.sql

coderd/database/sqlc.yaml

coderd/database/migrations/000219_notifications.down.sql

coderd/database/migrations/000219_notifications.up.sql

coderd/database/dbmem/dbmem.go

coderd/database/dbauthz/dbauthz.go

coderd/database/migrations/testdata/fixtures/000219_notifications.up.sql

johnstcn

Some comments below, but nothing blocking.

Signed-off-by: Danny Kopping <danny@coder.com>

spikecurtis · 2024-06-12T07:06:25Z

coderd/database/migrations/testdata/fixtures/000219_notifications.up.sql

+               ('b0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12', 'B', TRUE, template, template, 'Group 1'),
+               ('c0eebc99-9c0b-4ef8-bb6d-6bb9bd380a13', 'C', TRUE, template, template, 'Group 2');
+
+        INSERT INTO public.users(id, email, username, hashed_password, created_at, updated_at, status, rbac_roles, deleted)


Probably fine in a test fixture, but we should avoid using the schema name public since that's not what it's always called in customer databases.

Whoops, good catch!

spikecurtis · 2024-06-12T07:08:48Z

coderd/database/migrations/000219_notifications.down.sql

@@ -0,0 +1,5 @@
+DROP TABLE IF EXISTS notification_messages;
+DROP TABLE IF EXISTS notification_templates;
+DROP TYPE IF EXISTS notification_receiver;


I don't think this type is created in the UP migration

Thanks, this was a rename gone wrong.

spikecurtis · 2024-06-12T07:16:50Z

coderd/database/migrations/000219_notifications.up.sql

+    enabled        boolean DEFAULT TRUE NOT NULL,
+    title_template text                 NOT NULL,
+    body_template  text                 NOT NULL,
+    actions        jsonb,


What is "actions"? It doesn't even appear in your INSERT query and doesn't appear to be used anywhere in the PR upstack from here.

Your other comment about the InsertNotificationTemplate is correct; this is actually not needed right now. It'll be used later on; I'll remove it.

The actions are the calls-to-action which are associated with the template; see the insert at the bottom of this file.

Got it now. The upstack PR does use it, I can see. I didn't find it because I was looking at the main NotificationTemplate type, and it's used in FetchNewMessageMetadataRow

spikecurtis · 2024-06-12T07:26:53Z

coderd/database/queries/notifications.sql

+                leased_until = NOW() + CONCAT(sqlc.arg('lease_seconds')::int, ' seconds')::interval
+            WHERE id IN (SELECT nm.id
+                         FROM notification_messages AS nm
+                                  LEFT JOIN notification_templates AS nt ON (nm.notification_template_id = nt.id)


why does this need to join to notification_templates? I don't see any reference to nt in the WHERE clause...

Another good catch; this is the result of an incomplete refactoring.

spikecurtis · 2024-06-12T07:47:22Z

coderd/database/queries/notifications.sql

+                                 SKIP LOCKED
+                         LIMIT sqlc.arg('count'))
+            RETURNING id)
+SELECT


I think this would be easier to read as a single UPDATE

e.g.

UPDATE notification_messages as nm SET ... FROM notification_templates as nt WHERE nm.notification_template_id = nt.template_id AND ... RETURNING ...

It's not standard SQL, but postgres essentially does a join to tables in FROM, which allows you to use them in SET (not needed here) and RETURNING (what we want).

If that's too strange, I at least think it's clearer to return all the columns of notification_messages we want from the UPDATE, then do a single join to notification_templates, rather than returning the IDs and then doing two joins: back to itself and to notification_templates.

I will look into this 👍

One other idea: this query and also the FetchNewMessageMetadata query are complicated by the need to pull NotificationTemplate data. But there is a small, finite number of NotificationTemplates, and we don't allow modifications... so, we could just lazily load the NotificationTemplates and cache them in the notifier/manager.

That makes the queries simpler to understand (FetchNewMessageMetadata is probably not needed since you can just query the user), and likely faster to execute since Postgres doesn't need to do a join.

At some point down the line if we allow people to modify the templates at runtime, we'd need to invalidate the cache, but that seems pretty simple via the PubSub.

spikecurtis · 2024-06-12T08:15:24Z

coderd/database/queries/notifications.sql

+        notification_messages
+            SET updated_at = NOW(),
+                status = 'leased'::notification_message_status,
+                status_reason = 'Leased by notifier ' || sqlc.arg('notifier_id')::int,


I'm concerned that in a distributed system, these IDs are assigned locally, and thus don't really tell us which notifier has the message leased

Great point! Funnily enough I had this as a UUID previously but reverted to an int for some reason.

This will be implemented in the PR up the stack.

spikecurtis · 2024-06-12T09:02:09Z

coderd/database/queries/notifications.sql

+    leased_until     = NULL,
+    next_retry_after = NULL
+FROM (SELECT id, sent_at
+      FROM new_values) AS subquery


do we really need both the WITH clause and this clause remapping new_values to subquery? Is it not supported to just have

FROM (SELECT UNNEST(@ids::uuid[]) AS id, UNNEST(@sent_ats::timestamptz[]) AS sent_at) as new_values

Yes it is, thanks 👍

spikecurtis · 2024-06-12T09:06:24Z

coderd/database/queries/notifications.sql

+       FROM notification_messages AS nested
+       WHERE nested.updated_at < NOW() - INTERVAL '7 days'
+             -- ensure we don't clash with the notifier
+           FOR UPDATE SKIP LOCKED);


this SELECT FOR UPDATE SKIP LOCKED business is just a performance optimization, and unless we're running these queries very often I don't think it's needed. A plain ole DELETE statement will wait for locked rows. The SKIP LOCKED construction causes it to skip locked rows rather than wait for them.

spikecurtis · 2024-06-12T09:10:02Z

coderd/database/queries/notifications.sql

@@ -0,0 +1,139 @@
+-- name: InsertNotificationTemplate :one


Is this necessary? I thought all templates are going to be set by migrations

It's not necessary right now, correct. This was from an earlier design phase and I missed it; will remove.

Signed-off-by: Danny Kopping <danny@coder.com>

github-actions bot assigned dannykopping Jun 11, 2024

dannykopping mentioned this pull request Jun 11, 2024

feat: implement thin vertical slice of system-generated notifications #13537

Open

dannykopping force-pushed the dk/system-notifications-database branch from 1cb776a to 25636ec Compare June 11, 2024 13:15

dannykopping changed the title ~~feat: database tables & queries~~ feat: create database tables and queries for notifications Jun 11, 2024

dannykopping marked this pull request as ready for review June 11, 2024 14:13

dannykopping requested review from johnstcn, kylecarbs, mtojek and spikecurtis June 11, 2024 15:03

mtojek reviewed Jun 12, 2024

View reviewed changes

johnstcn reviewed Jun 12, 2024

View reviewed changes

coderd/database/migrations/testdata/fixtures/000219_notifications.up.sql Outdated Show resolved Hide resolved

johnstcn approved these changes Jun 12, 2024

View reviewed changes

mtojek approved these changes Jun 12, 2024

View reviewed changes

dannykopping force-pushed the dk/system-notifications-database branch from 1c4046c to 538bc42 Compare June 12, 2024 09:08

dannykopping added 4 commits June 12, 2024 11:08

feat: database tables & queries

6ccaba1

Adding more fixtures to appease the test

3a427b0

Signed-off-by: Danny Kopping <danny@coder.com>

Manual fixup of sqlc version until #13540 works

bdaea3b

Signed-off-by: Danny Kopping <danny@coder.com>

Review comments

f53a8aa

Signed-off-by: Danny Kopping <danny@coder.com>

spikecurtis reviewed Jun 12, 2024

View reviewed changes

Review comments

438bcba

Signed-off-by: Danny Kopping <danny@coder.com>

dannykopping force-pushed the dk/system-notifications-database branch from 538bc42 to 438bcba Compare June 12, 2024 10:51

github-actions bot added the stale This issue is like stale bread. label Jun 20, 2024

github-actions bot closed this Jun 23, 2024

johnstcn reopened this Jun 23, 2024

johnstcn removed the stale This issue is like stale bread. label Jun 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: create database tables and queries for notifications #13536

feat: create database tables and queries for notifications #13536

dannykopping commented Jun 11, 2024 •

edited

dannykopping commented Jun 11, 2024 •

edited

johnstcn left a comment

spikecurtis Jun 12, 2024

dannykopping Jun 12, 2024

spikecurtis Jun 12, 2024

dannykopping Jun 12, 2024

spikecurtis Jun 12, 2024

dannykopping Jun 12, 2024

spikecurtis Jun 12, 2024

spikecurtis Jun 12, 2024

dannykopping Jun 12, 2024

spikecurtis Jun 12, 2024

dannykopping Jun 12, 2024

spikecurtis Jun 12, 2024

spikecurtis Jun 12, 2024

dannykopping Jun 12, 2024

dannykopping Jun 12, 2024

spikecurtis Jun 12, 2024

dannykopping Jun 12, 2024

spikecurtis Jun 12, 2024

spikecurtis Jun 12, 2024

dannykopping Jun 12, 2024

feat: create database tables and queries for notifications #13536

Are you sure you want to change the base?

feat: create database tables and queries for notifications #13536

Conversation

dannykopping commented Jun 11, 2024 • edited

dannykopping commented Jun 11, 2024 • edited

johnstcn 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

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

dannykopping commented Jun 11, 2024 •

edited

dannykopping commented Jun 11, 2024 •

edited