Re: Database consistency issues
On Wed, 2018-08-08 at 12:27 -0400, Konstantin Ryabitsev wrote: > On Thu, Aug 09, 2018 at 02:01:11AM +1000, Daniel Axtens wrote: > > In the case of Patchwork 1.1.3, I am more confused - the Comment model > > has a foreign key to Patch here. So if you try to delete Patch you > > should have got a referential integrity error like I did with > > e.g. Events. (Patchwork pre 2.0 is a pain to test as it uses Vagrant > > VMs, which use VirtualBox by default, which breaks SecureBoot, so I'm > > not going to try to get it going tonight.) > > > > Was there anything you deleted after or during the migration? > > Daniel: > > Hmm... Well, the migration was a bit hairy, so it's possible something > did get missed. We migrated from 1.0, though, not from 1.1.3, so it's > possible something got missed? If it helps, I can send the database > schema that we currently have. A copy of the schema, ideally before and after if you've a backup, would be helpful. I don't know how much help I'll be but hopefully we can have someone look at it. Stephen ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
Re: Database consistency issues
On Thu, 2018-08-09 at 02:01 +1000, Daniel Axtens wrote: > > > > I will prepare a patch that adds the foreign keys. > > Further to this: > > Patchwork 2.1 has that Comment has a foreign key relationship to > submission, not patch. > > Patch *inherits* from Submission. This was, in hindsight, one of the > cases where an ORM and object-orientation do not sit well > together. Regardless, there's a migration > (0009_add_submission_model.py) that renames the old Patch to > Submission. It creates a new Patch table, which has Submission as a > foreign key. > > Now Submission doesn't have a foreign key to Patch, because a > Submission can be either a Patch or a CoverLetter or the body of a > comment. So deleting a Patch in 2.1 won't delete the Submission, so > it won't trigger the Comment foreign key, and you'll be left with > dangling submissions and comments. Oh, interesting. As you note, Submission doesn't have foreign key to Patch; rather, it's the other way round as Patch is a "submodel" of Submission. However, I'd always assumed [*] that there would be some kind of uniqueness constraint here that would ensure that deleting a Patch instance would result in the Submission instance also being deleted. Clearly, if this ever does happen (and I really don't know if it does. Again, this was a straight up assumption), it's something that's enforced by Django itself, i.e. in software instead of at the DB level. > I'm not immediately sure how best to address this except to continue to > unpick the general existence of Submission which is part of my long term > plan. > > [Stephen, do we allow comments on cover letters? Can we just add a > foreign key to Comment referencing Patch? Yup. We have a foreign key on Submission [1] which means a comment can be mapped to a Patch, a Cover Letter, or any other kind of submission we might add in the future (I'm seen forks on GitHub that capture non- patch/cover letter emails too, turning Patchwork into something like a patch-focused Pipermail). > Could we add nullable foreign > keys to Submission referencing Patch/CoverLetter/Comment so as to > enforce some more referential integrity? (a la Event)] I think we can go one better and fold CoverLetter and Patch back into Submission. Ruby on Rails uses single table inheritance [2], whereby everything exists in one table and you simply provide different "views" into this. This does mean you lose some features that the DB will provide (like the ability to make sure the diff column is non-null for patches) but the performance improvement might ultimately be worth it. I have a half-finished patch sitting in my local repo for a solid three months now that I really need to finish... > In the case of Patchwork 1.1.3, I am more confused - the Comment model > has a foreign key to Patch here. So if you try to delete Patch you > should have got a referential integrity error like I did with > e.g. Events. In case you're curious, the reason this is the case is described at [3]. In short, the commit message for each Patch used to be stored in a linked Comment. I got rid of that in this commit. Probably doesn't help with the actual issue though. > (Patchwork pre 2.0 is a pain to test as it uses Vagrant > VMs, which use VirtualBox by default, which breaks SecureBoot, so I'm > not going to try to get it going tonight.) Random thought: would it make sense to backport our Docker changes to these versions? They're not bug fixes so I wouldn't normally be inclined to backport this stuff but they're also not user facing. Hope this helps somewhat. Stephen [*] In hindsight, making assumptions where there is the very real possibility of data loss may not have been the best idea. Noted for future reference. [1] https://github.com/getpatchwork/patchwork/blob/f9772a0a3/patchwork/models.py#L598 [2] https://api.rubyonrails.org/classes/ActiveRecord/Inheritance.html [3] https://github.com/getpatchwork/patchwork/commit/ef56359fb ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
Re: Database consistency issues
On Thu, Aug 09, 2018 at 02:01:11AM +1000, Daniel Axtens wrote: In the case of Patchwork 1.1.3, I am more confused - the Comment model has a foreign key to Patch here. So if you try to delete Patch you should have got a referential integrity error like I did with e.g. Events. (Patchwork pre 2.0 is a pain to test as it uses Vagrant VMs, which use VirtualBox by default, which breaks SecureBoot, so I'm not going to try to get it going tonight.) Was there anything you deleted after or during the migration? Daniel: Hmm... Well, the migration was a bit hairy, so it's possible something did get missed. We migrated from 1.0, though, not from 1.1.3, so it's possible something got missed? If it helps, I can send the database schema that we currently have. Best, -K signature.asc Description: PGP signature ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
Re: Database consistency issues
> > I will prepare a patch that adds the foreign keys. Further to this: Patchwork 2.1 has that Comment has a foreign key relationship to submission, not patch. Patch *inherits* from Submission. This was, in hindsight, one of the cases where an ORM and object-orientation do not sit well together. Regardless, there's a migration (0009_add_submission_model.py) that renames the old Patch to Submission. It creates a new Patch table, which has Submission as a foreign key. Now Submission doesn't have a foreign key to Patch, because a Submission can be either a Patch or a CoverLetter or the body of a comment. So deleting a Patch in 2.1 won't delete the Submission, so it won't trigger the Comment foreign key, and you'll be left with dangling submissions and comments. I'm not immediately sure how best to address this except to continue to unpick the general existence of Submission which is part of my long term plan. [Stephen, do we allow comments on cover letters? Can we just add a foreign key to Comment referencing Patch? Could we add nullable foreign keys to Submission referencing Patch/CoverLetter/Comment so as to enforce some more referential integrity? (a la Event)] In the case of Patchwork 1.1.3, I am more confused - the Comment model has a foreign key to Patch here. So if you try to delete Patch you should have got a referential integrity error like I did with e.g. Events. (Patchwork pre 2.0 is a pain to test as it uses Vagrant VMs, which use VirtualBox by default, which breaks SecureBoot, so I'm not going to try to get it going tonight.) Was there anything you deleted after or during the migration? Regards, Daniel > > Thanks for the report! > > Regards, > Daniel ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
Re: Database consistency issues
On Thu, Aug 09, 2018 at 01:16:25AM +1000, Daniel Axtens wrote: SELECT id, submission_id FROM patchwork_comment WHERE submission_id=9896319; +--+---+ | id | submission_id | +--+---+ | 20810589 | 9896319 | +--+---+ 1 row in set (0.00 sec) MariaDB [KORG_PATCHWORK]> SELECT * FROM patchwork_submission WHERE id=9896319; Empty set (0.00 sec) Right, well this is definitely the most terrifying start to any email I have received since starting as one of the patchwork maintainers! Haha, sorry, I should have prefixed this with "I'm not seeing any data loss." :) I will prepare a patch that adds the foreign keys. Thanks! I know they are in place for the pgsql side of things, since the lore.kernel.org/patchwork instance is using postgresql. So, it was either a difference between db implementations, or a side-effect of migration. Best, -K signature.asc Description: PGP signature ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
Re: Database consistency issues
Hi Konstantin, > I'm trying to figure out how this can possibly happen: > >> SELECT id, submission_id FROM patchwork_comment WHERE > submission_id=9896319; > +--+---+ > | id | submission_id | > +--+---+ > | 20810589 | 9896319 | > +--+---+ > 1 row in set (0.00 sec) > > MariaDB [KORG_PATCHWORK]> SELECT * FROM patchwork_submission WHERE > id=9896319; > Empty set (0.00 sec) Right, well this is definitely the most terrifying start to any email I have received since starting as one of the patchwork maintainers! > There don't appear to be any foreign key constraints on the > patchwork_comment table -- would that be the side-effect of migration? I'm pretty sure that's right. I tested something similar to what you did on a local 2.1 setup: 1) find a comment mysql> select id, submission_id FROM patchwork_comment WHERE submission_id = 3; ++---+ | id | submission_id | ++---+ | 2 | 3 | ++---+ 1 row in set (0.00 sec) 2) verify it has a patchwork_patch entry mysql> select submission_ptr_id, patch_project_id from patchwork_patch where submission_ptr_id = 3; +---+--+ | submission_ptr_id | patch_project_id | +---+--+ | 3 |1 | +---+--+ 1 row in set (0.00 sec) 3) delete the associate patches, clearing out foreign keys as required mysql> delete from patchwork_patch where patch_project_id = 1; ERROR 1451 (23000): Cannot delete or update a parent row: a foreign key constraint fails (`patchwork`.`patchwork_event`, CONSTRAINT `patchwork_event_patch_id_15d4004e_fk_patchwork` FOREIGN KEY (`patch_id`) REFERENCES `patchwork_patch` (`submission_ptr_id`)) mysql> delete patchwork_event from patchwork_event inner join patchwork_patch on patchwork_event.patch_id = patchwork_patch.submission_ptr_id where patchwork_patch.patch_project_id = 1; Query OK, 64087 rows affected (14.09 sec) mysql> delete from patchwork_patch where patch_project_id = 1; ERROR 1451 (23000): Cannot delete or update a parent row: a foreign key constraint fails (`patchwork`.`patchwork_seriespatch`, CONSTRAINT `patchwork_seriespatc_patch_id_33c4d56e_fk_patchwork` FOREIGN KEY (`patch_id`) REFERENCES `patchwork_patch` (`submission_ptr_id`)) mysql> delete patchwork_seriespatch from patchwork_seriespatch inner join patchwork_patch on patchwork_seriespatch.patch_id = patchwork_patch.submission_ptr_id where patchwork_patch.patch_project_id = 1; Query OK, 33392 rows affected (1.24 sec) mysql> delete from patchwork_patch where patch_project_id = 1; ERROR 1451 (23000): Cannot delete or update a parent row: a foreign key constraint fails (`patchwork`.`patchwork_patchtag`, CONSTRAINT `patchwork_patchtag_patch_id_5fad6ac3_fk_patchwork` FOREIGN KEY (`patch_id`) REFERENCES `patchwork_patch` (`submission_ptr_id`)) mysql> delete patchwork_patchtag from patchwork_patchtag inner join patchwork_patch on patchwork_patchtag.patch_id = patchwork_patch.submission_ptr_id where patchwork_patch.patch_project_id = 1; Query OK, 11701 rows affected (0.23 sec) mysql> delete from patchwork_patch where patch_project_id = 1; Query OK, 33446 rows affected (2.98 sec) 4) Verify that the comment is still there ... mysql> select id, submission_id FROM patchwork_comment WHERE submission_id = 3; ++---+ | id | submission_id | ++---+ | 2 | 3 | ++---+ 1 row in set (0.00 sec) 5) But the patch is not... mysql> select submission_ptr_id, patch_project_id from patchwork_patch where submission_ptr_id = 3; Empty set (0.00 sec) Even worse, we don't even clear up submissions in PW 2.1 when you delete the patches! mysql> select id, project_id from patchwork_submission where id =3 ; +++ | id | project_id | +++ | 3 | 1 | +++ 1 row in set (0.00 sec) (This wouldn't be the case with deletion in PW1 as there was no separate submission table.) So - unless I'm mistaken or something else is also awry - it doesn't look like you've experienced data loss, just excess data not cleaned up. I will prepare a patch that adds the foreign keys. Thanks for the report! Regards, Daniel ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
Database consistency issues
Hi, all: I'm trying to figure out how this can possibly happen: > SELECT id, submission_id FROM patchwork_comment WHERE submission_id=9896319; +--+---+ | id | submission_id | +--+---+ | 20810589 | 9896319 | +--+---+ 1 row in set (0.00 sec) MariaDB [KORG_PATCHWORK]> SELECT * FROM patchwork_submission WHERE id=9896319; Empty set (0.00 sec) There don't appear to be any foreign key constraints on the patchwork_comment table -- would that be the side-effect of migration? Regards, -- Konstantin Ryabitsev Director, IT Infrastructure Security The Linux Foundation signature.asc Description: OpenPGP digital signature ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
