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

fix: add composite index for summing payments #140

Closed
wants to merge 2 commits into from

Conversation

rolznz
Copy link
Contributor

@rolznz rolznz commented Sep 21, 2023

fixes #140
improves performance of query to sum payments in current budget period

@rolznz rolznz requested review from bumi and kiwiidb September 21, 2023 14:35
@bumi
Copy link
Contributor

bumi commented Sep 22, 2023

Will this update the current DB, too? is this a migration?
I still think we need a combined index on app_id, created_at, preimage for that main query. and we might want to include the amount to easily calculate - then PG can get everything from the index and does not need to look into the rows

@rolznz
Copy link
Contributor Author

rolznz commented Sep 22, 2023

@bumi it's an automatic migration.

So we create an index for all 4 fields, should we still add indexes for the individual columns too?

@rolznz
Copy link
Contributor Author

rolznz commented Sep 22, 2023

@bumi updated

@rolznz rolznz changed the title fix: add index to payment created_at column fix: add composite index for summing payments Sep 22, 2023
Preimage string
CreatedAt time.Time
Preimage string `gorm:"index:idx_payment_sum"`
CreatedAt time.Time `gorm:"index:idx_payment_sum"`
Copy link
Contributor

Choose a reason for hiding this comment

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

is idx_ a convention we follow here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the first gorm composite index. I am happy to change it, do you have a suggestion?

Copy link
Contributor

@bumi bumi left a comment

Choose a reason for hiding this comment

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

I would prefer to have special migration files actually. some migration that can be run on the existing DBs then.
this feels a bit more stable to me. what do you think.

App App `gorm:"constraint:OnDelete:CASCADE"`
NostrEventId uint `gorm:"index" validate:"required"`
NostrEvent NostrEvent
Amount uint
Amount uint `gorm:"index:idx_payment_sum"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the amount in the index does not make sense to have the amount part of the index we do not query for that.

we do WHERE app_id = ? AND preimage IS NOT ? AND created_at > ?

so maybe we do the 3 in the index and include the amount that then can be used to sum things up.
https://atlasgo.io/guides/postgres/included-columns

@rolznz
Copy link
Contributor Author

rolznz commented Sep 27, 2023

Replaced by #148

@rolznz rolznz closed this Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants