Incremental Configuration Updates

[oxzi]

• Bump github.com/icinga/icingadb dependency

  The changed icingadb dependencies contains the latest state of the noma branch, Add extension required by Icinga Notifications icingadb#578, with an additional fix for utils.FromUnixMilli, utils.FromUnixMilli has floating point rounding issues icingadb#753 and Icinga/icingadb@066abea.

• Incremental Config Updates

  Enable incremental configuration updates by introducing two new columns- changed_at and deleted - for all tables directly referenced in the ConfigSet. The other relationship tables are requiring a changed_at update in their relative parent table.

  As a limitation, deleted rows within the database cannot be detected as the deletion logic now completely relies on the deleted column. Thus, deletions must be performed by setting both changed_at and deleted.

• PostgreSQL Triggers for Incremental Config Changes

  On an INSERT or UPDATE on each row of a "main configuration tables", as listed in the ConfigSet Go struct, the changed_at column field will automatically be bumped to the current timestamp. In addition, a DELETE on those tables will result in also setting changed_at to the current time as well as raising the deleted column instead of actually performing the DELETE command.

  Furthermore, the "relationship tables" will update the changed_at field in its referred row in the main table for INSERT, UPDATE or DELETE.

Closes #5.

The web counterpart is being developed in Icinga/icinga-notifications-web#187.

[oxzi]

After talking with @sukhwinder33445 about this change as he implements the web counterpart, I have removed the incr_cfg_delete function and its trigger usage, rewriting a DELETE to an UPDATE setting deleted = 'y' on the main configuration tables.

For historical reasons and if it might become relevant again, I have attached the diff below.

incr_cfg_delete diff

diff --git a/schema/pgsql/schema.sql b/schema/pgsql/schema.sql
index a5abfea..08f5406 100644
--- a/schema/pgsql/schema.sql
+++ b/schema/pgsql/schema.sql
@@ -88,6 +88,23 @@ CREATE FUNCTION incr_cfg_bump_changed_at_relation_2nd() -- join_tbl text, foreig
     END;
     $$;
 
+-- incr_cfg_delete is a BEFORE TRIGGER for DELETE, which sets changed_at to the current timestamp and raises the deleted
+-- column instead of actually deleting the row.
+CREATE FUNCTION incr_cfg_delete()
+    RETURNS trigger
+    LANGUAGE plpgsql
+    AS $$
+    BEGIN
+        -- Cannot change OLD's attributes, as this would require returning OLD which implies continuing with deletion.
+        EXECUTE format('
+            UPDATE %s
+            SET changed_at = EXTRACT(EPOCH FROM NOW()) * 1000, deleted = ''y''
+            WHERE id = %s',
+            TG_TABLE_NAME, OLD.id);
+        RETURN NULL;
+    END;
+    $$;
+
 CREATE TABLE available_channel_type (
     type text NOT NULL,
     name text NOT NULL,
@@ -119,6 +136,11 @@ CREATE TRIGGER trg_channel_incr_cfg_update
     FOR EACH ROW
     EXECUTE FUNCTION incr_cfg_bump_changed_at();
 
+CREATE TRIGGER trg_channel_incr_cfg_delete
+    BEFORE DELETE ON channel
+    FOR EACH ROW
+    EXECUTE FUNCTION incr_cfg_delete();
+
 CREATE TABLE contact (
     id bigserial,
     full_name text NOT NULL,
@@ -140,6 +162,11 @@ CREATE TRIGGER trg_contact_incr_cfg_update
     FOR EACH ROW
     EXECUTE FUNCTION incr_cfg_bump_changed_at();
 
+CREATE TRIGGER trg_contact_incr_cfg_delete
+    BEFORE DELETE ON contact
+    FOR EACH ROW
+    EXECUTE FUNCTION incr_cfg_delete();
+
 CREATE TABLE contact_address (
     id bigserial,
     contact_id bigint NOT NULL REFERENCES contact(id),
@@ -160,6 +187,11 @@ CREATE TRIGGER trg_contact_address_incr_cfg_update
     FOR EACH ROW
     EXECUTE FUNCTION incr_cfg_bump_changed_at();
 
+CREATE TRIGGER trg_contact_address_incr_cfg_delete
+    BEFORE DELETE ON contact_address
+    FOR EACH ROW
+    EXECUTE FUNCTION incr_cfg_delete();
+
 CREATE TABLE contactgroup (
     id bigserial,
     name text NOT NULL,
@@ -178,6 +210,11 @@ CREATE TRIGGER trg_contactgroup_incr_cfg_update
     FOR EACH ROW
     EXECUTE FUNCTION incr_cfg_bump_changed_at();
 
+CREATE TRIGGER trg_contactgroup_incr_cfg_delete
+    BEFORE DELETE ON contactgroup
+    FOR EACH ROW
+    EXECUTE FUNCTION incr_cfg_delete();
+
 -- Changes to contactgroup_member should be notified by an updated contactgroup.changed_at.
 CREATE TABLE contactgroup_member (
     contactgroup_id bigint NOT NULL REFERENCES contactgroup(id),
@@ -208,6 +245,11 @@ CREATE TRIGGER trg_schedule_incr_cfg_update
     FOR EACH ROW
     EXECUTE FUNCTION incr_cfg_bump_changed_at();
 
+CREATE TRIGGER trg_schedule_incr_cfg_delete
+    BEFORE DELETE ON schedule
+    FOR EACH ROW
+    EXECUTE FUNCTION incr_cfg_delete();
+
 CREATE TABLE timeperiod (
     id bigserial,
     owned_by_schedule_id bigint REFERENCES schedule(id), -- nullable for future standalone timeperiods
@@ -225,6 +267,11 @@ CREATE TRIGGER trg_timeperiod_incr_cfg_update
     FOR EACH ROW
     EXECUTE FUNCTION incr_cfg_bump_changed_at();
 
+CREATE TRIGGER trg_timeperiod_incr_cfg_delete
+    BEFORE DELETE ON timeperiod
+    FOR EACH ROW
+    EXECUTE FUNCTION incr_cfg_delete();
+
 -- Changes to timeperiod_entry should be notified by an updated timeperiod.changed_at.
 CREATE TABLE timeperiod_entry (
     id bigserial,
@@ -315,6 +362,11 @@ CREATE TRIGGER trg_source_incr_cfg_update
     FOR EACH ROW
     EXECUTE FUNCTION incr_cfg_bump_changed_at();
 
+CREATE TRIGGER trg_source_incr_cfg_delete
+    BEFORE DELETE ON source
+    FOR EACH ROW
+    EXECUTE FUNCTION incr_cfg_delete();
+
 CREATE TABLE object (
     id bytea NOT NULL, -- SHA256 of identifying tags and the source.id
     source_id bigint NOT NULL REFERENCES source(id),
@@ -377,6 +429,11 @@ CREATE TRIGGER trg_rule_incr_cfg_update
     FOR EACH ROW
     EXECUTE FUNCTION incr_cfg_bump_changed_at();
 
+CREATE TRIGGER trg_rule_incr_cfg_delete
+    BEFORE DELETE ON rule
+    FOR EACH ROW
+    EXECUTE FUNCTION incr_cfg_delete();
+
 -- Changes to rule_escalation should be notified by an updated rule.changed_at.
 CREATE TABLE rule_escalation (
     id bigserial,
diff --git a/schema/pgsql/upgrades/025.sql b/schema/pgsql/upgrades/025.sql
index ee5f595..3c36de4 100644
--- a/schema/pgsql/upgrades/025.sql
+++ b/schema/pgsql/upgrades/025.sql
@@ -52,6 +52,21 @@ CREATE FUNCTION incr_cfg_bump_changed_at_relation_2nd() -- join_tbl text, foreig
     END;
     $$;
 
+CREATE FUNCTION incr_cfg_delete()
+    RETURNS trigger
+    LANGUAGE plpgsql
+    AS $$
+    BEGIN
+        -- Cannot change OLD's attributes, as this would require returning OLD which implies continuing with deletion.
+        EXECUTE format('
+                UPDATE %s
+                SET changed_at = EXTRACT(EPOCH FROM NOW()) * 1000, deleted = ''y''
+                WHERE id = %s',
+                       TG_TABLE_NAME, OLD.id);
+        RETURN NULL;
+    END;
+    $$;
+
 ALTER TABLE channel
     ADD COLUMN changed_at bigint NOT NULL DEFAULT EXTRACT(EPOCH FROM NOW()) * 1000,
     ADD COLUMN deleted boolenum NOT NULL DEFAULT 'n';
@@ -98,21 +113,41 @@ CREATE TRIGGER trg_channel_incr_cfg_update
     FOR EACH ROW
     EXECUTE FUNCTION incr_cfg_bump_changed_at();
 
+CREATE TRIGGER trg_channel_incr_cfg_delete
+    BEFORE DELETE ON channel
+    FOR EACH ROW
+    EXECUTE FUNCTION incr_cfg_delete();
+
 CREATE TRIGGER trg_contact_incr_cfg_update
     BEFORE INSERT OR UPDATE ON contact
     FOR EACH ROW
     EXECUTE FUNCTION incr_cfg_bump_changed_at();
 
+CREATE TRIGGER trg_contact_incr_cfg_delete
+    BEFORE DELETE ON contact
+    FOR EACH ROW
+    EXECUTE FUNCTION incr_cfg_delete();
+
 CREATE TRIGGER trg_contact_address_incr_cfg_update
     BEFORE INSERT OR UPDATE ON contact_address
     FOR EACH ROW
     EXECUTE FUNCTION incr_cfg_bump_changed_at();
 
+CREATE TRIGGER trg_contact_address_incr_cfg_delete
+    BEFORE DELETE ON contact_address
+    FOR EACH ROW
+    EXECUTE FUNCTION incr_cfg_delete();
+
 CREATE TRIGGER trg_contactgroup_incr_cfg_update
     BEFORE INSERT OR UPDATE ON contactgroup
     FOR EACH ROW
     EXECUTE FUNCTION incr_cfg_bump_changed_at();
 
+CREATE TRIGGER trg_contactgroup_incr_cfg_delete
+    BEFORE DELETE ON contactgroup
+    FOR EACH ROW
+    EXECUTE FUNCTION incr_cfg_delete();
+
 CREATE TRIGGER trg_contactgroup_member_changed_at_relation
     AFTER INSERT OR UPDATE OR DELETE ON contactgroup_member
     FOR EACH ROW
@@ -123,11 +158,21 @@ CREATE TRIGGER trg_schedule_incr_cfg_update
     FOR EACH ROW
     EXECUTE FUNCTION incr_cfg_bump_changed_at();
 
+CREATE TRIGGER trg_schedule_incr_cfg_delete
+    BEFORE DELETE ON schedule
+    FOR EACH ROW
+    EXECUTE FUNCTION incr_cfg_delete();
+
 CREATE TRIGGER trg_timeperiod_incr_cfg_update
     BEFORE INSERT OR UPDATE ON timeperiod
     FOR EACH ROW
     EXECUTE FUNCTION incr_cfg_bump_changed_at();
 
+CREATE TRIGGER trg_timeperiod_incr_cfg_delete
+    BEFORE DELETE ON timeperiod
+    FOR EACH ROW
+    EXECUTE FUNCTION incr_cfg_delete();
+
 CREATE TRIGGER trg_timeperiod_entry_changed_at_relation
     AFTER INSERT OR UPDATE OR DELETE ON timeperiod_entry
     FOR EACH ROW
@@ -143,11 +188,21 @@ CREATE TRIGGER trg_source_incr_cfg_update
     FOR EACH ROW
     EXECUTE FUNCTION incr_cfg_bump_changed_at();
 
+CREATE TRIGGER trg_source_incr_cfg_delete
+    BEFORE DELETE ON source
+    FOR EACH ROW
+    EXECUTE FUNCTION incr_cfg_delete();
+
 CREATE TRIGGER trg_rule_incr_cfg_update
     BEFORE INSERT OR UPDATE ON rule
     FOR EACH ROW
     EXECUTE FUNCTION incr_cfg_bump_changed_at();
 
+CREATE TRIGGER trg_rule_incr_cfg_delete
+    BEFORE DELETE ON rule
+    FOR EACH ROW
+    EXECUTE FUNCTION incr_cfg_delete();
+
 CREATE TRIGGER trg_rule_escalation_changed_at_relation
     AFTER INSERT OR UPDATE OR DELETE ON rule_escalation
     FOR EACH ROW

[julianbrost]

My thinking when suggesting to not have changed_at/deleted in every config table but only in the main ones was a bit that should also be the natural thing to do in Web. For example, if you add an recipient somewhere in an escalation, you do this on the edit event rule page and you click save there, so there it would only be necessary to update rule.changed_at when the form is saved, all the changes to other tables would be covered implicitly by that.

Now this turned in to quite a bit of complex trigger mechanics instead. I would definitely consider adding these two columns to all tables instead of requiring three variants of the trigger functions. Is everything you implemented in PostgreSQL triggers also possible with MySQL and MariaDB?

as well as raising the deleted column instead of actually performing the DELETE command.

Does raising have any special meaning in this context? Or is it simply supposed to say SET deleted = 'y'?

Also, I'm not yet sure how good the idea is to hook DELETE in a way that it doesn't actually delete, i.e. might this be too surprising so that this could result in unexpected behavior? The only downside I see with executing an UPDATE ... SET deleted = 'y' instead would be that with the current current distinction into main/child config tables, you'd have to make a difference between those. If we'd decide to add the columns to every table, this would no longer be the case. (But on the other hand, there would then be a difference between adding a new group membership and reviving a previously deleted one.)

Edit: this part became irrelevant by itself, I wrote this comment before seeing the previous comment.

[sukhwinder33445]

I would say that if for example the rule is updated via form (rule.name ="UpdatedName"), the changed_at column should be updated automatically. Maridb/Mysql can do this with ON UPDATE. If an INSERT/UPDATE/DELETE takes place in the related (child) tables, the web should (can) update the changed_at column of the main(parent) table manually.

[julianbrost]

I would say that if for example the rule is updated via form (rule.name ="UpdatedName"), the changed_at column should be updated automatically.

If an INSERT/UPDATE/DELETE takes place in the related (child) tables, the web should (can) update the changed_at column of the main(parent) table manually.

Why do you want to make a distinction here? Would that make anything easier for web?

Maridb/Mysql can do this with ON UPDATE.

Does this even work here? Reads like this is a magic feature just for TIMESTAMP and DATETIME which we don't use.

[sukhwinder33445]

Why do you want to make a distinction here? Would that make anything easier for web?

Yes. For example, if the rule name is changed, the form returns new values that we can write directly to $db->update('rule', $formValues);. But now $formValues also needs the column changed_at. Adding this column is no big deal, but if db can do it itself, why should we do it manually. This will also prevent future errors if someone forgets to add this column to the $formValues.

Does this even work here? Reads like this is a magic feature just for TIMESTAMP and DATETIME which we don't use.

But changed_at is handled as timestamp.

Update:
I thought it would be simpler than the changes on the web, but it's not, and it's getting more complex with the triggers. And the data type is not a real timestamp. I can understand your reasoning now.

Web should do all that manually, and the triggers can be removed.

[oxzi]

While the triggers allowed me to offload a lot of complexity to the database, it now went back to the notification daemon. I hope that I have found most of the obvious bugs that can occur, especially during partial updates. This version now works "trigger-free" and has the two columns in all tables, including relationship tables. Please give it a try and report back.

[yhabteab]

Just a few comments that caught my eyes!

[yhabteab]

I am not quite sure about the naming pendingLastChange, as this map does not cache pending timestamps, but changed_at timestamp of a given table already seen by the daemon.

[yhabteab]

I would make sure that the timestamp given to SelectContext() everywhere is not zero instead of using such DBMs specific queries. You can simply introduce a helper method that retrieves the last seen ChangedAt timestamp from the cache store and sets it to types.UnixMill(0) if required. Such log messages look pretty confusing anyway: ... "changed_at_after": "0001-01-01T00:00:00.000Z.

[yhabteab]

Same here!

[yhabteab]

There is already a sqlx.In() method for expanding the args within the ... IN() clause. It is also already used in our code base.

icinga-notifications/internal/utils/utils.go

Lines 97 to 101 in 17a0abe

	query := fmt.Sprintf("%s WHERE %q IN (?)", db.BuildSelectStmt(subject, subject), idColumn)
	stmt, args, err := sqlx.In(query, ids)
	if err != nil {
	return errors.Wrapf(err, "cannot build placeholders for %q", query)
	}

[yhabteab]

I would just add a ruleIDs slice here and everywhere you use utils.MapKeys() and fill it in the loop above, as already done for rulesByID instead of utils.MapKeys().