Use a unique set of definitions to initialize the database #834

ghost · 2021-07-28T17:09:14Z

Purpose

This PR is about removing duplicate in database initialization as expressed in #689.

Context

Fixes #689.
Partially addresses #805

Changes

Database and user creations are done manually on Backend installation (as documented in the installation document)
Creates a new script share/create_db.pl that can be used to create the tables for any database engine.
Removes files share/initial-mysql.sql, share/initial-postgres.sql, script/create_db_mysql.pl, script/create_db_postgresql_9.3.pl and share/create_db_sqlite.pl. The SQL commands are now part of a create_db() routine in the MySQL.pm and PostgreSQL.pm files respectively.
Do not drop the tables before creating them anymore.

I left the whole commit history. Some commits were only needed to make the code easier to refactor later in the PR.

How to test this PR

Follow the installation instructions (develop) and make sure you have a running Backend.

hannaeko · 2021-07-29T08:28:32Z

Nice work, this looks promising. I do have a few comments.

I think you should use the connection settings provided in the configuration as much as possible, the database can be on a different host or port, this should be handled. Also I think the user creation should be optional. Maybe only use the superuser to create the local user and grant privileges if required, but then use the new user to create the tables; this can even be done using the database handler in perl. What do you think?

ghost · 2021-07-29T09:15:57Z

@blacksponge good remarks. I tried to only do some refactoring and keep the old behavior as much as possible. If we agree on this PR, I see few updates here and there (as well as your remarks).

matsduf · 2021-08-01T18:40:06Z

* Creates a new script `share/create_db.pl` that can be used to automatically create the database for all database engine. The config file is used to know which engine to use. It's also used to get specific values such as the database name, the database user and its password.

Is it really easier to maintain one script that should handle all database engines instead three different script? For the user it will be the same.

matsduf · 2021-08-01T18:48:10Z

share/create_db.pl

+    print "\n";
+    print "Automatically initialize the Zonemaster database based on the Zonemaster Backend config\n";
+    print "file. Use the values to create the database and a user with enough privileges to\n";
+    print "access this database. Also create all the Zonemaster tables inside this database.\n";


For FreeBSD this depends on #835 to find the configuration file.

To be consistent: "Zonemaster Backend" --> "Zonemaster-Backend" or "Zonemaster::Backend".

ghost · 2021-08-02T14:18:32Z

Is it really easier to maintain one script that should handle all database engines instead three different script?

Maybe not.

TBH I am still not completely satisfied with this PR. I need to think about it a little more.

ghost · 2021-08-04T09:04:10Z

I took a 180 degrees shift. I choose to remove the .sql files and keep the create_db subroutines (moving them in the MySQL.pm and PostgreSQL.pm file).
Database and user creations are back to the documentation. There is no wrapper script beside the new share/create_db.pl script that is only about initializing the tables independently from the engine.

I also updated the installation process with notes to update the "backend_config.ini" file based on the database name chosen, and its user/password.

The new installation procedure needs to be tested on FreeBSD.

The way I see it in the near future is to remove the call to share/create_db.pl in the installation process and make the Backend call create_db() on startup instead.

hannaeko · 2021-08-04T11:44:01Z

Tested with postgres 12 and mariadb 10, it is working well. I think it is cleaner this way.

ghost · 2021-08-05T14:26:30Z

I realized that the create_db() routine in SQLite was dropping the tables. I removed this part and included it into the related commit "Do not DROP tables on creation".

matsduf

I do not see the gain of this change. For the user (installation instructions) it will require more steps to create the database, especially for MariaDB/MySQL, but also for PostgreSQL and SQLite. We should try to make the installation step easy.

ghost · 2021-08-09T10:55:16Z

We should try to make the installation step easy.

I agree on this. And I think this might help go that way even if this PR adds more steps to the installation process.

