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

Identity #421

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Identity #421

wants to merge 13 commits into from

Conversation

codabrink
Copy link
Contributor

No description provided.

@codabrink codabrink changed the title Coda/identity Identity Mar 10, 2025
@codabrink codabrink marked this pull request as ready for review March 11, 2025 19:09
@codabrink codabrink requested a review from a team as a code owner March 11, 2025 19:09
SET
NOT NULL;

CREATE INDEX idx_address_log_identifier_inbox_id ON address_log (identifier, identifier_kind, inbox_id);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@neekolas Do you think removing and adding a new index on this table will be an issue?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it's a huge deal because the table is relatively small. But it will block the node from coming up until it's completed.

Would be better if we could add the index concurrently, but IIRC we get errors doing that because it happens in a transaction

Copy link
Contributor

Choose a reason for hiding this comment

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

CREATE INDEX CONCURRENTLY is what you might want. It takes twice as long, but it does not block concurrent transactions. I don't know whether the newest version of PG still releases the table lock and commits the ongoing transaction implicitly or not.

Since its a secondary non-unique index, you don't really care if it does not exist for a period of time. It will just make some queries slower.

Copy link
Collaborator

Choose a reason for hiding this comment

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

From my recollection we can't use it in our migrations because the migration runner wraps everything in a transaction, and transactions can't create indexes concurrently. But my knowledge of this codebase is very out of date and maybe the issue is resolved.

You can try it. If the issue is still there every test that touches the DB will fail.

Comment on lines +12 to +20
-- Default all of the existing identifier_kinds to 1 (Ethereum)
UPDATE address_log
SET
identifier_kind = 1;

ALTER TABLE address_log
ALTER COLUMN identifier_kind
SET
NOT NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know the internals of PG well enough to remember whether alter table holds an exclusive table lock or not. If an insert happens between the UPDATE and the ALTER, it will have a null column. There has to be a way to do it in one statement...

Copy link
Contributor

@mkysel mkysel Mar 11, 2025

Choose a reason for hiding this comment

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

I would do it like this

ALTER TABLE address_log ADD COLUMN identifier_kind INT DEFAULT 1;
UPDATE address_log SET identifier_kind = 1 WHERE identifier_kind IS NULL;
ALTER TABLE address_log ALTER COLUMN identifier_kind SET NOT NULL;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I did write this with the assumption that the migration holds an exclusive lock. I can just be safe and use your approach.

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