-
Notifications
You must be signed in to change notification settings - Fork 1
Network keys #250
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
base: master
Are you sure you want to change the base?
Network keys #250
Conversation
54b4067 to
0a8378b
Compare
9d9d948 to
8043590
Compare
QSparks
left a comment
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.
Looks good, just a few minor comments.
I am not familiar with the database internals, but the migration logic looks sound.
One possible edge case: network_name is nullable in the schema. If any rows have NULL network_name, they'll get NULL network_key values (which PostgreSQL's unique constraint allows). Do we ever have NULL network names in practice? If so, should we validate that all network names are populated?
tests/climate_baseline_helpers/test_climate_baseline_helpers.py
Outdated
Show resolved
Hide resolved
tests/alembic_migrations/versions/v_bdc28573df56_add_obs_raw_indexes/test_smoke.py
Outdated
Show resolved
Hide resolved
...s/alembic_migrations/versions/v_7a3b247c577b_add_varsperhistory_native_matview/test_smoke.py
Outdated
Show resolved
Hide resolved
jameshiebert
left a comment
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.
Just a couple small changes to consider. I'll let you be the judge of their practicality.
| ) | ||
| ) | ||
|
|
||
| # Drop existing triggers before modifying table structure so that we don't accidentally track |
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.
Near as I can tell, you got the sequence right for dropping/recreating all of the triggers/constraints/FKs. Not sure if this is possible, but could we disable the triggers or defer them, so we don't have to recreate them by definition? I feel like it would make the code less verbose, and give us the assurance that we're not changing the definition (unless that's what we want).
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.
I'm not sure it changes verbosity too much, but I've added some functions to enable and disable this trigger rather than removing it.
I'm fairly certain the code in its current state is safe, but if someone were to change the underlying functions that created and removed these triggers there is the potential for unexpected definition change.
| sa.Column( | ||
| "network_key", | ||
| sa.String(), | ||
| nullable=True, |
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.
To Quintin's point, network_name, though currently NULLable, shouldn't be (and in practice, never is), and I think network_key should not be nullable either. What good is a key that's NULL :)
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.
I agree but I think I'm I'm stuck in a catch 22 here. I can't add a new column with out it being either a) nullable or b) have a default value applied. Default values in postgres need to be pretty simple and can't refer to other column data even via functions.
In order to circumvent this the code is as currently applied: The column is created as a nullable but a trigger will populate this column any time an insert happens acting as a default.
…on in _init_.py for tables
Having chatted with Rod about this we came to the conclusion that being able to handle renaming networks while still being able to refer to them in some sort of human readable way is best served by adding a new immutable and unique column to the meta_network table. This column is
network_key. This PR handles the changes required to add this column as well as resolve various issues arising from adding columns to a history enabled Pycds table. Main changes:Info on the second point:
Rod's setup for history tracking uses a set of triggers that execute any time modifications are made to the base tables. These triggers are shared by all tables that the history tracking is applied to and rely on column order to insert the right information from the base table into the history. When adding a new column it is added to the end, we can add it to both tables but because history includes two extra columns:
deletedand<table>_hx_idwhen trying to run the triggers with the new column they aren't in alignment. There are 3 main potential fixes for this:We've opted for #1 for now as it is pretty rare that it is necessary to add new columns to an existing table and is the pragmatic approach to this problem.