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

Protect from concurrent update schema commands #2751

Closed
lmsurpre opened this issue Sep 9, 2021 · 4 comments
Closed

Protect from concurrent update schema commands #2751

lmsurpre opened this issue Sep 9, 2021 · 4 comments
Assignees
Labels
enhancement New feature or request P2 Priority 2 - Should Have schema-change a schema change

Comments

@lmsurpre
Copy link
Member

lmsurpre commented Sep 9, 2021

Is your feature request related to a problem? Please describe.
In Alvearie/alvearie-helm#27 we were hoping to move from helm hooks (once and only once execution) to init containers (runs for each pod) so that we can add a PostgreSQL subchart.

I have two main concerns with that:

  1. multiple pods start at the same time and i'm not super confident that we can safely execute the schema tool in parallel in all cases (and its hard to test for)
  2. it makes pod startup slower

Describe the solution you'd like

  1. create a new table in the FHIR_ADMIN schema for tracking schema upgrades
  2. when a schema action begins (anything other than "create-schema" I think), write a single row to this table to represent its "lease"
  3. include an expiration timestamp in the row and update the schema tool to move that timestamp forward while its running (it should never expire while upgrade is still running)
  4. before executing a schema-update (and other actions?) ensure that there is no lease row; or if there is one, ensure that the lease is expired. otherwise, exit with a successful status.

Describe alternatives you've considered
At first I was thinking we could use a simple table lock on the FHIR_ADMIN.VERSION_HISTORY table, but because we use multiple transactions for some schema updates, we don't think this is quite sufficient.

Acceptance Criteria

  1. GIVEN [a precondition]
    AND [another precondition]
    WHEN [test step]
    AND [test step]
    THEN [verification step]
    AND [verification step]

Additional context
The described solution addresses my first concern, which is the most important one.
However, it would also be great if we could cover my second concern with is the slow pod startup.
Therefor, it would be great if we could add the overall schema version to this table. Then we could do logic like this:

  1. once the schema-update is successful, mark this "lease" row as "completed"
  2. when doing an upgrade, if there is a completed lease row for this version of the schema (or higher), skip the update
@d0roppe
Copy link
Collaborator

d0roppe commented Oct 28, 2021

Moving back to in-progress as there is an issue with the whole transaction ending.

@punktilious
Copy link
Collaborator

Issue is with PostgreSQL hitting a duplicate row and needing its own statement syntax on conflict do nothing to avoid killing the transaction.

@d0roppe
Copy link
Collaborator

d0roppe commented Oct 31, 2021

Moving back to in progress as the new tables used for this is not removed with the drop-schema part of the fhir-persistence-schema-cli tool

@d0roppe
Copy link
Collaborator

d0roppe commented Nov 5, 2021

Verified that issuing multiple change schemas at the same time, they will not interfere with each other, 1 will do the work while the other will time out and you can try again.

@d0roppe d0roppe closed this as completed Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request P2 Priority 2 - Should Have schema-change a schema change
Projects
None yet
Development

No branches or pull requests

4 participants