search

tompave / fun_with_flags

Feature Flags/Toggles for Elixir
https://hexdocs.pm/fun_with_flags/FunWithFlags.html
MIT License
1.08k stars 79 forks source link

Add timestamps to Ecto DB table #154

Closed PaulOstazeski closed 1 year ago

PaulOstazeski commented 1 year ago

This adds inserted_at and updated_at columns to the database table when using Ecto, and uses the built-in Ecto behavior to populate their values without any additional work. This makes it possible to directly measure how long a given flag has been in its current state, which in turn makes it easier to determine when a flag is safe to be removed (i.e. that it has been "on" for some/many/all users for "long enough" that we haven't observed any downsides).

ArgyleWerewolf commented 1 year ago

To me, this is good.

tylerbarker commented 1 year ago

Hey @PaulOstazeski, I've had a PR up here for quite some time so until @tompave returns I've created a Hex published fork. You're welcome to raise your pull request there and I'll check it out 🙂

tompave commented 1 year ago

Hey @PaulOstazeski, I'm afraid I have to revert this. (revert PR: https://github.com/tompave/fun_with_flags/pull/160)

This weekend I've been catching up on some long overdue maintenance work on this repo, and I think I merged this a bit too quickly. This was my mistake, and I apologize.

I'm also afraid that I forgot to run CI on this PR. Normally I have to approve GHA runs for PRs, but it usually presents me a big visible button on the PR page to do so. For some reasons it didn't for this PR. (It could be either because it's been too long since the PR was opened, or because in the meantime I updated the GHA workflow on the master branch.) I thought "sure, it seems safe, I can run it locally", and just merged it.

Unfortunately I came across a few issues after that. I think we need to address them before we can consider this to be merged. Normally I'd work on it myself and roll forward, but my time at the moment is limited, and I don't want to leave the master branch in a not-working state, sorry.

If you would like to give this another try, here are my observations after testing this locally:

  1. Elixir 1.15 won't translate anymore repo to repo(), as it raises a compiler error. I fixed it with https://github.com/tompave/fun_with_flags/pull/159 (later reverted).
  2. The migration doesn't contain a down function and it cannot be rolled back. I fixed it with https://github.com/tompave/fun_with_flags/commit/a4d8e329b75a09aea3ecf23dd470bbf753d0005e in my working branch.
  3. On the incremental migration: I wonder if you have only tested it in MySQL. I could apply your migration locally on MySQL, but I couldn't run mix ecto.migrate on Postgres. I think that the issue is that once an error is raised within the migration (the very thing that I thought was elegant 🙃), then the migration transaction is flagged as aborted, and every subsequent SQL statements fail. In order for this to be merged, it needs to work for Postgres too.
  4. Still on the incremental migration: I couldn't apply it on either MySQL or Postgres if the table already contained records. That's because Ecto's timestamps default to NOT NULL and the migration doesn't provide a default. There are two solutions for this:
    1. Provide a default value. However adding a column with a default value is a blocking operation on Postgres, and I believe it can be tricky on MySQL too unless some non-blocking DDL API is used. That's not a problem for small tables, but it can lock the table in production if there is too much data. I'd be very uncomfortable releasing a new minor version of the package where the upgrade instructions come with a warning that it might require a DB maintenance window. 😬
    2. An alternative on the above is to do it in multiple steps, which is a common workaround in Postgres: add the column, add a default value (or backfill it), make it NOT NULL later. It's just more complex to rollout and to automate within the package.
    3. Make the timestamp columns nullable, so that the problem goes away. I was exploring this, but then I realized that I was still blocked by point 3 on Postgres, mentioned above.

On the last two points, I suppose that my main concern is: if I add this functionality to the package, then the upgrade path should be smooth and painless. If it requires too many manual steps, and it comes with risks, then I'd be more inclined to add it to the next major version, rather than a minor version bump.

I hope it makes sense!

PaulOstazeski commented 1 year ago

Thanks @tompave, I definitely missed some subtleties here. I'll take another crack at it and see if I can address these.