When you install the Backend, you only need to create a database and a user (for MySQL and PostgreSQL). You also need to set these values into the backend_config.ini file. That is all what is required, and this might require certain privileges. Once you have performed these steps, with the needed privileges, you can drop them and let the Backend perform the rest of the operations (the tables' creation).

This PR is about splitting the process in two. One part creates the database and the user (if applicable). The other part is about creating the tables. Currently everything has to be made by hand during installation, but a future work I am planning is to let the Backend automatically create the tables on startup (if they don't exist). This would then remove the "Database configuration" section for each engine. *
Then this might also give the possibility to develop a script that creates the database and the user (if applicable). I think that such a script should takes its inputs from the backend_config.ini file. But it should also be possible to bypass the database and/or user creation.

Finally I think that having the commands to create the database (and user if applicable) inside the installation instruction makes it easier for a user to tweak the installation process to its needs (for instance use a different database name, user, password, or even the database's host or port).

* I didn't add the tables' creation by the Backend to this PR to avoid overloading this PR

matsduf · 2021-08-09T21:36:17Z

If the second step can be done before the next release I am fine with this PR, but if we risk having such complex installation instructions at next release I am not fine with it.

I didn't add the tables' creation by the Backend to this PR to avoid overloading this PR

That is a good ambition.

ghost · 2021-08-12T14:04:07Z

If the second step can be done before the next release I am fine with this PR

This second step is done in #846

matsduf · 2021-08-12T21:02:12Z

If the second step can be done before the next release I am fine with this PR

This second step is done in #846

if #846 is the end result, then the end result gives a installation instruction that is too complex. I cannot see why it should be more complex than today. Command line commands (shell) is simpler than having to give SQL commands directly to the server or editing the configuration file.

ghost · 2021-08-16T10:42:25Z

if #846 is the end result, then the end result gives a installation instruction that is too complex. I cannot see why it should be more complex than today. Command line commands (shell) is simpler than having to give SQL commands directly to the server or editing the configuration file.

Yes #846 is the end result.
Indeed it is easier, however not easily adaptable. Say you want to use a different database, you need to update the "share/initial-mysql.sql" or "share/initial-postgres.sql" file, or manually create it and run "script/create_db_mysql.pl" or "script/create_db_postgresql_9.3.pl" as done with Travis.
Since the goal is about removing duplicates in the database definition, I realized that Travis that uses a different database that requires to run commands different from the one in the installation document in order to create the database. So I tried to merge everything to come with a unique solution. Maybe this can be improved somehow to avoid having all these new SQL commands in the installation process.

mattias-p · 2021-08-17T11:53:33Z

I really like that you put the database definitions in the adapter modules!

I agree with @matsduf that it's nicer to have only shell commands in the installation instruction, rather than jumping in and out of special purpose shells (e.g. mysql).

I can see four ways forward:

Merge this PR as is.
Make each SQL statement into its own command (e.g. echo "CREATE DATABASE zonemaster" | sudo mysql).
Put these SQL statements into an SQL file and have the user run that.
Move the database and user creation statements into the create_db procedure and make them use the database name and user name from the config file.

The fourth option has the advantage that it automatically coordinates the database name, user name and password between the SQL statements and the configuration file.

ghost · 2021-08-17T12:49:00Z

I agree with @matsduf that it's nicer to have only shell commands in the installation instruction, rather than jumping in and out of special purpose shells (e.g. mysql).

Oh, I think I now understand what was meant by "too complex", thanks for the light.
To be honest, when I was working on this PR I found that we had a very detailed instructions set for MySQL for FreeBSD, so I was inspired by it.

The thing with the 4th option is that you cannot reuse the data_source_name string as is because it takes the database name which does not exist yet. Hence you can't use dbh() either or _new_db() because they take a "user" and "password" (for MySQL and PostgreSQL) that are not yet defined. We could update the code to create the database and user when calling create_db, but I think this is too much effort for a small gain (but I might be wrong).

About the 3rd option, this will be slightly harder to tweak the parameters to someone needs.

So I'll go with option 2 to address your remarks.

ghost · 2021-08-17T13:55:08Z

So I'll go with option 2 to address your remarks.

I realized that this might leak the password.

sudo mysql -e "CREATE USER 'zonemaster'@'localhost' IDENTIFIED BY 'zonemaster';"

Should I make such update in the installation document?

* "results" column was changed to MEDIUMBLOB with commit a028b32 (see issue #616). * "undelegated" column definition and default value was updated with commit 37a371b (see issue #812) * missing index creation on "progress" This applies to the Perl initialization script.

Move database and user creations to the installation documentation instead of within the initial-postgres.sql file.

* Remove each script per database engine * Update MANIFEST

* Database initialization is now performed via create_db.pl * Update MANIFEST

* Call to create_db script for each database * Refactor SQLite initialization

ghost · 2021-09-09T15:03:26Z

I've rebased the branch on top of develop and fixed the conflicts within the Installation file.

Create a new section to document database configuration once per OS.

ghost · 2021-09-13T07:48:15Z

@blacksponge, @mattias-p, @matsduf could you re-review?

hannaeko

Nice work! Just a small comment

hannaeko · 2021-09-13T09:54:41Z

lib/Zonemaster/Backend/DB/MySQL.pm

@@ -95,6 +95,92 @@ sub dbh {
    return $self->dbhandle;
 }

+sub create_db {


Maybe include the create_db method in the requires of DB.pm ?

docs/Installation.md

matsduf · 2021-09-13T12:02:11Z

docs/Installation.md

+```sh
+cd `perl -MFile::ShareDir -le 'print File::ShareDir::dist_dir("Zonemaster-Backend")'`
+su -m zonemaster -c "perl create_db.pl"
+```


Suggestion to make it easier to just copy and paste:

Make create_db.pl executable

Make the command a one-liner:

su -m zonemaster -c "$(perl -MFile::ShareDir -le 'print File::ShareDir::dist_dir("Zonemaster-Backend")')/create_db.pl"

I'm sorry, for FreeBSD it was a bad suggestion from me. Default shell for FreeBSD root is C shell (or tcsh), and that does not support "$( )". Bourne shell (or sh) is the default shell for other users, and that supports "$( )". Back ticks have to be used, and that affects <"> too.

su -m zonemaster -c "`perl -MFile::ShareDir -le 'print File::ShareDir::dist_dir(qw(Zonemaster-Backend))'`/create_db.pl"

This should be fixed (and I realized that I forgot to remove the call to perl since the script is now executable).

docs/Installation.md

share/create_db.pl

matsduf · 2021-09-13T19:17:40Z

docs/Installation.md

+```sh
+cd `perl -MFile::ShareDir -le 'print File::ShareDir::dist_dir("Zonemaster-Backend")'`
+su -m zonemaster -c "perl create_db.pl"
+```


I'm sorry, for FreeBSD it was a bad suggestion from me. Default shell for FreeBSD root is C shell (or tcsh), and that does not support "$( )". Bourne shell (or sh) is the default shell for other users, and that supports "$( )". Back ticks have to be used, and that affects <"> too.

su -m zonemaster -c "`perl -MFile::ShareDir -le 'print File::ShareDir::dist_dir(qw(Zonemaster-Backend))'`/create_db.pl"

ghost added this to the v2021.2 milestone Jul 28, 2021

matsduf reviewed Aug 1, 2021

View reviewed changes

ghost changed the title ~~Use a unique script to initialize the database~~ Use a unique set of definitions to initialize the database Aug 4, 2021

ghost marked this pull request as ready for review August 4, 2021 09:21

ghost requested review from matsduf, mattias-p and hannaeko August 4, 2021 09:22

hannaeko previously approved these changes Aug 4, 2021

View reviewed changes

ghost linked an issue Aug 5, 2021 that may be closed by this pull request

Duplicated database definitions #689

Closed

ghost dismissed hannaeko’s stale review via a0662b1 August 5, 2021 14:24

ghost mentioned this pull request Aug 5, 2021

Harmonize databases #841

Merged

matsduf reviewed Aug 5, 2021

View reviewed changes

ghost mentioned this pull request Aug 12, 2021

Automatically create tables on damons' startup #846

Closed

Alexandre Pion added 2 commits September 9, 2021 14:08

Remove spaces and better indentation

2b26e78

Alexandre Pion added 9 commits September 9, 2021 14:39

Update installation instruction for PostgreSQL

2fee60b

Move database and user creations to the installation documentation instead of within the initial-postgres.sql file.

Do not DROP tables on creation

69c3b10

Move create_db into Zonemaster::Backend::DB::MySQL

65f7d5c

Move create_db into Zonemaster::Backend::DB::PostgreSQL

97f149b

Remove unused modules

c3bb3b3

New generic create_db call

0087d33

* Remove each script per database engine * Update MANIFEST

Refactor and adapt Travis

597e6d5

Remove unused SQL files

a53017e

* Database initialization is now performed via create_db.pl * Update MANIFEST

Update documentation installation process

ded7b90

* Call to create_db script for each database * Refactor SQLite initialization

Avoid repetition in documentation

78184e5

Create a new section to document database configuration once per OS.

hannaeko reviewed Sep 13, 2021

View reviewed changes

matsduf requested changes Sep 13, 2021

View reviewed changes

Alexandre Pion added 8 commits September 13, 2021 15:51

Missing requirement

9a75d7d

Missing semi-colon

c90bd84

Make script executable

a1531b8

Oneline command

a21d1b1

Explicitly logout from MariaDB

9ef2cd3

Improve wording

4005158

Editorial update

8b4c96c

Fix links

353ee36

matsduf requested changes Sep 13, 2021

View reviewed changes

Fix calling create_db command

98d79c2

matsduf approved these changes Sep 14, 2021

View reviewed changes

ghost merged commit 0460c26 into zonemaster:develop Sep 15, 2021

This was referenced Sep 29, 2021

More streamlined database initialization #566

Open

Update installation document #880

Closed

matsduf mentioned this pull request Dec 3, 2021

Better database initialization #805

Open

ghost deleted the create-db-script branch March 3, 2022 10:45

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a unique set of definitions to initialize the database #834

Use a unique set of definitions to initialize the database #834

ghost commented Jul 28, 2021 •

edited by ghost

Loading

hannaeko commented Jul 29, 2021

ghost commented Jul 29, 2021

matsduf commented Aug 1, 2021

matsduf Aug 1, 2021

ghost commented Aug 2, 2021

ghost commented Aug 4, 2021

hannaeko commented Aug 4, 2021

ghost commented Aug 5, 2021

matsduf left a comment

ghost commented Aug 9, 2021

matsduf commented Aug 9, 2021

ghost commented Aug 12, 2021

matsduf commented Aug 12, 2021

ghost commented Aug 16, 2021

mattias-p commented Aug 17, 2021

ghost commented Aug 17, 2021

ghost commented Aug 17, 2021

ghost commented Sep 9, 2021

ghost commented Sep 13, 2021

hannaeko left a comment

hannaeko Sep 13, 2021

ghost Sep 13, 2021

matsduf Sep 13, 2021

ghost Sep 13, 2021

matsduf Sep 13, 2021

ghost Sep 14, 2021

matsduf Sep 13, 2021

Use a unique set of definitions to initialize the database #834

Use a unique set of definitions to initialize the database #834

Conversation

ghost commented Jul 28, 2021 • edited by ghost Loading

Purpose

Context

Changes

How to test this PR

hannaeko commented Jul 29, 2021

ghost commented Jul 29, 2021

matsduf commented Aug 1, 2021

Choose a reason for hiding this comment

ghost commented Aug 2, 2021

ghost commented Aug 4, 2021

hannaeko commented Aug 4, 2021

ghost commented Aug 5, 2021

matsduf left a comment

Choose a reason for hiding this comment

ghost commented Aug 9, 2021

matsduf commented Aug 9, 2021

ghost commented Aug 12, 2021

matsduf commented Aug 12, 2021

ghost commented Aug 16, 2021

mattias-p commented Aug 17, 2021

ghost commented Aug 17, 2021

ghost commented Aug 17, 2021

ghost commented Sep 9, 2021

ghost commented Sep 13, 2021

hannaeko left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 28, 2021 •

edited by ghost

Loading