Skip to content

[db] MySQL: Set --explicit-defaults-for-timestamp=OFF #4226

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

Merged
merged 3 commits into from
May 17, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions chart/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,12 @@ mysql:
fullnameOverride: mysql
image:
tag: 5.7
primary:
extraEnvVars:
# We rely on this in our DB implementations: NULL (re-)sets configured columns to be initialized with CURRENT_TIMESTAMP.
# OFF is the default as documented [here](https://dev.mysql.com/doc/refman/5.7/en/server-system-variables.html#sysvar_explicit_defaults_for_timestamp) (we also see this in GCP), but not for this chart.
- name: MYSQL_EXTRA_FLAGS
value: --explicit-defaults-for-timestamp=OFF
auth:
existingSecret: db-password
serviceAccount:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,15 @@ export class DBAppInstallation implements AppInstallation {
@Column({
type: 'timestamp',
precision: 6,
default: () => 'CURRENT_TIMESTAMP(6)',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question:

Suggested change
default: () => 'CURRENT_TIMESTAMP(6)',
default: () => 'CURRENT_TIMESTAMP(6) ON UPDATE CURRENT_TIMESTAMP(6)',

transformer: Transformer.MAP_ISO_STRING_TO_TIMESTAMP_DROP
})
creationTime: string;

@Column({
type: 'timestamp',
precision: 6,
default: () => 'CURRENT_TIMESTAMP(6) ON UPDATE CURRENT_TIMESTAMP(6)',
transformer: Transformer.MAP_ISO_STRING_TO_TIMESTAMP_DROP
})
lastUpdateTime: string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export class DBLayoutData implements LayoutData {
@Column({
type: 'timestamp',
precision: 6,
default: () => 'CURRENT_TIMESTAMP(6) ON UPDATE CURRENT_TIMESTAMP(6)',
transformer: Transformer.MAP_ISO_STRING_TO_TIMESTAMP_DROP
})
lastUpdatedTime: string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export class DBLicenseKey {
@Column({
type: 'timestamp',
precision: 6,
default: () => 'CURRENT_TIMESTAMP(6)',
transformer: Transformer.MAP_ISO_STRING_TO_TIMESTAMP_DROP
})
installationTime: string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export class DBPrebuiltWorkspace implements PrebuiltWorkspace {
@Column({
type: 'timestamp',
precision: 6,
default: () => 'CURRENT_TIMESTAMP(6)',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here:

Suggested change
default: () => 'CURRENT_TIMESTAMP(6)',
default: () => 'CURRENT_TIMESTAMP(6) ON UPDATE CURRENT_TIMESTAMP(6)',

transformer: Transformer.MAP_ISO_STRING_TO_TIMESTAMP_DROP
})
creationTime: string;
Expand Down
1 change: 1 addition & 0 deletions components/gitpod-db/src/typeorm/entity/db-snapshot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export class DBSnapshot implements Snapshot {
@Column({
type: 'timestamp',
precision: 6,
default: () => 'CURRENT_TIMESTAMP(6)',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the ON UPDATE CURRENT_TIMESTAMP(6) also be part of this?

Suggested change
default: () => 'CURRENT_TIMESTAMP(6)',
default: () => 'CURRENT_TIMESTAMP(6) ON UPDATE CURRENT_TIMESTAMP(6)',

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

creationTime should only be updated on creation I guess, not when the row get's updated (that's what ON UPDATE CURRENT_TIMESTAMP does).

TBH I did not - but propably should have - cross-verified this with the actual DB columns but judged from the meaning....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geropl I'm happy to align this the other way around then, by removing the ON UPDATE CURRENT_TIMESTAMP(6) from the DB where you don't set them.

Would that make sense?

transformer: Transformer.MAP_ISO_STRING_TO_TIMESTAMP_DROP
})
creationTime: string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export class DBUserMessageViewEntry {
@Column({
type: 'timestamp',
precision: 6,
default: () => 'CURRENT_TIMESTAMP(6)',
transformer: Transformer.MAP_ISO_STRING_TO_TIMESTAMP_DROP
})
viewedAt: string;
Expand Down
8 changes: 6 additions & 2 deletions components/gitpod-db/src/typeorm/transformer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,11 @@ export namespace Transformer {

export const MAP_ISO_STRING_TO_TIMESTAMP_DROP: ValueTransformer = {
to(value: any): any {
// DROP all input values as they are set by the DB 'ON UPDATE'/ as default value
// DROP all input values as they are set by the DB 'ON UPDATE'/ as default value.
// We're relying on the system variable explicit-defaults-for-timestamp here (link: https://dev.mysql.com/doc/refman/5.7/en/server-system-variables.html#sysvar_explicit_defaults_for_timestamp):
// `undefined` gets converted to NULL, which on the DB is turned into the configured default value for the column.
// In our case, that's 100% CURRENT_TIMESTAMP.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd prefer being super-explicit here, in order to not leave any room for confusion

Suggested change
// In our case, that's 100% CURRENT_TIMESTAMP.
// In our case, that's 100% CURRENT_TIMESTAMP(6).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think precision 6 we only have for part of them, no?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// This was done initially so we don't have to make fields like `creationTime` optional, or have to use two types for the same table.
return undefined;
},
from(value: any): any {
Expand Down Expand Up @@ -72,4 +76,4 @@ export class CompositeValueTransformer implements ValueTransformer {
from(value: any): any {
return this.upper.from(this.lower.from(value));
}
}
}