Allow empty table with allow_empty flag#37
Conversation
Coverage Report Results
1 empty file skipped. |
| with self.tracker.track(_tracker.Stage.PRE_VALIDATION): | ||
| if self.introspector.table_is_empty(table=self.table): | ||
| raise TableIsEmpty("No reason to repack an empty table.") | ||
| if not self.convert_pk_to_bigint: |
There was a problem hiding this comment.
❓ Are there any other schema changes where it is valid to avoid an error for an empty table here? And if not are there likely to be others added? Mainly thinking if that's the case then having a dedicated function for this that can be extended if other schema changes are added rather than needing to dig through
There was a problem hiding this comment.
We expect to support other schema changes (such as adding an exclusion constraint) at some point.
For now, I think it's okay. But when we do add other schema changes, we should refactor the check into a function as you suggest.
There was a problem hiding this comment.
+1 on Suzannah's suggestion, I think it makes the code more self-documenting (i.e. this bit cares about schema changes, not bigint conversions specifically, it just so happens that the only supported schema change currently is bigint conversion)
There was a problem hiding this comment.
Following other discussions, I've changed this PR to add the allow_empty flag instead of inferring that empty tables should be allowed due the presence of a schema change (initially just bigint conversion). With that change, I think this discussion thread is now moot. Agree?
There was a problem hiding this comment.
Yeah I like this! The allow_empty flag makes a lot of sense
| with self.tracker.track(_tracker.Stage.PRE_VALIDATION): | ||
| if self.introspector.table_is_empty(table=self.table): | ||
| raise TableIsEmpty("No reason to repack an empty table.") | ||
| if not self.convert_pk_to_bigint: |
There was a problem hiding this comment.
+1 on Suzannah's suggestion, I think it makes the code more self-documenting (i.e. this bit cares about schema changes, not bigint conversions specifically, it just so happens that the only supported schema change currently is bigint conversion)
Prior to this change, running psycopack on an empty table would cause the TableIsEmpty exception to be raised. This change adds an option to allow empty tables to be handled without raising an exception. This enables psycopack to be used (e.g. for schema changes) irrespective of whether the table is empty or not.
timb07
force-pushed
the
allow-empty-table-with-bigint-conversion
branch
from
d8c1996 to
85f64e2
Compare
timb07
changed the title
allow_empty flag
s-cooper18
left a comment
s-cooper18
left a comment
There was a problem hiding this comment.
I like the change - big tick from me
3 participants
Prior to this change, running psycopack on an empty table would cause the
TableIsEmptyexception to be raised.This change adds an
allow_emptyoption to allow empty tables to be handled without raising an exception. This enables psycopack to be used (e.g. for schema changes) irrespective of whether the table is empty or not.This PR has been changed following discussions in this thread; here's the previous description:
Prior to this change, running psycopack on an empty table would cause theTableIsEmptyexception to be raised.This change adds a check for a schema change (initially just PK bigint conversion) before checking for an empty table. This enables psycopack to be used for schema changes irrespective of whether the table is empty or not.