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

storage: add chain.evm_tokens token_name index #466

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

Conversation

pro-wh
Copy link
Collaborator

@pro-wh pro-wh commented Jun 27, 2023

from #460

This speeds up searching tokens by name. For simplicity, we're not merging this for now (2024-06) until search proves problematically slow.

@@ -309,6 +309,10 @@ func (c *Client) listIndexerMaterializedViews(ctx context.Context) ([]string, er

// Wipe removes all contents of the database.
func (c *Client) Wipe(ctx context.Context) error {
if _, err := c.pool.Exec(ctx, "DROP EXTENSION IF EXISTS pg_trgm CASCADE;"); err != nil {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this works out okay when it's at the beginning, because this way dropping the extension with cascade cleans up its types and functions

@@ -322,6 +326,7 @@ func (c *Client) Wipe(ctx context.Context) error {

// List, then drop all custom types.
// Query from https://stackoverflow.com/questions/3660787/how-to-list-custom-types-using-postgres-information-schema
// TODO: Don't delete extensions' types.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really a TODO, now that we're dropping the extension?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we can switch course to dropping all extensions (except for some)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see, so IIUC it's a TODO in the sense that if we added another extension, and that extension defined a custom type, and we chose not to use DROP EXTENSION IF EXISTS on it, we'd have to amend this code here so that it doesn't delete the extension's types.
In that hypothetical world, the same goes for other objects I guess? I don't know enough about postgres extensions but I imagine they can introduce functions and tables and whatnot.

Maybe this line can be something a little milder? Like NOTE: If you introduce postgres extensions with their own custom types, this will try to delete those types too.

I don't see it as a realistic TODO, i.e. I don't expect us to ever need to work on it in a principled way.

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.

Thanks!

I don't see us adding and/or removing extensions very frequently, so I think it's no big deal if there's a little friction in Wipe() around that.

Base automatically changed from pro-wh/feature/wipe to main June 30, 2023 22:14
@ptrus
Copy link
Member

ptrus commented Jun 13, 2024

Is this still relevant?

@pro-wh
Copy link
Collaborator Author

pro-wh commented Jun 14, 2024

I think we put it off until the search got slower

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.

Not sure how this fell through the cracks unapproved for so long. Thank you, LGTM!

Remaining request: I did find the TODO in the code very confusing again (I was reading this with fresh eyes now, a year after the first review), and would strongly prefer to see it reworded the way I suggested in the comment thread, or similarly.

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