-
-
Notifications
You must be signed in to change notification settings - Fork 301
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add broadcast functionality from triggers #1156
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
af3caf2
to
6fbe933
Compare
lib/realtime/tenants/repo/migrations/20240917170412_add_payload_to_messages.ex
Outdated
Show resolved
Hide resolved
b346ad0
to
b27bf56
Compare
lib/realtime/tenants/repo/migrations/20240917170412_add_payload_to_messages.ex
Outdated
Show resolved
Hide resolved
b27bf56
to
ef0d4ca
Compare
Should our payload match what we have here in the Webhooks trigger? |
9ab5171
to
0e27113
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wouldn't hurt to have a check like
IF TG_LEVEL = 'STATEMENT' THEN
RAISE EXCEPTION 'realtime.broadcast_changes should be triggered for each row, not for each statement';
END IF;
to make sure no one tries to "optimize" their trigger for bulk operations
lib/realtime/tenants/repo/migrations/20240917170412_add_payload_to_messages.ex
Outdated
Show resolved
Hide resolved
row_data jsonb := '{}'::jsonb; | ||
-- Declare entry that will be written to the realtime.messages table | ||
topic_name text := TG_ARGV[0]::text; | ||
event_name text := COALESCE(TG_ARGV[1]::text, TG_TABLE_SCHEMA || '.' || TG_TABLE_NAME); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the TG_ARGV[1]::text
needed vs relying on TG_TABLE_SCHEMA
+ TG_TABLE_NAME
?
it looks like that would allow users to override the event / qualified table name
what happens if the user overrides it with nonsense instead of a valid target?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our event names are arbitrary, this is just prescriptive for broadcasting changes from tables.
@filipecabaco we could just not make this an argument for now. One less thing for devs to think about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the idea is to have this be flexible so if a user wants to have multiple tables to be funnelled into a single topic with a single event they are able to. just giving extra flexibility (and an extra gun to potentially lose a foot to be fair)
I'll bring this into the discussion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will keep the event overridable as it might make it easier for users to migrate to this approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our event names are arbitrary, this is just prescriptive for broadcasting changes from tables.
oh nice. In that case this looks like a great solution as long as its documented well
Adds a new functionality to broadcast db changes. It adds: * New function to broadcast from postgres using triggers
bd3075e
to
25a965b
Compare
8806f00
to
69ce712
Compare
69ce712
to
1a3434f
Compare
What kind of change does this PR introduce?
Adds a new functionality to broadcast db changes. It adds:
Example: