-
Notifications
You must be signed in to change notification settings - Fork 899
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
Fix serializing of time/date columns using yaml serializer #1458
base: master
Are you sure you want to change the base?
Fix serializing of time/date columns using yaml serializer #1458
Conversation
ed4f74e
to
a4730dc
Compare
Ok, this is not working for MySQL ( |
@fatkodima I think that makes sense to me. I tested out with MySQL and using |
It sounds like we have insufficient test coverage, then?
We've had a few issues with YAML serialization over the years, and I've often thought of changing the default from YAML to JSON. In fact, I think the best choice is a You can also write a custom serializer. In fact, it would be great if you could try that first in your own app, and report back after a few months in production. Please see https://github.com/paper-trail-gem/paper_trail?tab=readme-ov-file#6b-custom-serializer |
We are currently running this patch in production for postgres for 4 months already. |
Oh, great! Which RDBMS are you using in production? I'm trying to understand the above statement "this is not working for MySQL" |
expect(attrs["created_at"]).to match(/2015/) | ||
else | ||
expect(attrs["created_at"].to_i).to eq(time.to_i) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a comment here explaining the difference between RDBMS?
@klass.connection.type_cast(serialized) | ||
else | ||
serialized | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems similar to TypeSerializers::PostgresArraySerializer
, in that it wraps a particular type (in this case Date, Time
). Should we follow that pattern, and have a TypeSerializers::DateTimeSerializer
?
a4730dc
to
5a6ff78
Compare
Updated with the suggestion, please take a look. |
This PR has been automatically marked as stale due to inactivity. |
Ping. |
This PR has been automatically marked as stale due to inactivity. |
We are currently using paper_trail and have billions of items in the
versions
table and the table is huge.I noticed, that for yaml serializer date/time objects are serialized as ruby objects. Something like
This generates 179 bytes per field.
But that should be serialized into the string. Something like
--- 2024-01-27 18:03:07 UTC
That is 28 bytes long, so 150 bytes difference.
Considering that most people have at least 2 datetime columns (
created_at
andupdated_at
) for each table, that saves 300 bytes per row in the table.For example, if we have a table with 4 billion rows, that is
4 * 10^9 * 300 / 10^9
= 1200 GB 😱 saved.master
(if not - rebase it).code introduces user-observable changes.
and description in grammatically correct, complete sentences.