feat: option to allow specifying the default precision for auto-generated timestamp columns

[papandreou]

Pull Request Checklist

• Have you added new tests to prevent regressions?
• If a documentation update is necessary, have you opened a PR to the documentation repository?
• Did you update the typescript typings accordingly (if applicable)?
• Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
• Does the name of your PR follow our conventions?

Background

I'm working on upgrading Sequelize from 6 to the 7 alphas in a rather big application that uses Postgres as its main database. Its test suite includes a "schema drift" checker that compares the result of sequelize.sync() against the previous schema with migrations applied. When we upgraded to Sequelize 7, this script caught hundreds of drifts because Sequelize 7 provisions the auto-generated createdAt/updatedAt/deletedAt columns as TIMESTAMP(6) WITH TIME ZONE, whereas with Sequelize 6 they came out as TIMESTAMP WITH TIME ZONE because no explicit precision was specified.

Reading up on the Postgres docs, it seems like the two type declarations mean the same, for all intents and purposes, also wrt. the SQL standard. That makes me think that the explicit precision slipped into #14505 by accident, so I suggest changing it back adding an option to override it in order to to limit the amount of unnecessary churn that users will run into when upgrading Sequelize 🤗

Description Of Change

Add a defaultTimestampPrecision option that can be specified per Sequelize instance.

Todos

• Fix tests

[WikiRik]

Your PR description talks about postgres but the tests you are updating are for mariadb/mysql. Is that correct?

[papandreou]

Your PR description talks about postgres but the tests you are updating are for mariadb/mysql. Is that correct?

Yeah, as far as I can tell, #14505 changed the precision for all dialects, so I think it makes sense to undo this part of the change everywhere too. It seems like the mariadb/mysql tests are just a bit more detailed. (I'm having trouble running the complete test suite locally, so I'm iterating a bit using your CI setup 😇 )

[WikiRik]

(I'm having trouble running the complete test suite locally, so I'm iterating a bit using your CI setup 😇 )

No worries, I do that quite often as well. Running integration tests on all dialects locally is not always worth the additional effort

[papandreou]

Ah, the failing mariadb/mysql indicate that with my change, DATETIME columns without an explicit precision will default to second precision.

The MySQL docs say:

The fsp value, if given, must be in the range 0 to 6. A value of 0 signifies that there is no fractional part. If omitted, the default precision is 0. (This differs from the standard SQL default of 6, for compatibility with previous MySQL versions.)

I guess that clears up why #14505 included this change in the first place 😅

Then my new point of view is that sequelize should only set an explicit precision for the mariadb/mysql dialects, as they're the ones diverging from the SQL standard 🤔

I don't know exactly how to accomplish that without breaking something else, so I think I'll need help with that! My alternative is to apply a migration that changes the type of 216 columns in my DB schema from timestamp with time zone to timestamp(6) with time zone. I would survive that, but I think it would be good to save everyone who's upgrading (and cares about schema drifts) the trouble of that.

[papandreou]

@ephys, I'd like to hear your input on this when you have some time 🤗

[ephys]

The thing is, we should likely set it to 3 by default instead of 6 or letting it default to 6, because the Date object's own precision only goes up to 3 :(

We'll need to wait for Temporal to have something capable of representing a precision of 6 decimal places

[papandreou]

Hey @ephys, thanks for your reply!

The thing is, we should likely set it to 3 by default instead of 6 or letting it default to 6, because the Date object's own precision only goes up to 3 :(

That makes sense, but maybe there could then be a way to override the default precision for long-time loyal users? ;)

If not, I can also live with that and just write that migration to change the types of those 216 columns from TIMESTAMP WITH TIME ZONE to TIMESTAMP(6) WITH TIME ZONE and move on, but in that case it would be nice to know that I would only have to do it once, and not have the default change again later.

(We've been using Postgres+Sequelize since version 2.1.3, and I can tell that ironically the auto-provisioned timestamp columns used to come out as TIMESTAMP(6) WITH TIME ZONE, probably some time before version 5.0.0. Then at some point they became TIMESTAMP WITH TIME ZONE, and now #14505 changed them back to TIMESTAMP(6) WITH TIME ZONE 😅 )

[papandreou]

I took the liberty of changing the PR to introduce a defaultTimestampPrecision option instead, so that the user can choose themselves, and also be immune to future changes of the default value 😇

[papandreou]

@ephys, sorry to ping you, but would you mind giving some kind of signal? I have an opportunity to change those 216 columns to TIMESTAMP(6) WITH TIME ZONE in a service window tonight. Would be nice to know if I should just jump on that, or if there's a chance that something like this PR can be landed.

[WikiRik]

I think that this is a good approach. As @ephys mentioned we might even want to set the default to 3 until Temporal is ready to be used

[papandreou]

@WikiRik okay, that sounds good, I'll hold off for a bit and not run any migrations for now 😇

[WikiRik]

Looks good to me. Let's keep the possible change of the default for a future PR