Skip to content

DB Optimizations - Convert event_type to int. - #36

Open
mjiori wants to merge 17 commits into
qafrom
shrink-db-size
Open

DB Optimizations - Convert event_type to int.#36
mjiori wants to merge 17 commits into
qafrom
shrink-db-size

Conversation

@mjiori

@mjiori mjiori commented Sep 17, 2025

Copy link
Copy Markdown
Contributor

7/31/26 - The scope of the PR now contains only the conversion of event_type. Other optimizations have been left to other branches. All tests are passing.

This PR provides a combination of DB migrations and code changes to help address the rising size and cost of the Registry RDS database.

  • column removed: created_at
  • column removed: updated_at
  • premis_events.event_type has been fully converted from a varchar type to an int type. A new lookup table has been added to match ints to strings, so we don’t lose any data.
  • premis_events.object
  • premis_events.agent
  • storage URLs

The migrations are separated simply to make them easier to run on the large production database, and also in the service of separation of concerns.

Numerous changes were necessary in Registry and Preservation Services to accommodate these database changes.

Manual testing:

  • Ingest bag
  • Restore bag
  • Delete bag
  • View events (type, object, agent)

@diamondap

Copy link
Copy Markdown
Member

This looks good so far. One question: in the migration 013_shrink_db_size.sql, you add three lookup tables for event type, agent and object, and you create foreign key constraints in premis_events pointing to the lookup tables. Where/when do you actually populate the lookup tables? If the lookup tables aren't populated, it seems the migration will fail as the foreign keys aren't there.

@mjiori

mjiori commented Sep 19, 2025

Copy link
Copy Markdown
Contributor Author

This looks good so far. One question: in the migration 013_shrink_db_size.sql, you add three lookup tables for event type, agent and object, and you create foreign key constraints in premis_events pointing to the lookup tables. Where/when do you actually populate the lookup tables? If the lookup tables aren't populated, it seems the migration will fail as the foreign keys aren't there.

Correct, I have not added those INSERT statements to populate the lookup tables just yet. Will do.

@diamondap

Copy link
Copy Markdown
Member

This looks good. Two minor questions/issues:

  1. In constants.go, all the event types are currently set to zero. I assume you'll change that before merging?
  2. In premis_event.go, the Validate() method complains if event.Agent and event.Object are less than zero. It should probably complain if they are less than or equal to zero. By default, Go sets int values to zero, and zero doesn't match any of the event type values defined in db/migrations/013_shrink_db_size.sql. That means Validate() could easily accept empty Agent and Object values that don't map to anything in the DB.

@mjiori mjiori changed the title DRAFT: DB Optimizations DB Optimizations Dec 16, 2025
@mjiori
mjiori marked this pull request as ready for review December 16, 2025 07:34
@mjiori
mjiori requested a review from diamondap December 16, 2025 07:37
@mjiori
mjiori changed the base branch from master to qa December 16, 2025 07:47

@diamondap diamondap left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The db migration starts with number 013. There's another db migration in the authy removal or MFA branch that also has a migration starting with 013. Registry applies the migrations in order, so you may want to renumber one of these 013 migrations and be sure they don't conflict with each other. Hopefully, they can be applied in any order without stepping on each other.

@mjiori

mjiori commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

This looks good. Two minor questions/issues:

  1. In constants.go, all the event types are currently set to zero. I assume you'll change that before merging?
  2. In premis_event.go, the Validate() method complains if event.Agent and event.Object are less than zero. It should probably complain if they are less than or equal to zero. By default, Go sets int values to zero, and zero doesn't match any of the event type values defined in db/migrations/013_shrink_db_size.sql. That means Validate() could easily accept empty Agent and Object values that don't map to anything in the DB.
  1. Yes, that is changed.
  2. The Agent/Object conversion has been moved to another branch.

@mjiori
mjiori requested a review from diamondap July 31, 2026 14:39
@mjiori mjiori changed the title DB Optimizations DB Optimizations - Convert event_type to int. Jul 31, 2026

@diamondap diamondap left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In db/migrations/013_convert_event_type.sql, line 31, you insert a record into the event_type_lookup table with id zero and value 'unknown event'. Make sure that postgres actually accepts zero as a value in the id column. Id sequences may or may not start at 1. Also, I'm not sure you want this value in the table. If an event type resolves to zero in common/util.go - ConvertEventTypeToInt(), that means the system doesn't recognize it and we should probably not insert it into the db. I think the PremisEvent.Validate() method might reject it. If you want to keep event type zero in the lookup table just for reference purposes, that might be okay. Just don't save actual events with a type value of zero.

It's nice to see that you added icons for premis events.

In pgmodels/failed_fixity_summary.go line 18, where pev.event_type = 21, you should probably use constants.EventFixityCheck instead of hard-coding the number 21.

Otherwise, all looks good for a merge.

@mjiori

mjiori commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Fixed issue with the failed_fixity_summary.

I am keeping the 0 event type in the lookup table as a fallback, though we should never have any events actually inserted that have this type.

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.

2 participants