Skip to content

[db] Change the primary key of the d_b_oauth_auth_code_entry table #13583

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

Closed
wants to merge 4 commits into from

Conversation

andrew-farries
Copy link
Contributor

@andrew-farries andrew-farries commented Oct 4, 2022

Description

Following on from #13524 which added the uid column to the d_b_oauth_auth_code_entry table and #13577 which ensured that all new records set the uid field, this PR changes to the primary key to the uid column (and renames it to id).

Related Issue(s)

Part of #9198

How to test

  1. Run the local-companion app local-app against the preview environment:
GITPOD_HOST=https://af-make-uib1daf36a03.preview.gitpod-dev.com ./local-app test
  1. Run through the auth flow.
  2. See that the new rows in the d_b_oauth_auth_code_entry table have their id column set.

Release Notes

NONE

Documentation

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide

@andrew-farries andrew-farries requested a review from a team October 4, 2022 14:25
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Oct 4, 2022
@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-af-make-uid-column-pk.16 because the annotations in the pull request description changed
(with .werft/ from main)

Copy link
Member

@easyCZ easyCZ left a comment

Choose a reason for hiding this comment

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

What do we expect to happen when the migrations take place but existing instances are still running with PK set previously? I believe this will work fine, unless the @PrimaryGeneratedColumn() affects how the values are determined (and possibly generated)

/hold for Q

@andrew-farries
Copy link
Contributor Author

What do we expect to happen when the migrations take place but existing instances are still running with PK set previously? I believe this will work fine, unless the @PrimaryGeneratedColumn() affects how the values are determined (and possibly generated)

Not sure I understand the question. Do you mean is it possible to perform this key switch against a real table with data? If so, this migration has been run against such a table as part of testing this.

@easyCZ
Copy link
Member

easyCZ commented Oct 5, 2022

When the migrations run, the application that's running will still have the @PrimaryGeneratedColumn() annotation - is this a problem? This could result in a generated value being written to the id property.

@andrew-farries
Copy link
Contributor Author

I think the table is infrequently used enough that we can take the chance of a write to the table happening during the migration. I'm not sure how we would avoid the problem anyway, actually.

@geropl
Copy link
Member

geropl commented Oct 5, 2022

I think the table is infrequently used enough that we can take the chance of a write to the table happening during the migration.

I generally agree with the sentiment that we have compare effort and value. But we have to take into account that things happen, e.g.:

  • one of our assumptions about transactionality of the migration is wrong, and it fails mid-flight, leaving it up to us to fix the situation manually
  • this happens to get rolled out together with another, later migration that takes a lot of time (say, ~15mins) to execute

In both situations, suddenly the old code is broken because of this, causing an incident.

I'm not sure how we would avoid the problem anyway, actually.

I think we could decouple the migration from the code change, no? In a way where we:

  1. fill both, uid and id from code
  2. roll out the PK change (by setting id = uid instead of rename, or doing a three-way-swap), and stop filling uid in code
  3. drop the old uid column altogether

Might be a little bit more work, but would make me feel much safer. 👍

@andrew-farries
Copy link
Contributor Author

Closing this in favour of the safer approach mentioned by @gero in this comment.

#13616 is the first step towards that.

@andrew-farries andrew-farries deleted the af/make-uid-column-pk branch October 5, 2022 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants