Skip to content
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

Remove all in-code use of leadership "instance name" + constrain column to single value #325

Merged
merged 1 commit into from
May 2, 2024

Conversation

brandur
Copy link
Contributor

@brandur brandur commented May 2, 2024

With the addition of #301, and more specifically the schema-based
namespacing that it brings in around listen/notify, we're fully moving
towards a world where the recommendation for running multiple Rivers in
a single database is very definitive: isolate them by schema.

The elector's had a long-standing parameter called "instance name"
that's stored into the river_leader table, and which was prospectively
going to be used for River namespacing, but something we never made use
of it. In a world of schema isolation, each schema will have its own
river_leader table, and no kind of additional namespacing is needed
within the table.

I was originally going to try and approach herein we drop name out of
river_leader completely, but looking more closely, it's the table's
primary key. We could add a new column that'd act as a primary key
instead (e.g. imagine a boolean primary key column with a check
constraint that makes sure it's always true), but nothing we'd add would
be that much better. Instead, I elected to give name a default value
of default (matching the previous default instance name), and add a
check constraint verifying that it's always default, making it
effectively a single row table. The nice part about this approach is
that we can put these changes into the V4 migration in #301, and we
won't require any additional changes in any future migration.

With name now constrained to a single value, we can simplify all the
river_leader-based queries by removing their name parameters, then
remove instance name completely from elector code and drivers, giving us
a thorough overall cleanup.

…mn to single value

With the addition of #301, and more specifically the schema-based
namespacing that it brings in around listen/notify, we're fully moving
towards a world where the recommendation for running multiple Rivers in
a single database is very definitive: isolate them by schema.

The elector's had a long-standing parameter called "instance name"
that's stored into the `river_leader` table, and which was prospectively
going to be used for River namespacing, but something we never made use
of it. In a world of schema isolation, each schema will have its own
`river_leader` table, and no kind of additional namespacing is needed
within the table.

I was originally going to try and approach herein we drop `name` out of
`river_leader` completely, but looking more closely, it's the table's
primary key. We could add a new column that'd act as a primary key
instead (e.g. imagine a boolean primary key column with a check
constraint that makes sure it's always true), but nothing we'd add would
be that much better. Instead, I elected to give `name` a default value
of `default` (matching the previous default instance name), and add a
check constraint verifying that it's always `default`, making it
effectively a single row table. The nice part about this approach is
that we can put these changes into the V4 migration in #301, and we
won't require any additional changes in any future migration.

With `name` now constrained to a single value, we can simplify all the
`river_leader`-based queries by removing their name parameters, then
remove instance name completely from elector code and drivers, giving us
a thorough overall cleanup.
@brandur brandur force-pushed the brandur-drop-instance-name branch from 9717e12 to 7f75dc0 Compare May 2, 2024 00:33
@brandur brandur requested a review from bgentry May 2, 2024 00:37
@brandur
Copy link
Contributor Author

brandur commented May 2, 2024

@bgentry Thoughts on this approach? See PR description, but I realized that name was acting as the primary key for river_leader. We could change the key to something else, but it'd involve adding some new dummy column to act as one, so I opted instead to default name to default, and add a check constraint to verify that only the value default is allowed. An advantage of this compared to what you and I had discussed before is that name will stay around, meaning no future migration will be required.

Copy link
Contributor

@bgentry bgentry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me!

@brandur
Copy link
Contributor Author

brandur commented May 2, 2024

Wicked, thanks!

@brandur brandur merged commit 7f87768 into master May 2, 2024
10 checks passed
@brandur brandur deleted the brandur-drop-instance-name branch May 2, 2024 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants