fix: reduce postgres notify messaging #409
Conversation
saikonen
force-pushed
the
fix/reduce-postgres-notify-messaging
branch
from
61579f9
to
ce4711b
Compare
|
|
||
| triggers_to_cleanup = [ | ||
| res[0] for res in results | ||
| if TRIGGER_VERSION not in res[0] |
There was a problem hiding this comment.
we can probably do with startswith to be extra careful. Plus maybe a slightly cheaper operation.
There was a problem hiding this comment.
the trigger names have a notify_ui prefix so I figured keeping it to make it clear what the triggers are used for made sense. I can shuffle the prefix and version around of course, but there are only a handful of triggers to go through (or should be?) so there shouldn't be any concern with how cheap the operation is. This only runs once during server startup
There was a problem hiding this comment.
I got a tad confused btw -- there seems to be AsyncTaskTablePostgres here but then it is also in ui_backend_service/data/db/tables. Is this normal? Is one used for the metadata service and one for the ui backend. It was a tad confusing because then I wasn't sure which one would get to read operations/conditions, etc for triggers. It's a minor nit but just curious.
There was a problem hiding this comment.
the services/data table classes are functionally aimed for the original metadata services needs. During the ui_backend development introducing new stuff into them was out of scope, and if I recall correctly we did add some changes to existing methods specifically for the UI use case. This is why the ui_backend has its own table classes, that use services/data as a base
there might be some duplication nowadays as the base classes have received some changes as well, but for example keeping the custom queries that the UI relies on out of the base classes made sense back then.
There was a problem hiding this comment.
but then again (to immediately contradict my points), the trigger setup has been kept in the base classes for reasons that I don't recall. Possibly due to some thoughts that the UI backend would initially run off a proper read replica so it would not be able to modify the DB to set up any triggers itself. This ended up not being possible after all due to limitations with read replicas regarding pg_notify, and eventually having the ui_backend be responsible for the trigger setup was the best option.
There is definitely a lot of room for cleanup regarding the table classes.
DRAFT
outline for versioning table triggers and introducing a cleanup process for setting them up, so we can update them successfully in case of required changes.
Also trying out reducing postgres notify messaging on certain tables.