Skip to content

Conversation

@Leitsi
Copy link
Contributor

@Leitsi Leitsi commented Apr 3, 2023

This change is Reviewable

Copy link
Contributor

@culka culka left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @Leitsi)


migrations/hsl/default/1680267332835_add-scheduled-stop-point-numeric-id/up.sql line 10 at r2 (raw file):

-- Reset the sequence, so first id will be 7000001.
SELECT setval('service_pattern.scheduled_stop_point_numeric_id_seq', 7000000);

This sounds potentially very bad.

Luckily we can currently ignore pre-existing values.


migrations/hsl/default/2000000001002_R_after_migrate_scheduled_stop_point/up.sql line 2 at r2 (raw file):

ALTER TABLE service_pattern.scheduled_stop_point
  ADD CONSTRAINT service_pattern_scheduled_stop_point_numeric_id_range_check CHECK (1 <= numeric_id AND numeric_id <= 9999999);

I like BETWEEN. But it does the same thing.


scripts/specific-schema.sh line 1 at r2 (raw file):

#!/usr/bin/env bash

This file probably should not be included

@Leitsi Leitsi force-pushed the add-numeric-id-fields branch from 516192a to 5d16539 Compare April 3, 2023 13:10
Copy link
Contributor Author

@Leitsi Leitsi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @culka)


migrations/hsl/default/1680267332835_add-scheduled-stop-point-numeric-id/up.sql line 10 at r2 (raw file):

Previously, culka (Teemu Mäkinen) wrote…

This sounds potentially very bad.

Luckily we can currently ignore pre-existing values.

Discussed this elsewhere.

Proceeding with this. Other possible solutions that we could think of feel more complex than this legacy-support-field deserves.


migrations/hsl/default/2000000001002_R_after_migrate_scheduled_stop_point/up.sql line 2 at r2 (raw file):

Previously, culka (Teemu Mäkinen) wrote…

I like BETWEEN. But it does the same thing.

👍 changed.

Copy link
Contributor Author

@Leitsi Leitsi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @culka)


scripts/specific-schema.sh line 1 at r2 (raw file):

Previously, culka (Teemu Mäkinen) wrote…

This file probably should not be included

Fixed now that this is rebased correctly.

@Leitsi Leitsi force-pushed the add-numeric-id-fields branch from 5d16539 to 970b311 Compare April 6, 2023 18:00
Copy link
Contributor

@jarkkoka jarkkoka left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 4 files at r2, 2 of 2 files at r3, 11 of 11 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @culka and @Leitsi)


migrations/hsl/default/1680785812130_set_jore3_importer_tables_ownership/up.sql line 17 at r4 (raw file):

ALTER TABLE route.line OWNER TO dbimporter;
ALTER TABLE service_pattern.vehicle_mode_on_scheduled_stop_point OWNER TO dbimporter;
ALTER TABLE service_pattern.scheduled_stop_point OWNER TO dbimporter;

Hmmm... I'm wondering whether everything works without changing ownership of service_pattern.scheduled_stop_point_invariant table which is written via triggers.

Code quote:

ALTER TABLE service_pattern.scheduled_stop_point OWNER TO dbimporter;

Copy link
Contributor

@culka culka left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Leitsi)

Copy link
Contributor Author

@Leitsi Leitsi left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jarkkoka)


migrations/hsl/default/1680785812130_set_jore3_importer_tables_ownership/up.sql line 17 at r4 (raw file):

Previously, jarkkoka (Jarkko Kaura) wrote…

Hmmm... I'm wondering whether everything works without changing ownership of service_pattern.scheduled_stop_point_invariant table which is written via triggers.

Should work. The only really necessary thing here are the scheduled_stop_point changes, they were required for truncates work with sequences. Changed others as well for consistency. At least the scheduled_stop_point_invariant is not truncated by the importer, even via cascades.

@Leitsi Leitsi force-pushed the add-numeric-id-fields branch 2 times, most recently from 12384cb to 0d78c84 Compare April 17, 2023 10:26
Leitsi added 7 commits April 17, 2023 14:34
…user

Mainly so that the importer can reset sequences on truncate.
To be used for some exports instead of UUIDs.
Shorter, and now more consistent with other similar constraints.
…s to HSL schema

Separate commit to better highlight actual modifications in next one.
@Leitsi Leitsi force-pushed the add-numeric-id-fields branch from 0d78c84 to fa3d1c7 Compare April 17, 2023 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants