[pmg-devel] [PATCH pmg-api 0/2] fix table order on backup and add foreign key constraint for new tables

[pmg-devel] [PATCH pmg-api 0/2] fix table order on backup and add foreign key constraint for new tables

[Stoiko Ivanov]

This follows the RFC from
https://lists.proxmox.com/pipermail/pmg-devel/2024-February/002776.html

huge thanks to Dominik for catching the issue upon backup+restore with
the constraints added, and to Stefan Hanreich, for looking through the
table create statements with me!

For the creation of indexes - I think we are covered - as there is an
explicit index on the referenced row (e.g. rule.id for rule_attributes) -
it was just not part of the diff-context.
Stefan correctly pointed out that the primary-key index (rule_id, name),
should already be enough - see [0], but in this case (rather little
rows, few writes) I think having the index in addition as we do for the
existing attribut table has its merits

changes from the RFC:
* added a patch to create our rule-db backup in the correct order (tables
  referencing columns in other tables coming after) - while this did work,
  because the schema does not use foreign keys and other constraints
  currently I'd still think it has its merits - e.g. should we ever
  introduce constraints for other tables old backups could not get
  restored anymore. The better approach would probably be to replace our
  own dumping logic by an appropriate call to pg_dump - but this can
  happen as a follow-up
* tested a restore of a backup created with the patches applied (more
  testing would still be much appreciated
* formated the patches with `-U6` so that the create index statements are
  visible, without applying the patch


[0] https://www.postgresql.org/docs/current/indexes-multicolumn.html

Stoiko Ivanov (2):
  backup: reorder tables in order of dependency
  database: use foreign keys for rule and object group attributes

 src/PMG/Backup.pm  | 6 +++---
 src/PMG/Cluster.pm | 2 --
 src/PMG/DBTools.pm | 6 ++----
 src/PMG/RuleDB.pm  | 6 ------
 4 files changed, 5 insertions(+), 15 deletions(-)

-- 
2.39.2

[pmg-devel] [PATCH pmg-api 1/2] backup: reorder tables in order of dependency

[Stoiko Ivanov]

Currently we dump the tables manually and print the relevant
DDL statements into the dump-file before copying the data.

The complete backup and dump specific tables part would probably be
best replaced by an appropriate `pg_dump` invocation - but as a
stop-gap measure reorder the tables in order of their dependencies.

Noticed while adding a foreign key constraint on 2 new tables
(objectgroup_attribute and rule_attribute) and running backup+restore.

Reported-by: Dominik Csapak <d.csapak@proxmox.com>
Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/PMG/Backup.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/PMG/Backup.pm b/src/PMG/Backup.pm
index 5891afb..ab7e628 100644
--- a/src/PMG/Backup.pm
+++ b/src/PMG/Backup.pm
@@ -90,18 +90,18 @@ sub dumpdb {
     eval {
 	$dbh->begin_work;
 
 	# read a consistent snapshot
 	$dbh->do("SET TRANSACTION ISOLATION LEVEL SERIALIZABLE");
 
+	dump_table($dbh, 'objectgroup', $ofh, 'objectgroup_id_seq', 'id');
+	dump_table($dbh, 'object', $ofh, 'object_id_seq', 'id');
 	dump_table($dbh, 'attribut', $ofh);
 	dump_table($dbh, 'objectgroup_attributes', $ofh);
-	dump_table($dbh, 'rule_attributes', $ofh);
-	dump_table($dbh, 'object', $ofh, 'object_id_seq', 'id');
-	dump_table($dbh, 'objectgroup', $ofh, 'objectgroup_id_seq', 'id');
 	dump_table($dbh, 'rule', $ofh, 'rule_id_seq', 'id');
+	dump_table($dbh, 'rule_attributes', $ofh);
 	dump_table($dbh, 'rulegroup', $ofh);
 	dump_table($dbh, 'userprefs', $ofh);
 
 	# we do not save the following tables: cgreylist, cmailstore, cmsreceivers, clusterinfo
     };
     my $err = $@;
-- 
2.39.2

[pmg-devel] [PATCH pmg-api 2/2] database: use foreign keys for rule and object group attributes

[Stoiko Ivanov]

the change is small, reduces code and lets the task of referential
integrity to postgresql

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/PMG/Cluster.pm | 2 --
 src/PMG/DBTools.pm | 6 ++----
 src/PMG/RuleDB.pm  | 6 ------
 3 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/src/PMG/Cluster.pm b/src/PMG/Cluster.pm
index f468618..17ba44d 100644
--- a/src/PMG/Cluster.pm
+++ b/src/PMG/Cluster.pm
@@ -529,14 +529,12 @@ sub sync_ruledb_from_master {
 
 	$ldb->do("DELETE FROM Rule");
 	$ldb->do("DELETE FROM RuleGroup");
 	$ldb->do("DELETE FROM ObjectGroup");
 	$ldb->do("DELETE FROM Object");
 	$ldb->do("DELETE FROM Attribut");
-	$ldb->do("DELETE FROM Objectgroup_Attributes");
-	$ldb->do("DELETE FROM Rule_Attributes");
 
 	eval {
 	    $rdb->begin_work;
 
 	    # read a consistent snapshot
 	    $rdb->do("SET TRANSACTION ISOLATION LEVEL SERIALIZABLE");
diff --git a/src/PMG/DBTools.pm b/src/PMG/DBTools.pm
index 3e814dc..6112566 100644
--- a/src/PMG/DBTools.pm
+++ b/src/PMG/DBTools.pm
@@ -294,25 +294,25 @@ my $userprefs_ctablecmd =  <<__EOD;
     CREATE INDEX UserPrefs_MTime_Index ON UserPrefs (MTime);
 
 __EOD
 
 my $rule_attributes_cmd = <<__EOD;
     CREATE TABLE Rule_Attributes (
-      Rule_ID INTEGER NOT NULL,
+      Rule_ID INTEGER NOT NULL REFERENCES Rule (ID) ON DELETE CASCADE,
       Name VARCHAR(20) NOT NULL,
       Value BYTEA NULL,
       PRIMARY KEY (Rule_ID, Name)
     );
 
     CREATE INDEX Rule_Attributes_Rule_ID_Index ON Rule_Attributes(Rule_ID);
 
 __EOD
 
 my $object_group_attributes_cmd = <<__EOD;
     CREATE TABLE Objectgroup_Attributes (
-      Objectgroup_ID INTEGER NOT NULL,
+      Objectgroup_ID INTEGER NOT NULL REFERENCES Objectgroup (ID) ON DELETE CASCADE,
       Name VARCHAR(20) NOT NULL,
       Value BYTEA NULL,
       PRIMARY KEY (Objectgroup_ID, Name)
     );
 
     CREATE INDEX Objectgroup_Attributes_Objectgroup_ID_Index ON Objectgroup_Attributes(Objectgroup_ID);
@@ -632,14 +632,12 @@ sub init_ruledb {
 	my $glids = "SELECT object.ID FROM Object, Objectgroup WHERE " .
 	    "objectgroup_id = objectgroup.id and class = 'greylist'";
 
 	$dbh->do(
 	    "DELETE FROM Rule;"
 	    ." DELETE FROM RuleGroup;"
-	    ." DELETE FROM Rule_Attributes;"
-	    ." DELETE FROM Objectgroup_Attributes;"
 	    ." DELETE FROM Attribut WHERE Object_ID NOT IN ($glids);"
 	    ." DELETE FROM Object WHERE ID NOT IN ($glids);"
 	    ." DELETE FROM Objectgroup WHERE class != 'greylist';"
 	);
     }
 
diff --git a/src/PMG/RuleDB.pm b/src/PMG/RuleDB.pm
index e5fe56e..315f79b 100644
--- a/src/PMG/RuleDB.pm
+++ b/src/PMG/RuleDB.pm
@@ -273,15 +273,12 @@ sub delete_group {
 	$self->{dbh}->do("DELETE FROM ObjectGroup " .
 			 "WHERE ID = ?", undef, $groupid);
 
 	$self->{dbh}->do("DELETE FROM RuleGroup " .
 			 "WHERE Objectgroup_ID = ?", undef, $groupid);
 
-	$self->{dbh}->do("DELETE FROM Objectgroup_Attributes " .
-			 "WHERE Objectgroup_ID = ?", undef, $groupid);
-
 	$sth = $self->{dbh}->prepare("SELECT * FROM Object " .
 				      "where Objectgroup_ID = ?");
 	$sth->execute($groupid);
 
 	while (my $ref = $sth->fetchrow_hashref()) {
 	    $self->{dbh}->do("DELETE FROM Attribut " .
@@ -769,15 +766,12 @@ sub delete_rule {
 	$self->{dbh}->begin_work;
 
 	$self->{dbh}->do("DELETE FROM Rule " .
 			 "WHERE ID = ?", undef, $ruleid);
 	$self->{dbh}->do("DELETE FROM RuleGroup " .
 			 "WHERE Rule_ID = ?", undef, $ruleid);
-	$self->{dbh}->do("DELETE FROM Rule_Attributes " .
-			 "WHERE Rule_ID = ?", undef, $ruleid);
-
 	$self->{dbh}->commit;
     };
     if (my $err = $@) {
 	$self->{dbh}->rollback;
 	syslog('err', $err);
 	return undef;
-- 
2.39.2

Re: [pmg-devel] [PATCH pmg-api 0/2] fix table order on backup and add foreign key constraint for new tables

[Dominik Csapak]

Can confirm my previous issues are gone now

cluster sync works, backup/restore works, factory defaults work

deletion of the groups/rules also remove the corresponding
attribute entries.

Altough I'm not super into postgres syntax/behaviour regarding
foreign keys, consider this

Tested-by: Dominik Csapak <d.csapak@proxmox.com>
Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>

[pmg-devel] applied: [PATCH pmg-api 0/2] fix table order on backup and add foreign key constraint for new tables

[Thomas Lamprecht]

Am 22/02/2024 um 13:40 schrieb Stoiko Ivanov:
> This follows the RFC from
> https://lists.proxmox.com/pipermail/pmg-devel/2024-February/002776.html
> 
> huge thanks to Dominik for catching the issue upon backup+restore with
> the constraints added, and to Stefan Hanreich, for looking through the
> table create statements with me!
> 
> For the creation of indexes - I think we are covered - as there is an
> explicit index on the referenced row (e.g. rule.id for rule_attributes) -
> it was just not part of the diff-context.
> Stefan correctly pointed out that the primary-key index (rule_id, name),
> should already be enough - see [0], but in this case (rather little
> rows, few writes) I think having the index in addition as we do for the
> existing attribut table has its merits
> 
> changes from the RFC:
> * added a patch to create our rule-db backup in the correct order (tables
>   referencing columns in other tables coming after) - while this did work,
>   because the schema does not use foreign keys and other constraints
>   currently I'd still think it has its merits - e.g. should we ever
>   introduce constraints for other tables old backups could not get
>   restored anymore. The better approach would probably be to replace our
>   own dumping logic by an appropriate call to pg_dump - but this can
>   happen as a follow-up
> * tested a restore of a backup created with the patches applied (more
>   testing would still be much appreciated
> * formated the patches with `-U6` so that the create index statements are
>   visible, without applying the patch
> 
> 
> [0] https://www.postgresql.org/docs/current/indexes-multicolumn.html
> 
> Stoiko Ivanov (2):
>   backup: reorder tables in order of dependency
>   database: use foreign keys for rule and object group attributes
> 
>  src/PMG/Backup.pm  | 6 +++---
>  src/PMG/Cluster.pm | 2 --
>  src/PMG/DBTools.pm | 6 ++----
>  src/PMG/RuleDB.pm  | 6 ------
>  4 files changed, 5 insertions(+), 15 deletions(-)
> 


applied both patches with Dominik's T-b & R-b, thanks!