add scripts to backfill undelegated fields in db #833

hannaeko · 2021-07-28T11:18:46Z

Purpose

Update the old 'undelegated' values in the database.

PostgreSQL tested with ~1% of the records of the zonemaster.net database on Postgres 9.3 and 13.3
SQLite
MySQL tested with ~1% of the records of the zonemaster.net database on MariaDB 10.3

Context

Fixes #829

Changes

Add patch scripts.

How to test this PR

Postgres

Check the value of delegated vs undelegated tests (all test are delegated, so the second query should return nothing except if you have the patch for undelegated tests in which case there can be a few).

select DISTINCT params->>'ds_info' as ds_info, params->>'nameservers'  as nameservers from test_results where undelegated = 0 ;

select DISTINCT params->>'ds_info' as ds_info, params->>'nameservers' as nameservers from test_results where undelegated = 1 ;

Apply update script perl zonemaster-backend/share/patch_postgresql_db_zonemaster_backend_ver_7.0.0.pl (ignore error if the table has already been changed, "if not exists" is not compatible with PG 9.3)
Repeat the first step to verify that all tests has been classified correctly.

MySQL

Check the value of delegated vs undelegated tests (all test are delegated, so the second query should return nothing except if you have the patch for undelegated tests in which case there can be a few).

 select distinct json_extract(params, '$.nameservers'), json_extract(params, '$.ds_info') from test_results where undelegated = 0;

 select distinct json_extract(params, '$.nameservers'), json_extract(params, '$.ds_info') from test_results where undelegated = 1;

Apply update script perl zonemaster-backend/share/patch_mysql_db_zonemaster_backend_ver_7.0.0.pl
Repeat the first step to verify that all tests has been classified correctly.

matsduf

$ make distcheck
"/usr/local/bin/perl" "-Iinc" "-MExtUtils::Manifest=fullcheck" -e fullcheck
Not in MANIFEST: share/patch_mysql_db_zonemaster_backend_ver_7.0.0.pl
Not in MANIFEST: share/patch_sqlite_db_zonemaster_backend_ver_7.0.0.pl

matsduf · 2021-07-29T09:36:06Z

I have tested SQLite where some delegated were reported as undelegated and vice versa. After running the script all were in the correct category.

In the instructions there is an extra step for FreeBSD (export ZONEMASTER_BACKEND_CONFIG_FILE="/usr/local/etc/zonemaster/backend_config.ini") which is easy to forget and could be removed by an update in the scripts.

Replace

use Zonemaster::Backend::Config;
use Zonemaster::Backend::DB::SQLite;

with

BEGIN {
    if ($^O eq "freebsd") {
        unless ($ENV{ZONEMASTER_BACKEND_CONFIG_FILE}) {
            $ENV{ZONEMASTER_BACKEND_CONFIG_FILE} = "/usr/local/etc/zonemaster/backend_config.ini"
        }
    }
    require Zonemaster::Backend::Config;
}
use Zonemaster::Backend::DB::SQLite;

I have tested this with SQLite, but it should work the same way in the other scripts.

matsduf

The following files must also be update: README.md and docs/upgrade_db_zonemaster_backend_ver_7.0.0.md

hannaeko · 2021-07-29T11:19:31Z

In the instructions there is an extra step for FreeBSD (export ZONEMASTER_BACKEND_CONFIG_FILE="/usr/local/etc/zonemaster/backend_config.ini") which is easy to forget and could be removed by an update in the scripts.

For this issue I suggest a small change in configuration loading, see #835

hannaeko · 2021-07-29T13:29:36Z

The following files must also be update: README.md and docs/upgrade_db_zonemaster_backend_ver_7.0.0.md

Those files have been updated.

share/patch_mysql_db_zonemaster_backend_ver_7.0.0.pl

share/patch_sqlite_db_zonemaster_backend_ver_7.0.0.pl

matsduf · 2021-07-29T15:00:14Z

In the instructions there is an extra step for FreeBSD (export ZONEMASTER_BACKEND_CONFIG_FILE="/usr/local/etc/zonemaster/backend_config.ini") which is easy to forget and could be removed by an update in the scripts.

For this issue I suggest a small change in configuration loading, see #835

~~I guess there are pros and cons with both solutions. A "local" solution in the patch scripts make sure that the change will not affect anything else. Also see my comment in #835.~~

matsduf · 2021-07-29T15:01:32Z

README.md

@@ -40,7 +40,7 @@ Older than 1.0.3                      | [Upgrade to 1.0.3]    |
 At least 1.0.3 but older than 1.1.0   | [Upgrade to 1.1.0]    |
 At least 1.1.0 but older than 5.0.0   | [Upgrade to 5.0.0]    |
 At least 5.0.0 but older than 5.0.2   | [Upgrade to 5.0.2]    | For MySQL/MariaDB only
-At least 5.0.2 but older than 7.0.0   | [Upgrade to 7.0.0]    | For PostgreSQL only
+At least 5.0.2 but older than 7.0.0   | [Upgrade to 7.0.0]    | For all database types


I suggest that the comment is empty when all database engines are affected.

matsduf · 2021-07-29T22:28:33Z

If #835 is accepted, then that should be merged first, and then the special instructions for FreeBSD in the upgrade instructions can be removed.

ghost

Looks good. A small comment though and I commented #835 as well.

ghost · 2021-08-05T09:06:46Z

share/patch_postgresql_db_zonemaster_backend_ver_7.0.0.pl

+    eval {
+        $dbh->do( 'ALTER TABLE test_results ADD COLUMN undelegated integer NOT NULL DEFAULT 0' );
+    };
+    print "Error while changing DB schema:  " . $@;


Shouldn't this be printed only if there are errors?

Yes indeed, fixed.

matsduf · 2021-08-12T21:22:38Z

