Closed gmile closed 5 months ago
@gmile you've stumbled on a bug.
How did you run the benchmarks. Were you running MySQL and Postgres locally vs running them in docker?
Overall thanks for bumping the benchmarks!
@warmwaffles to your question:
How did you run the benchmarks. Were you running MySQL and Postgres locally vs running them in docker?
I ran MySQL in a container, but PostgreSQL locally (natively, running the the host OS).
Your question made me realise its not fair to compare results of SQLite3 and PostgreSQL running on my computer natively with results for MySQL running inside a container, e.g. via virtualization which has a performance penalty. I'm using orbstack which claims to be faster the Docker, but still. I'll install MySQL on my mac natively and will regenerate the benchmark once again.
On a separately note, to clarify existence of commit https://github.com/elixir-sqlite/ecto_sqlite3/pull/144/commits/a9902f38ba8699a989eacd4b96b111aa49bbe361. Using docker image for MySQL 5.7 didn't work for me, because there's no arm64 OCI manifest generated for 5.7 version, but there's for 8.
I was able to reproduce the exception mentioned in my message with this self-contained Elixir script:
If I make a change to a script - it will no longer raise:
diff --git a/time-issue-repro.exs b/time-issue-repro.exs
index 8643d65..52e173e 100644
--- a/time-issue-repro.exs
+++ b/time-issue-repro.exs
@@ -38,7 +38,7 @@ defmodule Main do
Ecto.Migrator.run(Repo, [{0, Migration0}], :up, all: true, log_migrations_sql: :debug)
- Repo.load(Post, %{title: "New blog post", published_at: ~T[21:25:04.361140]})
+ Repo.load(Post, %{title: "New blog post", published_at: "21:25:04.361140")
end
end
@warmwaffles I added this commit https://github.com/elixir-sqlite/ecto_sqlite3/pull/144/commits/bceff9a7bf38518b875e41bc6c13f631e76598f2 just to see if I am able to get pass the issue when running the benchmark
It work, so not only "insert" part, but all benchmarks were regenerated successfully. However, I don't believe my fix is correct. For example, running benchmark having %Date{}
struct as a value appears to run just fine without any changes:
I am not sure why explicit handling of %Time{}
is necessary in principle 🤔
Sorry this took so long to merge. It completely slipped through on me. Feel free to ping mention me if you see another PR for these libraries fall through.
@warmwaffles noted, and no worries, I kind of forgot about this one too 🙈 Thanks for merging! I just hope this change is not going to break anything https://github.com/elixir-sqlite/ecto_sqlite3/pull/144/commits/bceff9a7bf38518b875e41bc6c13f631e76598f2 🙏
It shouldn't. You added a test for it and I think it was just an oversight by me initially.
This refreshes insertion benchmark stats + does a couple of minor fixes to the bench README.md.
I couldn't update all benchmarks due to a failure that I haven't had time to look into. Here's a full failure output: