Skip to content
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

consolidate migrations #574

Merged
merged 3 commits into from
Dec 12, 2023
Merged

consolidate migrations #574

merged 3 commits into from
Dec 12, 2023

Conversation

Andrew7234
Copy link
Collaborator

@Andrew7234 Andrew7234 commented Nov 29, 2023

Task

Once the runtime stuff is largely stable, and before (or after) we start mucking with consensus:

Group DB migrations into a smaller set of files.

This comes at the cost of later not being able to (trivially) migrate an arbitrarily old DB ... but since we think it's unlikely there are instances of nexus out in the wild, that's OK.

This PR

Completes the tas.

Functionally a no-op. It would be nice to do a full reindex once this goes in, however, future deployments will continue to work since the current deployed databases have much higher 'current migration' number. However, any future migrations applied on top of this will also not apply until a reindex is done.

Validation:
Comparison of the dumped database schema (using pg_dump -s) for main branch / feature branch:

~/oasis/oasis-block-indexer(andrew7234/consolidate-migrations*) » diff main.sql new.sql                                                andrewlow@Beleriand

Explanation: function was removed since it was no longer used
218,230d217
< --
< -- Name: floor_5min(timestamp with time zone); Type: FUNCTION; Schema: public; Owner: rwuser
< --
<
< CREATE FUNCTION public.floor_5min(ts timestamp with time zone) RETURNS timestamp with time zone
<     LANGUAGE sql IMMUTABLE
<     AS $_$
<     SELECT date_trunc('hour', $1) + date_part('minute', $1)::int / 5 * '5 minutes'::interval;
< $_$;
<
<
< ALTER FUNCTION public.floor_5min(ts timestamp with time zone) OWNER TO rwuser;
<

Explanation: different ordering of columns in evm_nfts table
515a503,505
>     owner public.oasis_addr,
>     num_transfers integer DEFAULT 0 NOT NULL,
>     metadata jsonb,
520,523c510
<     image text,
<     owner public.oasis_addr,
<     num_transfers integer DEFAULT 0 NOT NULL,
<     metadata jsonb
---
>     image text

Explanation: different ordering of columns within a table
669a657
>     "timestamp" timestamp with time zone NOT NULL,
676d663
<     "timestamp" timestamp with time zone NOT NULL,

Explanation: pkey constraint on main is named differently since the table originally had a different name. No effect
962c949
< -- Name: evm_token_balances evm_token_balance_analysis_pkey; Type: CONSTRAINT; Schema: analysis; Owner: rwuser
---
> -- Name: evm_token_balances evm_token_balances_pkey; Type: CONSTRAINT; Schema: analysis; Owner: rwuser
966c953
<     ADD CONSTRAINT evm_token_balance_analysis_pkey PRIMARY KEY (runtime, token_address, account_address);
---
>     ADD CONSTRAINT evm_token_balances_pkey PRIMARY KEY (runtime, token_address, account_address);

@Andrew7234 Andrew7234 force-pushed the andrew7234/consolidate-migrations branch from 98ad2a3 to 1ccad3c Compare November 29, 2023 05:48
@ptrus
Copy link
Member

ptrus commented Nov 29, 2023

However, any future migrations applied on top of this will also not apply until a reindex is done.

Cant we just manually update the schema version (to 0) on live deployments and avoid reindexing?

@mitjat
Copy link
Contributor

mitjat commented Nov 29, 2023

However, any future migrations applied on top of this will also not apply until a reindex is done.

Cant we just manually update the schema version (to 0) on live deployments and avoid reindexing?

Probably not to 0, or the migration library will try to apply the migrations from this PR. I believe we need update schema_migrations set version=5; because this PR reduces the number of migrations to 0..5.

But yeah, +1 to doing that so that future migrations can run smoothly. Maybe otherwise the migrations lib might even complain because it might try down-migrations (from the current 24 down to 5) and realize it doesn't have reverse-migration scripts? Anyway, let's just do surgery. That's what staging is for :)

Copy link
Contributor

@mitjat mitjat left a comment

Choose a reason for hiding this comment

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

Thank you for the validation and the listing+explanation of diffs! Makes for an easy LGTM :)

Also, God's work :)

@mitjat
Copy link
Contributor

mitjat commented Nov 29, 2023

Oh when cutting a release, we should call special attention to this. We should also bump the minor version because this is a backwards-incompatible DB schema change.

@mitjat
Copy link
Contributor

mitjat commented Nov 29, 2023

Note: #573 brings another small migration. Something to keep an eye on in case it goes in first.

@Andrew7234 Andrew7234 force-pushed the andrew7234/consolidate-migrations branch from 1ccad3c to 097b141 Compare December 12, 2023 06:43
@Andrew7234 Andrew7234 merged commit 4745858 into main Dec 12, 2023
6 checks passed
@Andrew7234 Andrew7234 deleted the andrew7234/consolidate-migrations branch December 12, 2023 17: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.

3 participants