Merge #835, rebase this and remove the special setting for FreeBSD.

…-scripts

hannaeko · 2021-08-18T10:15:03Z

Merge #835, rebase this and remove the special setting for FreeBSD.

This is now done.

share/patch_mysql_db_zonemaster_backend_ver_7.0.0.pl

mattias-p · 2021-08-19T11:31:25Z

share/patch_postgresql_db_zonemaster_backend_ver_7.0.0.pl

+        print "Error while changing DB schema:  " . $@;
+    }
+
+    $dbh->do( qq[


Could this be done using the much simpler for-loop from the MySQL and SQLite implementations?

It could but it has a impact on the performance. Processing 13K rows (about 1% of the zonemaster.net database) with the sql statement takes around 1 second and doing it with the while loop takes 17 seconds.

That's an impressive difference. But this is a one-time task.
And the SQL statement is quite complex. What are the chances that a bug has snuck into it?

Fair enough, I can can change it to the other implementation.

@blacksponge, have you addressed the comment from @mattias-p?

It has now been addressed.

matsduf · 2021-09-01T08:24:14Z

When @blacksponge and @mattias-p agree on share/patch_postgresql_db_zonemaster_backend_ver_7.0.0.pl then I am ready to approve.

hannaeko added 3 commits July 28, 2021 13:08

add scripts to backfill undelegated fields in db

8cc5186

make the script compatible with PG 9.3

eab5dfb

add mysql patch script

9c7f895

hannaeko requested review from matsduf and a user July 28, 2021 13:20

matsduf reviewed Jul 29, 2021

View reviewed changes

update manifest

f9b56ef

matsduf requested changes Jul 29, 2021

View reviewed changes

matsduf added this to the v2021.2 milestone Jul 29, 2021

hannaeko mentioned this pull request Jul 29, 2021

improve config search and add default freebsd config location #835

Merged

update docs

d921fdf

ghost reviewed Jul 29, 2021

View reviewed changes

share/patch_mysql_db_zonemaster_backend_ver_7.0.0.pl Outdated Show resolved Hide resolved

share/patch_sqlite_db_zonemaster_backend_ver_7.0.0.pl Outdated Show resolved Hide resolved

matsduf reviewed Jul 29, 2021

View reviewed changes

remove unused import

122aa0f

ghost reviewed Aug 5, 2021

View reviewed changes

only print error if exists

aea3853

ghost previously approved these changes Aug 5, 2021

View reviewed changes

This was referenced Aug 5, 2021

Update the database patch (upgrade) scripts #618

Closed

Harmonize databases #841

Merged

hannaeko dismissed ghost ’s stale review via 51ee8f0 August 12, 2021 07:49

lasy load rows

51ee8f0

hannaeko added 2 commits August 16, 2021 09:49

Merge remote-tracking branch 'origin/develop' into update-undelegated…

0cfcbc3

…-scripts

remove special instructions

4e8ea76

ghost previously approved these changes Aug 16, 2021

View reviewed changes

mattias-p reviewed Aug 19, 2021

View reviewed changes

improve code readability

25625ff

hannaeko dismissed ghost ’s stale review via 25625ff August 24, 2021 11:21

ghost previously approved these changes Aug 24, 2021

View reviewed changes

hannaeko dismissed ghost ’s stale review via 0dc62fe September 1, 2021 15:29

change postgres implementation

0dc62fe

matsduf approved these changes Sep 1, 2021

View reviewed changes

ghost approved these changes Sep 2, 2021

View reviewed changes

hannaeko merged commit 86ae65a into zonemaster:develop Sep 8, 2021

ghost mentioned this pull request Sep 29, 2021

Provide update script for "undelegated"? #829

Closed

hannaeko self-assigned this Nov 17, 2021

hannaeko added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Nov 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add scripts to backfill undelegated fields in db #833

add scripts to backfill undelegated fields in db #833

hannaeko commented Jul 28, 2021 •

edited

Loading

matsduf left a comment •

edited

Loading

matsduf commented Jul 29, 2021 •

edited

Loading

matsduf left a comment •

edited

Loading

hannaeko commented Jul 29, 2021

hannaeko commented Jul 29, 2021

matsduf commented Jul 29, 2021 •

edited

Loading

matsduf Jul 29, 2021

hannaeko Aug 3, 2021

matsduf commented Jul 29, 2021

ghost left a comment

ghost Aug 5, 2021

hannaeko Aug 5, 2021

matsduf commented Aug 12, 2021

hannaeko commented Aug 18, 2021

mattias-p Aug 19, 2021

hannaeko Aug 23, 2021

mattias-p Aug 25, 2021

hannaeko Aug 25, 2021

matsduf Sep 1, 2021

hannaeko Sep 1, 2021

matsduf commented Sep 1, 2021

add scripts to backfill undelegated fields in db #833

add scripts to backfill undelegated fields in db #833

Conversation

hannaeko commented Jul 28, 2021 • edited Loading

Purpose

Context

Changes

How to test this PR

Postgres

MySQL

matsduf left a comment • edited Loading

Choose a reason for hiding this comment

matsduf commented Jul 29, 2021 • edited Loading

matsduf left a comment • edited Loading

Choose a reason for hiding this comment

hannaeko commented Jul 29, 2021

hannaeko commented Jul 29, 2021

matsduf commented Jul 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsduf commented Jul 29, 2021

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsduf commented Aug 12, 2021

hannaeko commented Aug 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsduf commented Sep 1, 2021

hannaeko commented Jul 28, 2021 •

edited

Loading

matsduf left a comment •

edited

Loading

matsduf commented Jul 29, 2021 •

edited

Loading

matsduf left a comment •

edited

Loading

matsduf commented Jul 29, 2021 •

edited

Loading