chore (sync-service): unique publication and slot names per tenant #1966
Conversation
✅ Deploy Preview for electric-next ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| start_tenant_opts = [ | ||
| app_config: app_config, | ||
| app_config: updated_app_config, |
There was a problem hiding this comment.
The app config is currently fixed for all tenants, you're making it tenant specific which changes it's semantics and makes it a bit confusing in my opinion. I would instead rename publication_name to publication_prefix and do the tenant ID hash append elsewhere.
There was a problem hiding this comment.
To give an example of the confusion, Application.Configuration is implemented as a persistent_term so that any piece of code can grab the config, but if it does this it will get the wrong publication_name
There was a problem hiding this comment.
Doing the tenant ID hash append elsewhere is error prone. I used to do it in tenant/supervisor.ex on L64 and 66 but the app config is also used in other places where the append would also have to happen, resulting in code duplication and easy to forget the append somewhere which would break it. The most robust approach is to turn the app config in a per tenant config such that it contains the right publication and slot names everywhere it is passed. Perhaps let's rename it to per_tenant_app_config as per @msfstef's suggestion.
There was a problem hiding this comment.
Well TenantConfig would be more concise 😄 If you want to go that route, then you'll need to change the semantics of the module and probably get of the rid of the persistant_term. It sounds like this is worth leaving until the multi-tenant code has been separated from the core sync-service code.
| "@core/sync-service": patch | ||
| --- | ||
|
|
||
| Make publication and slot names unique per tenant. |
There was a problem hiding this comment.
If we decide to add the hash for the default tenant (i.e. "non-multi-tenancy mode") then this is a breaking change that would benefit from a little bit more explantation in the changeset or elsewhere. We should advise users that the new deploy will "clear" their cache and they will need to clean up their old replication slot.
We need to make the decision as to whether we want to not include this hash for the cases where tenant_id === default_tenant. I think it makes sense, especially because currently if ELECTRIC_TENANT_ID is not specified (which is the most likely scenario for non-multi-tenancy) we auto generate a UUID which means that on every electric boot the default tenant id is going to be different and would create a different replication slot.
There was a problem hiding this comment.
Yes, there are pros and cons for both approaches (always using the hash or only in multi tenant mode). Let's discuss this with the wider team and make a decision.
|
Status update: this PR has been put on hold until we extract multi tenancy out of the core Electric library. |
|
Closing as this is addressed by #1991 |
This PR modifies the publication and slot names such that they contain the hash of the tenant ID.
Since tenant IDs are expected to be unique across all Electric instances this will result in unique publication and slot names. This makes it possible for several Electric instances to connect to the same PG database.