|
Description
Nick Clemens (kidclamp)
2021-07-22 14:49:52 UTC
Created attachment 123104 [details] [review] Bug 28739: Ignore store, update and delete when called from Template Toolkit Created attachment 123105 [details] [review] [FOR DISCUSSION] Bug 28739: Add Koha::Object->read_only (In reply to Nick Clemens from comment #0) > Currently you can do something like: > [% biblio.delete %] > > We should have a way to restrict this Sure. But I definitely like the idea, hahaha Very interesting.. I like the readonly flag.. I wonder about how we identify all 'setters' however.. we have a few methods in objects that have actions at a distance.. we'd need to somehow catch those too.. somehow identifying them as setters for this case. Created attachment 126190 [details] [review] Bug 28739: Execute the letter processing inside a transaction New patch for a KISS approach, still for discussion as I haven't stressed it much. Why didn't we think about that before? Sounds like the more robust and simple solution :) (In reply to Jonathan Druart from comment #6) > New patch for a KISS approach, still for discussion as I haven't stressed it > much. > > Why didn't we think about that before? Sounds like the more robust and > simple solution :) Hmm. I dont hear you talk about performance, deadlocks etc. ? You would do that for every single notice.. (In reply to Marcel de Rooy from comment #7) > (In reply to Jonathan Druart from comment #6) > > New patch for a KISS approach, still for discussion as I haven't stressed it > > much. > > > > Why didn't we think about that before? Sounds like the more robust and > > simple solution :) > > Hmm. I dont hear you talk about performance, deadlocks etc. ? You would do > that for every single notice.. Jonathan, can you run some tests to get some metrics? I think it's safe to assume attempts at db changes will be rare. I'd be interested how much time the transaction wrap adds when no updates take place. I'm hoping the answer is little to none! Considering you might send thousands of overdue, due and predue notices a night, it would be nice to have metrics over that kind of range. Another thought: What about having another more restricted mysql user context for these kind of things? Only reading. Was thinking about Reporting too for that matter. But code wise it would require a bit more discipline to use the right db connection etc. Safer, not nicer? *** Bug 30071 has been marked as a duplicate of this bug. *** (In reply to Marcel de Rooy from comment #9) > Another thought: What about having another more restricted mysql user > context for these kind of things? Only reading. > Was thinking about Reporting too for that matter. +1 I've been wanting a read-only database connection for a while. I was just thinking about for Reports, but this would be another good use case. (It would be good to be able to specify a different endpoint as well for a clustered DB setup where you have read-only nodes). > But code wise it would require a bit more discipline to use the right db > connection etc. Safer, not nicer? We might need to do some re-factoring, but after reviewing https://metacpan.org/pod/DBIx::Class::Schema it looks like it shouldn't be too hard to manage multiple connections. Created attachment 130482 [details] [review] Bug 28739: Execute the letter processing inside a transaction I still think that the transaction is the best (efficient and easy to implement) we can do. We should not care about performance here, especially when the only bit that could suffer are cronjobs. David, you discussed everything but the last suggestion (I admit that the patch was confusing, it embedded non-related stuffs). Chiming in, did some back to back testing of the overdue_notices.pl on our test database. Generated ~480 notices from ~1000 overdue items. No real world difference with or without processing inside a transaction. Both finished in 48-50s when running a few times in sequence. Thanks for the feedback, Björn. I am considering this version ready for signoff. Created attachment 135236 [details] [review] Bug 28739: Execute the letter processing inside a transaction Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Oop, I lost track of this one. Really glad to see someone did some performance testing.. I've just confirmed the same.. for me the impact was imperceivable. Works as expected and is covered by tests (and passes QA script too). Signing off (In reply to Björn Nylén from comment #21) > Chiming in, did some back to back testing of the overdue_notices.pl on our > test database. Generated ~480 notices from ~1000 overdue items. No real > world difference with or without processing inside a transaction. Both > finished in 48-50s when running a few times in sequence. Great to hear, Bjorn. But I think that it is not all there is to say about performance. The point is imo: how are other processes that touch the same table hindered by all transactions that process_message_queue might start (per notice)? In a normal situation without [% branch.delete %] stuff, there is nothing to rollback. So I guess that shouldnt be a big thing. (Can we prove that too?) But we are testing edge cases under security. So what would be the impact on a script that does branch updating while you run these 500 notices with a branch delete creating 500 transactions and rollbacks? The penalty here could well be less imperceivable ;) Still not a real-life situation. Perhaps anyone else has a better idea to test? Created attachment 136804 [details] [review] Bug 28739: Execute the letter processing inside a transaction Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> (In reply to Jonathan Druart from comment #6) > New patch for a KISS approach, still for discussion as I haven't stressed it > much. > > Why didn't we think about that before? Sounds like the more robust and > simple solution :) Oh that's clever! (In reply to Martin Renvoize from comment #4) > Very interesting.. I like the readonly flag.. I wonder about how we identify > all 'setters' however.. we have a few methods in objects that have actions > at a distance.. we'd need to somehow catch those too.. somehow identifying > them as setters for this case. I'm trying to think if this could still be an issue. In theory, all direct database changes would be nullified by the rollback. I suppose the actions at a distance to worry about would be API/network calls, file system changes, and anything that doesn't directly relate to the database. This is where a granular authorization layer would be handy. But that would be a huge project on its own. Hmm, it just occured to me, I didn't test this in conjunction with bug 19966... Or rather for the passing in an object case. I'm reasonably sure it'll still work as expected, but may be worth adding a test for to make sure? Pushed to master for 22.11. Nice work everyone, thanks! Missing dependencies for 22.05, no backport |