Page MenuHomeSoftware Heritage
Differential D3802

pg: Make date_neg_utc_offset is not null if date is not null.
ClosedPublic
Actions

Authored by vlorentz on Aug 19 2020, 12:45 PM.

Details

Reviewers
olasd
anlambert
Group Reviewers
Reviewers
Commits
rDSTOe2b14948da14: pg: Make date_neg_utc_offset is not null if date is not null.
Summary

Fixes a regression in 038a219f84d6b8a4f02b48f9ad3c5d823d097790,
as it made converters expect a boolean.

We stopped writing this kind of nulls since we started using model
objects for insertions, but didn't migrate existing data.

This commit migrates these nulls to false, then adds a constraint
to enforce it in the future.

Diff Detail

Event Timeline

vlorentz created this revision.Aug 19 2020, 12:45 PM
swh-public-ci added a comment.Aug 19 2020, 12:49 PM

Build is green

Patch application report for D3802 (id=13404)

Rebasing onto bd925475d9...

Current branch diff-target is up to date.
Changes applied before test
commit 2d7980d784c05b6550b0ede86546c391d45c45c1
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Aug 19 12:40:01 2020 +0200

    pg: Make date_neg_utc_offset is not null if date is not null.
    
    Fixes a regression in 038a219f84d6b8a4f02b48f9ad3c5d823d097790,
    as it made converters expect a boolean.
    
    We stopped writing this kind of nulls since we started using model
    objects for insertions, but didn't migrate existing data.
    
    This commit migrates these nulls to false, then adds a constraint
    to enforce it in the future.

See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/818/ for more details.

Harbormaster completed remote builds in B14668: Diff 13404.Aug 19 2020, 12:49 PM
anlambert requested changes to this revision.Aug 19 2020, 1:29 PM

You also need to increase db version in 30-swh-schema.sql file. Could you also add tests for the new constraints ?

sql/upgrades/160.sql
22–24

I do not get that part. Why checking for non null offset ?

This revision now requires changes to proceed.Aug 19 2020, 1:29 PM
vlorentz updated this revision to Diff 13405.Aug 19 2020, 1:32 PM

update schema version

vlorentz added a comment.Aug 19 2020, 1:33 PM
In D3802#94066, @anlambert wrote:

Could you also add tests for the new constraints ?

It's impossible to trigger them because it would mean building a model.TimestampWithTimezone object with a None value for neg_utc_offset, which attrs-strict doesn't allow.

sql/upgrades/160.sql
22–24

I don't understand the question. It's an integrity check, like all other "not null" constraints

swh-public-ci added a comment.Aug 19 2020, 1:36 PM

Build is green

Patch application report for D3802 (id=13405)

Rebasing onto bd925475d9...

Current branch diff-target is up to date.
Changes applied before test
commit 131d156469724e07dd34efc5a47873c1b63097f5
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Aug 19 12:40:01 2020 +0200

    pg: Make date_neg_utc_offset is not null if date is not null.
    
    Fixes a regression in 038a219f84d6b8a4f02b48f9ad3c5d823d097790,
    as it made converters expect a boolean.
    
    We stopped writing this kind of nulls since we started using model
    objects for insertions, but didn't migrate existing data.
    
    This commit migrates these nulls to false, then adds a constraint
    to enforce it in the future.

See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/819/ for more details.

Harbormaster completed remote builds in B14669: Diff 13405.Aug 19 2020, 1:37 PM
anlambert added a comment.Aug 19 2020, 1:54 PM
In D3802#94074, @vlorentz wrote:
In D3802#94066, @anlambert wrote:

Could you also add tests for the new constraints ?

It's impossible to trigger them because it would mean building a model.TimestampWithTimezone object with a None value for neg_utc_offset, which attrs-strict doesn't allow.

Fair enough. Do we really need those constraints then ?

sql/upgrades/160.sql
22–24

