|
Description
Marcel de Rooy
2022-06-02 11:59:59 UTC
Should we add 'fake' records for the missing identifiers in the authority record number range at upgrade time? These should most probably be the records that have been deleted once (ignoring other technical problems, perhaps with the autoincrement of the table). Should this be a separate table like we have now or should we use a deleted flag on this table as it has been discussed for others? (In reply to Katrin Fischer from comment #2) > Should this be a separate table like we have now or should we use a deleted > flag on this table as it has been discussed for others? Separate table: see description. (In reply to Marcel de Rooy from comment #1) > Should we add 'fake' records for the missing identifiers in the authority > record number range at upgrade time? These should most probably be the > records that have been deleted once (ignoring other technical problems, > perhaps with the autoincrement of the table). I would not do that. There could be huge ranges missing due to not resetting PK during migration. *** Bug 10417 has been marked as a duplicate of this bug. *** Created attachment 165040 [details] [review] Bug 30888: Db rev for new table deletedauth_header In line with the other deleted tables. (See also bug 20271.) Note: Removed the auto_increment for this deleted table. There is no need for it and it could only mess up the table. Additionally, removed the index on origin. Test plan: Run updatedatabase. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 165041 [details] [review] Bug 30888: [DBIX] Schema file for deletedauth_header Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 165042 [details] [review] Bug 30888: Copy info to deleted table Test plan: Delete an authority from the interface. Check presence in the table deletedauth_header, verify that the correct authid and authtype have been inserted. Bonus: Check leader position 05 in deleted table. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 165043 [details] [review] Bug 30888: Unit tests Extending DelAuthority test in AuthoritiesMarc.t. Adding Koha/Authority.t for new method move_to_deleted. Test plan: Run both tests. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to Katrin Fischer from comment #4) > (In reply to Marcel de Rooy from comment #1) > > Should we add 'fake' records for the missing identifiers in the authority > > record number range at upgrade time? These should most probably be the > > records that have been deleted once (ignoring other technical problems, > > perhaps with the autoincrement of the table). > > I would not do that. There could be huge ranges missing due to not resetting > PK during migration. Still thinking about filling it (optionally) with data from a table in a older backup or so? With some maint script perhaps, but not on this report. We had the discussion for "recalls" and decided to not stick to the current (wrong) pattern. So I don't think this is acceptable, you need to add a new column to the existing table instead. See recalls.completed. Also bug 19532 comment 5. (In reply to Jonathan Druart from comment #11) > We had the discussion for "recalls" and decided to not stick to the current > (wrong) pattern. > > So I don't think this is acceptable, you need to add a new column to the > existing table instead. > > See recalls.completed. > Also bug 19532 comment 5. I just read: Regarding the number of critical issues we had the last few months, I do not think it is a good idea to reuse this pattern. We should not move data from one table to another but add a deleted/old flag instead. Is that a discussion? :) There is a lot of discussion on 20271. And it is open for 6 years already. That is imo not a good argument to block a simple and needed development like this one. I could understand that if you add a NEW table that you should do it as you said. But adding this burden to an EXISTING table is really something else. Adding this 'requirement' effectively kills this development. Can imagine that we could raise serious issues too when we add a flag and forget a few occurrences throughout the whole codebase where we read records from the table. If if would be a requirement, we should have community consensus about it and make it a coding guideline. Which I do not recommend. So IIUC the contention is that authorities is an existing table and adding a deleted flag will cause it to be forgotten to be checked for in places => so bad bugs. VS having a separate table, which causes bad bugs. Introducing a flag will cause bugs only on that moment. (found and fixed in the next months) Does having a separate table causes bugs over time or will that also be on the moment of introduction? In other words, were the bugs due to the other deleted* tables always there? Or were they more recent due to other changes and it's easier to introduce bugs than with a deleted flag? (In reply to Victor Grousset/tuxayo from comment #13) Thanks for taking a look. > So IIUC the contention is that authorities is an existing table and adding a > deleted flag will cause it to be forgotten to be checked for in places => so > bad bugs. > VS having a separate table, which causes bad bugs. Please explain why a separate table causes bad bugs. Some issues that we experienced in the past were related to autoincrement. And note that I deliberately removed the autoincrement here from the deleted table. > Introducing a flag will cause bugs only on that moment. (found and fixed in > the next months) Hopefully :) One could argue that having one table requires more careful coding and is therefore more error prone? > Does having a separate table causes bugs over time or will that also be on > the moment of introduction? In other words, were the bugs due to the other > deleted* tables always there? Or were they more recent due to other changes > and it's easier to introduce bugs than with a deleted flag? See above too. I am having the strong imnpression that the option with a flag might introduce more bugs than the current development. Touching more code etc. Some related questions: Why enforce something here that we have not been able to do in the last 6 years for a few similar tables? And what about: I opened this bug close to 2 years ago. And already mentioned the 'merging tables' discussion in my description. When I finally submit code, the whole discussion is revived. Is that fair? This small patch set is imo ready to be used but now the whole things must be designed differently? Will add yet another comment about the two scenarios from a more general perspective more or less proving that both ways have pro and cons. We cannot say here that the deleted flag-option should be better. Please read this on dba.stackexchange.com: https://dba.stackexchange.com/questions/14402/tombstone-table-vs-deleted-flag-in-database-syncronization-soft-delete-scenari/14419 It contains questions like: How fast do deletes need to be? How fast do un-deletes need to be? How often will deleted data be queried and will it be queried with data that has not been deleted? How fast do queries of deleted data need to be? Do you need to preserve only deleted items or changes as well? Do you need to keep the table/indexes on the primary table small? What partitioning and/or change tracking technologies are available on the database platform? How much disk space is available? Will the deleting occur on the fly or in batch operations? In this authorities case, deletes are not required to be very fast. And we are not undeleting right now. The deleted data is not queried often. We do not preserve all changes. There is no need to keep all indexes on deleted data (I removed one here as well). So I would tend (as objective as I can..) to the tombstone table here for performance reasons. (And less code as argued already.) (In reply to Marcel de Rooy from comment #14) > Please explain why a separate table causes bad bugs. Some issues that we > experienced in the past were related to autoincrement. Hm, I though bug 19532 comment 5 was about more stuff than autoincrement. Because these are indeed reliably avoidable now that it's known. > > Introducing a flag will cause bugs only on that moment. (found and fixed in > > the next months) > > Hopefully :) One could argue that having one table requires more careful > coding and is therefore more error prone? When doing delete flags, there should be something in the data layer to by default query only the non deleted data. Otherwise it will be hard to never forget to filter. And code everywhere will have some additional complexity to filter the data. It's a bit of code duplication in many places. Do we have all/almost all the interactions with the authorities table in the data layer? Otherwise it's not sure it's possible to deal with these issues :/ > Some related questions: > Why enforce something here that we have not been able to do in the last 6 > years for a few similar tables? Here, there is no tombstone table so introducing the flag is less work than doing that + removing the tombstone table. Still more work than for a totally new table like recalls. But before that, the question is, should we even avoid introducing a tombstone table? That's suddenly not as clear. > Will add yet another comment about the two scenarios from a more general > perspective more or less proving that both ways have pro and cons. > [...] > Please read this on dba.stackexchange.com: > https://dba.stackexchange.com/questions/14402/tombstone-table-vs-deleted- > flag-in-database-syncronization-soft-delete-scenari/14419 From the discussions and attempts to switch to delete flags in the last years, it seemed a given that a tombstone table was an anti-pattern but it turns out totally not [U+1F631] Back to trying to find the contention point: Now that it's known how to avoid autoincrement issues with tombstone tables, do they have a significant drawback for our use case? I'm interested in the discussions here and want to help this one move forward.. I've had it added to the next dev meeting at a topic. I'm feeling out of date and knowledge at this point so don't want to push either way until we have that discussion. (In reply to Martin Renvoize from comment #17) > I'm interested in the discussions here and want to help this one move > forward.. I've had it added to the next dev meeting at a topic. I'm feeling > out of date and knowledge at this point so don't want to push either way > until we have that discussion. Great, thanks. Confident that we will decide the right thing, lol We discussed this topic at the Developer's meeting on November 11: https://koha-hedgedoc.servers.llownd.net/Development_IRC_meeting_11_September_2024?# (In reply to Katrin Fischer from comment #19) > We discussed this topic at the Developer's meeting on November 11: > > https://koha-hedgedoc.servers.llownd.net/ > Development_IRC_meeting_11_September_2024?# This has been decided long after this development was submitted. The report was open from 2022 but nobody cared. So I do not agree. Are the people who 'decided this' also willing to refactor along the lines of the new approach? I just fear that we wont see anything moving here soon.. Closing this report. (In reply to Marcel de Rooy from comment #20) > (In reply to Katrin Fischer from comment #19) > > We discussed this topic at the Developer's meeting on November 11: > > > > https://koha-hedgedoc.servers.llownd.net/ > > Development_IRC_meeting_11_September_2024?# > > This has been decided long after this development was submitted. The report > was open from 2022 but nobody cared. So I do not agree. > > Are the people who 'decided this' also willing to refactor along the lines > of the new approach? I just fear that we wont see anything moving here soon.. > > Closing this report. Please don't close. I think it's still very valid to want to keep the deleted authorities. Maybe remove yourself as Assignee instead. But I understand your problem, making it a combined table required extentially more work to the existing code than moving the records on deletion. Just to add to the discussion here. What we generally agreed upon was that we want to combine for new tables and the tombstone should be the exception. Marcel, you make a great argument for it here, therefore I see no reason why this should need to be refactored. The decision, as I understood it, is mainly about preference. And we want to combine tables where we can, but not at all costs. Happy to hear from other attendees of the meeting whether they got a different understanding of the consesus. So I stayed pretty quiet in the developer meeting around this topic to be honest. I still don't feel hugely knowledgable about the topic now, but my general feeling has changed over the years around this sort of topic. Whilst I can see the advantages of a single table, I can also see the advantages of tombstone tables.. and in fact I think they both have their place in Koha. My feeling was that we agreed that generally we'd push for combined tables for Koha whenever appropriate, but I don't think we should rule out tombstones when they make sense for performance and code simplicity reasons. In this particular case, I'd actually push for the pragmatic solution of taking the code that's written. It follows some solid patterns we already have whilst also being reasonably modern in it's approach. Personally, I'd prefer to see this move and be accepted rather than have it die. (In reply to Paul Derscheid from comment #22) > Just to add to the discussion here. What we generally agreed upon was that > we want to combine for new tables and the tombstone should be the exception. > > Marcel, you make a great argument for it here, therefore I see no reason why > this should need to be refactored. > > The decision, as I understood it, is mainly about preference. And we want to > combine tables where we can, but not at all costs. > > Happy to hear from other attendees of the meeting whether they got a > different understanding of the consesus. Thanks for your clarification. That was not very clear in comment19 having no further access there to the meeting minutes. (Out of scope: Personally prefer to still have them on on the koha wiki than on yet another website/login etc.) Just one sentence because unrelated to the bug: the code is in place to produce plain-text minutes as well, they just need to be uploaded to minutes.koha-community.org somehow. Sorry Marcel, I was not aware of difficulties accessing the notes. I just didn't want the discussion to die there without making it to this bug. I think Martin made excellent points and I have a very similar feeling, but didn't want us ending up with "not making a decision" and stayed more quiet too. I think for completely new features it makes sense to think about a combined table from the beginning. I also feel that Tomas made a point about "business data" vs. other data that got a bit lost. So maybe both approaches can have a place in Koha. A deletedauthorities table would fit the existing pattern we have for biblio, biblioitems etc. and will not require us to change a huge amount of code (I am thinking from indexing to every method that accesses this data to filter for display etc.). So I'd also vote for keeping these patches alive. Great, so we have the confirmation that there isn't a fatal flaw in tombstone tables! :D --- Patches still apply but tests fail because bug 30047 added the new "heading" column to the auth_header table. Created attachment 171850 [details] [review] Bug 30888: add `heading` column after bug 30047 added it to auth_header Created attachment 171851 [details] [review] Bug 30888: [DBIX] Schema file for deletedauth_header Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> "heading" column added
manual deletion and merge works :D
but merge test fails
/kohadevbox/koha/t/db_dependent/AuthoritiesMarc.t .. 6/13
# Failed test 'Moved to deleted'
# at /kohadevbox/koha/t/db_dependent/AuthoritiesMarc.t line 335.
# got: undef
# expected: anything else
# Looks like you planned 2 tests but ran 3.
# Looks like you failed 1 test of 3 run.
# Failed test 'ModAuthority() tests'
# at /kohadevbox/koha/t/db_dependent/AuthoritiesMarc.t line 338.
# Looks like you planned 3 tests but ran 2.
I don't know if creating authorities is different now with the "heading" column and it would be missing in the calls.
(In reply to Victor Grousset/tuxayo from comment #27) > Great, so we have the confirmation that there isn't a fatal flaw in > tombstone tables! :D > Hum? Where did you read that exactly? There was no real discussion during the dev meeting tbh. The situation is still not clear for future improvements/new patches. It seems that we are ok to let this go as it, because the code is there already and that the auth is closed (in regard of the code and DB structure) to bib/item. They which will certainly never be merged as bug 20271 is abandoned, and accepting more like this is moving even more into that direction. I am only trying to reflect the few things what have been said during the meeting, not taking any positions here. A decision should still be taken for new tables IMO, and a coding guideline written down. (In reply to Jonathan Druart from comment #31) > (In reply to Victor Grousset/tuxayo from comment #27) > > Great, so we have the confirmation that there isn't a fatal flaw in > > tombstone tables! :D > > > > Hum? Where did you read that exactly? Mainly from «the tombstone should be the exception[...]I see no reason why this should need to be refactored.» «Whilst I can see the advantages of a single table, I can also see the advantages of tombstone tables.. and in fact I think they both have their place in Koha.» «So I'd also vote for keeping these patches alive.» And a very conservative interpretation is that there are no **fatal** issues with tombstone tables for our case. Fatal as in impossible to deal with. As in doomed. And the dba.stackexchange.com indicates it's a valid approach in a lot of case. And we needed to know if there was anything specific to Koha and authorities that would be fatal for it's use. But I think I get what you mean. My message can still come up as they are fine to use. So no, as it stands the consensus is that tombstone tables should be avoided as much as possible. (In reply to Jonathan Druart from comment #32) > There was no real discussion during the dev meeting tbh. The situation is > still not clear for future improvements/new patches. > [...] > A decision should still be taken for new tables IMO, and a coding guideline > written down. Oh, +1 then to have that written in the guidelines. It seemed clear that under pretty much any circumstance tombstone tables shouldn't be introduced. But here we might blur the line. And having a guideline would make less likely that someone else would think they have a case comparable to authorities. And would code and propose a tombstone table when it was avoidable. (In reply to Victor Grousset/tuxayo from comment #30) > "heading" column added > > manual deletion and merge works :D > > but merge test fails > > /kohadevbox/koha/t/db_dependent/AuthoritiesMarc.t .. 6/13 > # Failed test 'Moved to deleted' > # at /kohadevbox/koha/t/db_dependent/AuthoritiesMarc.t line 335. > # got: undef > # expected: anything else > # Looks like you planned 2 tests but ran 3. > # Looks like you failed 1 test of 3 run. > > # Failed test 'ModAuthority() tests' > # at /kohadevbox/koha/t/db_dependent/AuthoritiesMarc.t line 338. > # Looks like you planned 3 tests but ran 2. > > > I don't know if creating authorities is different now with the "heading" > column and it would be missing in the calls. Not sure what happened here. But looks like the test in Delete got moved into Mod. Maybe rebase issue. Created attachment 172090 [details] [review] Bug 30888: Db rev for new table deletedauth_header In line with the other deleted tables. (See also bug 20271.) Note: Removed the auto_increment for this deleted table. There is no need for it and it could only mess up the table. Additionally, removed the index on origin. Test plan: Run updatedatabase. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 172091 [details] [review] Bug 30888: add `heading` column after bug 30047 added it to auth_header Created attachment 172092 [details] [review] Bug 30888: [DBIX] Schema file for deletedauth_header Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 172093 [details] [review] Bug 30888: Copy info to deleted table Test plan: Delete an authority from the interface. Check presence in the table deletedauth_header, verify that the correct authid and authtype have been inserted. Bonus: Check leader position 05 in deleted table. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 172094 [details] [review] Bug 30888: Unit tests Extending DelAuthority test in AuthoritiesMarc.t. Adding Koha/Authority.t for new method move_to_deleted. Test plan: Run both tests. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 172095 [details] [review] Bug 30888: Db rev for new table deletedauth_header In line with the other deleted tables. (See also bug 20271.) Note: Removed the auto_increment for this deleted table. There is no need for it and it could only mess up the table. Additionally, removed the index on origin. Test plan: Run updatedatabase. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 172096 [details] [review] Bug 30888: add `heading` column after bug 30047 added it to auth_header Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 172097 [details] [review] Bug 30888: [DBIX] Schema file for deletedauth_header Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 172098 [details] [review] Bug 30888: Copy info to deleted table Test plan: Delete an authority from the interface. Check presence in the table deletedauth_header, verify that the correct authid and authtype have been inserted. Bonus: Check leader position 05 in deleted table. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 172099 [details] [review] Bug 30888: Unit tests Extending DelAuthority test in AuthoritiesMarc.t. Adding Koha/Authority.t for new method move_to_deleted. Test plan: Run both tests. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Back to NSO. Victor, could you confirm the tests pass now and your SO ? Thanks! (In reply to Marcel de Rooy from comment #45) > Back to NSO. Victor, could you confirm the tests pass now and your SO ? > Thanks! s/your SO/add your SO/ Note for QA:
FAIL Koha/Schema/Result/DeletedauthHeader.pm
FAIL pod coverage
POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666
WARN tidiness
The file is less tidy than before (bad/messy lines before: 0, now: 30)
Please ignore.
Created attachment 172549 [details] [review] Bug 30888: Db rev for new table deletedauth_header In line with the other deleted tables. (See also bug 20271.) Note: Removed the auto_increment for this deleted table. There is no need for it and it could only mess up the table. Additionally, removed the index on origin. Test plan: Run updatedatabase. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 172550 [details] [review] Bug 30888: add `heading` column after bug 30047 added it to auth_header Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 172551 [details] [review] Bug 30888: [DBIX] Schema file for deletedauth_header Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 172552 [details] [review] Bug 30888: Copy info to deleted table Test plan: Delete an authority from the interface. Check presence in the table deletedauth_header, verify that the correct authid and authtype have been inserted. Bonus: Check leader position 05 in deleted table. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 172553 [details] [review] Bug 30888: Unit tests Extending DelAuthority test in AuthoritiesMarc.t. Adding Koha/Authority.t for new method move_to_deleted. Test plan: Run both tests. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Thanks for the fix Marcel.
It works! :)
------
> Bonus: Check leader position 05 in deleted table.
Yep, went from (n)ew to (d)eleted
00348nz[...] => 00348dz[...]
(In reply to Victor Grousset/tuxayo from comment #53) > Thanks for the fix Marcel. > > It works! :) Great! |