Conversation
|
Is this related to this issue: #2080. I see they share the same JIRA. |
| # This is an IDEMPOTENT change - it can be backported, because applying it | ||
| # multiple times is harmless. |
There was a problem hiding this comment.
What branches are you thinking of backporting to? I thought the problem with backporting migrations was the divergence between the sequences.
There was a problem hiding this comment.
There are several possible issues with backporting migrations, and "not idempotent" is the biggest one. The comment is a reminder that this change doesn't have that problem.
Backporting this will definitely require a hand-backport, since the "previous migration" in its dependency-list won't be in older branches. It'll still work though. If, say, in container/2.20 we mark this as 0040_bigint_sequences.py, dependent on 0039, and an installation then updates to 2.28 - 0040_add_remote_repo_filter.py will apply, and then and eventually 0047_bigint_sequences will REapply, do nothing, and be fine.
There was a problem hiding this comment.
I'm almost certain this will still run into a Django error about inconsistent migration history down the road.
What if we execute that SQL (if necessary) as part of a post_migration hook?
Do we not need to change the Django class of the field too? (Or is that impossible, since we don't have the model of the through table?)
There was a problem hiding this comment.
We can change the default that Django chooses for AutoFields - see https://docs.djangoproject.com/en/4.2/ref/settings/#default-auto-field. But that only helps for future-models.
It's not that we don't have a Model - we do, it's (for this particular problem) BlobManifest. The problem is that that Model doesn't come with a "primary key" - just a multi-field uniqueness-constraint. In Django5.2, we can define multi-key PKs (see https://docs.djangoproject.com/en/6.0/topics/composite-primary-key/), which will make the point moot. Some day.
I can definitely investigate a post-migration hook - would make backporting easier/safer. Drafting this PR while I experiment.
| ] | ||
|
|
||
| operations = [ | ||
| migrations.RunSQL(update_sequences_to_bigint), |
There was a problem hiding this comment.
Should there be a reverse_sql specified? What about elidable?
There was a problem hiding this comment.
It's not elidable (unless/until we take one of the paths mentioned in the hackmd I've been writing up).
Reversing the change is only possible if done before the sequence hits 2G - if its bigger than that, then trying to get from BIGINT back to INTEGER is going to (at best) cause problems. Generally speaking, this should never be reversed.
|
@ggainey @gerrod3 going forwards, how do we ensure that non-BaseModel-derived models use the correct sequence type? We can update these specific fields to BigAutoField, but we should probably also ensure that new such fields are set to BigAutoField by default via: And/or migrate to UUID pks via BaseModel |
| # multiple times is harmless. | ||
| update_sequences_to_bigint = """ | ||
| ALTER SEQUENCE container_blobmanifest_id_seq AS BIGINT; | ||
| ALTER SEQUENCE container_manifestlistmanifest_id_seq AS BIGINT; |
There was a problem hiding this comment.
Does Django still recognize this as AutoField, or does it see it as BigAutoField? I suppose even if the former is true it still works?
|
For what this is, as long as it doesn't confuse Django either now or for a future BigAutoField migration, LGTM |
ggainey
force-pushed
the
pulp-890
branch
2 times, most recently
from
5b1da84 to
8271b39
Compare
4 participants
This addresses the failure noted in https://issues.redhat.com/browse/PULP-890 .