Ok got it, I was mislead by the not valid keywords and I forgot we can have null dates.

vlorentz added a comment.Aug 19 2020, 1:56 PM
In D3802#94078, @anlambert wrote:

Fair enough. Do we really need those constraints then ?

We don't need them, but it prevents integrity issues later. We already have a bunch of not null where it's relevant, and even a check ((date is null) or (author is not null)) on release for the same reason.

anlambert added a comment.Aug 19 2020, 2:03 PM
In D3802#94080, @vlorentz wrote:
In D3802#94078, @anlambert wrote:

Fair enough. Do we really need those constraints then ?

We don't need them, but it prevents integrity issues later. We already have a bunch of not null where it's relevant, and even a check ((date is null) or (author is not null)) on release for the same reason.

Ack. Until the data got migrated, could you also handle the None to False conversion in the converters.db_to_date function ?

anlambert accepted this revision.Aug 19 2020, 2:12 PM
This revision is now accepted and ready to land.Aug 19 2020, 2:12 PM
vlorentz updated this revision to Diff 13412.Aug 19 2020, 2:57 PM

rebase

swh-public-ci added a comment.Aug 19 2020, 3:09 PM

Build is green

Patch application report for D3802 (id=13412)

Could not rebase; Attempt merge onto bd925475d9...

Updating bd925475..e2b14948
Fast-forward
 sql/upgrades/160.sql               | 41 ++++++++++++++++++++++++++++++++++++++
 swh/storage/backfill.py            |  1 +
 swh/storage/converters.py          |  6 +++++-
 swh/storage/sql/30-swh-schema.sql  |  2 +-
 swh/storage/sql/60-swh-indexes.sql | 22 ++++++++++++++++++++
 5 files changed, 70 insertions(+), 2 deletions(-)
 create mode 100644 sql/upgrades/160.sql
Changes applied before test
commit e2b14948da143925b0421415580df63e48d18a5b
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Aug 19 12:40:01 2020 +0200

    pg: Make date_neg_utc_offset is not null if date is not null.
    
    Fixes a regression in 038a219f84d6b8a4f02b48f9ad3c5d823d097790,
    as it made converters expect a boolean.
    
    We stopped writing this kind of nulls since we started using model
    objects for insertions, but didn't migrate existing data.
    
    This commit migrates these nulls to false, then adds a constraint
    to enforce it in the future.

commit 7dcd5708b0cffdacd0082288262ccc4d78cec2de
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Aug 19 14:06:21 2020 +0200

    converters: convert neg_utc_offset to False if it is None.

commit e2f066549123f069bf0ae34d593038ffd834d9c4
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Aug 19 14:54:45 2020 +0200

    backfiller: Add missing 'extra_header' field.
    
    This field wasn't backfilled; and it wasn't caught by tests, because
    revisions in swh.journal.tests.journal_data.TEST_OBJECTS were had
    an empty extra_header field.
    
    Starting with swh-journal v0.4.3, one of the revisions in TEST_OBJECTS
    has a non-empty extra_header field, so it will become a test failure
    without this commit

See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/825/ for more details.

Harbormaster completed remote builds in B14676: Diff 13412.Aug 19 2020, 3:09 PM
Closed by commit rDSTOe2b14948da14: pg: Make date_neg_utc_offset is not null if date is not null. (authored by vlorentz). · Explain WhyAug 20 2020, 8:51 AM
This revision was automatically updated to reflect the committed changes.
vlorentz added a commit: rDSTOe2b14948da14: pg: Make date_neg_utc_offset is not null if date is not null..

Revision Contents
Changeset List

PathSize
sql/
upgrades/
41 lines
swh/
storage/
sql/
2 lines
22 lines

Diff 13412

View Options

sql/upgrades/160.sql

Loading...
View Options

swh/storage/sql/30-swh-schema.sql

Loading...
View Options

swh/storage/sql/60-swh-indexes.sql

Loading...
About · Developer information · Legal