search

teksi / wastewater

[DEV] Future TEKSI wastewater module, adapted data model to fit VSA-DSS 2020 new standard
https://teksi.github.io/wastewater
GNU General Public License v3.0
2 stars 5 forks source link

deleting reaches: disabling symbology triggers leaves dead data in the database (reach_points etc) #485

Closed urskaufmann closed 1 hour ago

urskaufmann commented 3 weeks ago

Describe the bug

-> no more visible records in TWW

Conclusion: deleting in vw_tww_reach leaves dead data in classes reach_point, channel, networkelement and wastewater_structure

Expected behavior all related data is also deleted. Is this bug because of the disabled trigger?

Screenshots / data image

Desktop (please complete the following information):

urskaufmann commented 3 weeks ago

If I delete just same reaches without disabling symbology triggers before, the reach_point are also deleted.

What can we do, that we do not get dead data because of deleting records while the triggers are disabled? Deleting reach_points when deleting reaches is a must! No exception. The same in superclass-subclass-relation, both must be deleted, when one record is deleted. It's a bit more difficult with the reaches and channels...

ponceta commented 3 weeks ago

@cymed I think we have to clarify application triggers (which dispatch the data to the right places) with symbology triggers.

Main difference being : symobology triggers can be run anytime as they are just updating fields and stuff for symbology purposes

application triggers can't be simply deactivated or only by a sys_admin knowing which operation can be operated when these are deactivated.

ponceta commented 3 weeks ago

@urskaufmann I think the performance issue is also a high priority point to assess, we made some performance assessment for QWAT at the time we implemented the TRIGGER / VIEW approach, there's no reason the edition performance should be so low as we have to disable application functionalities to make simple edition process.

cymed commented 3 weeks ago

@cymed I think we have to clarify application triggers (which dispatch the data to the right places) with symbology triggers. The behaviour has to do with symbology triggers indirectly:

We do not have an instead of trigger on tww_app.vw_tww_reach but a rule vw_tww_reach_delete that deletes the reach only. On delete of the reach we have a trigger tww_app.symbology_on_reach_delete that was disabled in the old tww_app.disable_symbology_trigger() and thus was disabled in my alteration. If this trigger deletes the networkelement (no effect on symbology), the reach points (calling a symbology trigger) and the channel if it is no longer referenced by the reach (firing another symbology trigger)

Main difference being : symobology triggers can be run anytime as they are just updating fields and stuff for symbology purposes

application triggers can't be simply deactivated or only by a sys_admin knowing which operation can be operated when these are deactivated.

ponceta commented 2 weeks ago

We have to clearly define what is symbology and what is not.

In QGEP the term symbology was already used but all of them where dropped and recreated before and after any upgrade task (and they were dropped, not only disabled).

https://github.com/QGEP/datamodel/blob/124e25279988a97b347989f83ca291cf5297d5cc/06_symbology_functions.sql#L790-L811

Which is now :

We will then have two trigger functions types: disable/enable_symbology_triggers disable/enable_modification_triggers

Since both are on an application triggers. For symbology loops in system triggers, I would add a check/CASE to loop only if symbology triggers are activated.

All of the triggers and rules affecting the datamodel should be documented somewhere to avoid any surprise behaviour.

ponceta commented 1 week ago

@3nids @domi4484 I would need advice from your side :

  1. Why having a rule instead of a delete trigger only for the vw_tww_reach view?
  2. Should we have more triggers to ease the separation between the modification and symbology tasks?
  3. Is the functions / trigger separation a good idea?

Could comment and evaluate what is proposed on https://github.com/teksi/wastewater/pull/491 and what should be adapted to avoid such issues in the future?

cymed commented 1 week ago

It needs to be a rule, because we only want to delete the channel if there are no other reaches with a fk on that channel.

domi4484 commented 1 week ago

If we disable triggers or functions it is normal that something will stop working. This is also why by default this options are disabled and available only for admins. Btw. I would propose to put those actions a bit more hidden into the Settings -> Advanced menu image

Another thing to take care, is that if you disable the triggers, those are disabled for everyone working on the same database and not only for you.

Probably I am missing some background, but for me the main question is why do we need to disable the triggers? Because of performance?

ponceta commented 1 week ago

The general idea was to disable only symbology triggers (the ones doing an update that can be done anytime on the whole database)

Theses triggers are often taking loops and stuff that can lead to performance issues when dealing with several features. 

- Do we know specifically which triggers make things slow? Only the symbology or the modification as well?
cymed commented 1 week ago

Potential improvements include