getAlby / nostr-wallet-connect-next

Nostr Wallet Connect (NIP-47) application to allow apps to connect to your node
https://nwc.getalby.com
Apache License 2.0
7 stars 3 forks source link

Cleanup stale events in the DB (#361) #388

Open rdmitr opened 1 month ago

rdmitr commented 1 month ago

Fixes https://github.com/getAlby/hub/issues/87

rolznz commented 1 month ago

@rdmitr unfortunately this does not work - there's a missing "ON DELETE CASCADE" on the fk_payments_request_event constraint, so it fails to delete events with payments.

Unfortunately to add this I think we need to inside a transaction rename the existing payments table and create a new one.

https://www.techonthenet.com/sqlite/foreign_keys/foreign_delete.php#:~:text=How%20to%20Add%20a%20Foreign,data%20into%20the%20new%20table.

I think we also don't need to delete the response_events manually, they should be automatically deleted by deleting the request_events, since they have a foreign key with a cascading delete.

I will pick this up.

bumi commented 1 month ago

Unfortunately to add this I think we need to inside a transaction rename the existing payments table and create a new one.

this does not sounds like what we want to do. esp. not on an open DB that is in use. why?

then remove that foreign key constraint. I don't think it's necessarily good to have this on the DB level here?

rolznz commented 1 month ago

this does not sounds like what we want to do. esp. not on an open DB that is in use.

It wouldn't be on an open DB, it would be a new database migration that runs on startup.

then remove that foreign key constraint. I don't think it's necessarily good to have this on the DB level here?

That could be another option. But I don't think SQLite supports dropping constraints (e.g. ALTER TABLE payments DROP FOREIGN KEY fk_payments_request_event;). Unless I'm wrong we will still need to create a new table and transfer the payments over.

The recommended way to “drop” a foreign key in SQLite is actually to transfer the data to a new table without a foreign key.

bumi commented 1 month ago

I don't think creating new tables and moving data over on bootup is a good practice. I also have never heard about this and the need for this.

rdmitr commented 1 month ago

@bumi unfortunately, sqlite explicitly does not support altering constraints (https://sqlite.org/omitted.html):

Only the RENAME TABLE, ADD COLUMN, RENAME COLUMN, and DROP COLUMN variants of the ALTER TABLE command are supported. Other kinds of ALTER TABLE operations such as ALTER COLUMN, ADD CONSTRAINT, and so forth are omitted.

Looks like the workaround Roland mentioned is the only way to go if we are to change the constraint. However, we anticipate that those tables can be large. Such a migration on a slow machine (like Raspberry Pi) can be quite slow.

But then again, do we have a lot of active users of the app? If we go forward with this migration, will it affect a lot of people? If not, should we give it a try? Or maybe even change the original database schema so that all new installations get proper DB?