Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setting a database baseline #1439

Merged
merged 11 commits into from
Jun 27, 2024
Merged

Setting a database baseline #1439

merged 11 commits into from
Jun 27, 2024

Conversation

fnkbsi
Copy link
Contributor

@fnkbsi fnkbsi commented Apr 20, 2024

Some combination of DB versions and OS versions seems to have problems with the DB migration scripts.
@juherr suggested (#1394 (comment)) a baseline script to solve the issues (e.g. #1417).
The PR #1394 and #1428 addresses the same issues, but changes the existing migration scripts, which could causes trouble at updating existing Steve servers.

The baseline also improved slightly the build process, because less DB build/migration operation are executed.

The script is exported (by mariadb-dump) from a fresh build Steve instance. The baseline creates all tables and views and inserts the data of the setting table.

@juherr
Copy link
Contributor

juherr commented Apr 20, 2024

That's nice but, as I know, it won't work because baseline need at least Flyway Teams 😓
https://documentation.red-gate.com/fd/feature-glossary-165740620.html

@fnkbsi
Copy link
Contributor Author

fnkbsi commented Apr 20, 2024

That's nice but, as I know, it won't work because baseline need at least Flyway Teams 😓 https://documentation.red-gate.com/fd/feature-glossary-165740620.html

I tried it and it worked (with a additional migration script). So I hope maven use the Flyway baseline (command) which is included in the Flyway Community version.
Can someone confirm?

@juherr
Copy link
Contributor

juherr commented Apr 20, 2024

I didn't notice baseline command is available, thanks! I will check asap

@juherr
Copy link
Contributor

juherr commented May 2, 2024

@fnkbsi I didn't check yet but if the baseline is working, you should be able to remove this lines without breaking the CI.

Could you try?

@juherr
Copy link
Contributor

juherr commented May 2, 2024

@fnkbsi For historical reason, I think the sources should include the V1_0_0 baseline. WDYT?

@fnkbsi
Copy link
Contributor Author

fnkbsi commented May 6, 2024

@fnkbsi I didn't check yet but if the baseline is working, you should be able to remove this lines without breaking the CI.

Could you try?

Why should we change the privileges at the workflow script? There is no difference between the update and baseline scripts, the baseline scripts just replaces the update scripts up to the correspondent version number on a new installation.

@fnkbsi
Copy link
Contributor Author

fnkbsi commented May 6, 2024

@fnkbsi For historical reason, I think the sources should include the V1_0_0 baseline. WDYT?

Setting the baseline at version 1_0_0 is also possible, just renaming the script V1_0_0 baseline.sql to B1_0_0 baseline.sql should do the trick.
Two baseline scripts in this state of development are not necessary, so @goekay should decide which version becomes be the baseline.

@juherr
Copy link
Contributor

juherr commented May 6, 2024

Why should we change the privileges at the workflow script? There is no difference between the update and baseline scripts, the baseline scripts just replaces the update scripts up to the correspondent version number on a new installation.

Because the extra rights were due to migrations before B1_0_0.
With B1_0_0 we don't need the extra rights anymore.

just renaming the script V1_0_0 baseline.sql to B1_0_0 baseline.sql should do the trick.

That was my point, could you add B1_0_0 too?

@fnkbsi
Copy link
Contributor Author

fnkbsi commented May 8, 2024

just renaming the script V1_0_0 baseline.sql to B1_0_0 baseline.sql should do the trick.

That was my point, could you add B1_0_0 too?

If I add the B1_0_0 and don't remove the B1_0_5 the B1_0_0 script is dead code, because it will never be used. So I am hesitant to integrated it only for cosmetics.

…GRANT SELECT ON mysql.proc TO 'steve'@'%';" -v || true"
@goekay
Copy link
Member

goekay commented Jun 2, 2024

hey there, i am up to speed with this change and discussion. sorry for the delay. first of all, thanks for the PR and insightful discussion!

regarding where to set the baseline:

  • current PR takes the latest snapshot of DB and makes it a baseline. with this approach, for fresh installations, the only db migration will be B1_0_5.
  • @juherr was suggesting, i think, setting the baseline to an earlier snapshot after which versioned migrations should continue as usual. with this approach, for fresh installations, the migration sequence will be: B1_0_0, V1_0_1, V1_0_2, V1_0_3, V1_0_4, V1_0_5. afaik, he was not suggesting to keep both baselines.

i am actually fine with both. i kinda like @juherr's suggestion a bit better because granularity of changes will be kept instead of one bulk monolith.

my concern is these exports coming from heidi sql. i am not sure how it works, but i would rather prefer mysql's (or mariadb's) native dump functionality to prevent man-in-the-middle interpretations and probable errors. is this possible? or, is my concern even valid? wdyt?

talking about mysql and mariadb... i see that the matrix builds were fine but, this baseline will be compatible with both, right? nuances started to appear in new mariadb versions because of which mariadb started to drift from mysql.

@juherr
Copy link
Contributor

juherr commented Jun 2, 2024

Yes. I think major versions are better candidate for baselines.

I've proposed an alternative pr #1455 which removes mysql privileges that are useless after v1.0

@goekay
Copy link
Member

goekay commented Jun 3, 2024

I think major versions are better candidate for baselines.

i was not following semantic versioning with db migration files though. these are just linear changes to be applied.

my approach was:

  • bump the version of the lowest level every time you make a change (as long as the versions remain in single digits).
  • if the previous step is about the cause double digits, bump the higher level's version.

example 1 : 0.8.8 -> 0.8.9 (fine)
example 2: 0.8.9 -> 0.8.10 (uh oh) -> 0.9.0 (fine again)
example 3: 1.1.9 -> 1.1.10 (uh oh) -> 1.2.0 (fine again)

therefore, the baseline should and could be after the final change where the requirement for privileges is no more necessary.

@juherr
Copy link
Contributor

juherr commented Jun 3, 2024

Funny versionning convention. I didn't get that before. Thanks for the clarification.

In that case whatever version is used as baseline will be ok.
In my opinion, the best part of using a baseline is the drop of extra rights on the database.

About my PR, I made the baseline by hand and applied every fixes myself to be sure to respect the history.

@fnkbsi
Copy link
Contributor Author

fnkbsi commented Jun 3, 2024

i am actually fine with both. i kinda like @juherr's suggestion a bit better because granularity of changes will be kept instead of one bulk monolith.

In general I agree, but V1_0_1 - V1_0_4 include only 'Alter Table' commands with minor changes.
1_0_5 added two views, so the length of the script is slightly longer.

my concern is these exports coming from heidi sql. i am not sure how it works, but i would rather prefer mysql's (or mariadb's) native dump functionality to prevent man-in-the-middle interpretations and probable errors. is this possible? or, is my concern even valid? wdyt?

There are differences in the exported/dumped scripts. On a first glance I've seen two or three general differences (except the comments).
The first difference is maria-dump drops the tables and creates a new, the heidi script creates the table only if the table not exists. Then there are differences on creating temporary tables and in the executable comments. These differences I haven't further investigated yet.

talking about mysql and mariadb... i see that the matrix builds were fine but, this baseline will be compatible with both, right? nuances started to appear in new mariadb versions because of which mariadb started to drift from mysql.

Based on the github actions results, yes. Also I tested it locally with a mariadb v11 on my windows machine.

@leomwa
Copy link

leomwa commented Jun 8, 2024

FWIW, I pulled down this branch and attempted a docker compose up, but it failed:

2024-06-08 06:19:19 [INFO] Rule 1: org.apache.maven.enforcer.rules.version.RequireMavenVersion passed
2024-06-08 06:19:19 [INFO] ------------------------------------------------------------------------
2024-06-08 06:19:19 [INFO] BUILD FAILURE
2024-06-08 06:19:19 [INFO] ------------------------------------------------------------------------
2024-06-08 06:19:19 [INFO] Total time:  8.550 s
2024-06-08 06:19:19 [INFO] Finished at: 2024-06-08T04:19:19Z
2024-06-08 06:19:19 [INFO] ------------------------------------------------------------------------
2024-06-08 06:19:19 [ERROR] Failed to execute goal org.apache.maven.plugins:maven-enforcer-plugin:3.4.1:enforce (enforce-java) on project steve: 
2024-06-08 06:19:19 [ERROR] Rule 0: org.apache.maven.enforcer.rules.version.RequireJavaVersion failed with message:
2024-06-08 06:19:19 [ERROR] Detected JDK version 11.0.20 (JAVA_HOME=/opt/java/openjdk) is not in the allowed range [17,).
2024-06-08 06:19:19 [ERROR] -> [Help 1]
2024-06-08 06:19:19 [ERROR] 
2024-06-08 06:19:19 [ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
2024-06-08 06:19:19 [ERROR] Re-run Maven using the -X switch to enable full debug logging.
2024-06-08 06:19:19 [ERROR] 
2024-06-08 06:19:19 [ERROR] For more information about the errors and possible solutions, please read the following articles:
2024-06-08 06:19:19 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException

@fnkbsi
Copy link
Contributor Author

fnkbsi commented Jun 10, 2024

FWIW, I pulled down this branch and attempted a docker compose up, but it failed:

2024-06-08 06:19:19 [INFO] Rule 1: org.apache.maven.enforcer.rules.version.RequireMavenVersion passed
2024-06-08 06:19:19 [INFO] ------------------------------------------------------------------------
2024-06-08 06:19:19 [INFO] BUILD FAILURE
2024-06-08 06:19:19 [INFO] ------------------------------------------------------------------------
2024-06-08 06:19:19 [INFO] Total time:  8.550 s
2024-06-08 06:19:19 [INFO] Finished at: 2024-06-08T04:19:19Z
2024-06-08 06:19:19 [INFO] ------------------------------------------------------------------------
2024-06-08 06:19:19 [ERROR] Failed to execute goal org.apache.maven.plugins:maven-enforcer-plugin:3.4.1:enforce (enforce-java) on project steve: 
2024-06-08 06:19:19 [ERROR] Rule 0: org.apache.maven.enforcer.rules.version.RequireJavaVersion failed with message:
2024-06-08 06:19:19 [ERROR] Detected JDK version 11.0.20 (JAVA_HOME=/opt/java/openjdk) is not in the allowed range [17,).
2024-06-08 06:19:19 [ERROR] -> [Help 1]
2024-06-08 06:19:19 [ERROR] 
2024-06-08 06:19:19 [ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
2024-06-08 06:19:19 [ERROR] Re-run Maven using the -X switch to enable full debug logging.
2024-06-08 06:19:19 [ERROR] 
2024-06-08 06:19:19 [ERROR] For more information about the errors and possible solutions, please read the following articles:
2024-06-08 06:19:19 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException

SteVe doesn't support Java 11 anymore, minimum version is Java 17. You need to update the JDK in your docker

@goekay
Copy link
Member

goekay commented Jun 10, 2024

if @leomwa made a fresh pull of the branch without modifications, it should be java 17 already. Dockerfile we use references java 17. therefore, i am confused since i dont know where this java 11 comes into play.

@goekay
Copy link
Member

goekay commented Jun 23, 2024

@fnkbsi i see you made some changes after my comment. are these changes in reaction to my remarks? if yes, are you finished? is this PR stable now?

@fnkbsi
Copy link
Contributor Author

fnkbsi commented Jun 23, 2024

@goekay Yes after your remarks I changed the base of the script to a mysqldump.exe exported script. The PR is stable unless there are more comments.

@goekay
Copy link
Member

goekay commented Jun 23, 2024

thanks! migration file LGTM.

i think you can even remove the GRANT SUPER ... privileges from CI and from the readme here.

@fnkbsi
Copy link
Contributor Author

fnkbsi commented Jun 24, 2024

i think you can even remove the GRANT SUPER ... privileges from CI and from the readme here.

Without the Super privileges the workflow does not run successful. MySql need SET_USER_ID privilege (https://github.com/fnkbsi/steve/actions/runs/9641524165) and mariadb struggles with the views without the Super privilege (https://github.com/fnkbsi/steve/actions/runs/9641825097).

@goekay
Copy link
Member

goekay commented Jun 24, 2024

Without the Super privileges the workflow does not run successful.

... because the view creation dictates steve@localhost as definer which is not necessarily needed.

@goekay goekay self-requested a review June 24, 2024 19:15
@goekay
Copy link
Member

goekay commented Jun 26, 2024

@fnkbsi should i merge or do you want to? i approved the PR to signal that you can merge it.

@fnkbsi
Copy link
Contributor Author

fnkbsi commented Jun 26, 2024

@goekay: Please merge.

@goekay goekay merged commit e5a1378 into steve-community:master Jun 27, 2024
22 checks passed
@fnkbsi fnkbsi deleted the dbBaseline branch June 27, 2024 07:19
goekay added a commit that referenced this pull request Jun 27, 2024
SUPER is not needed anymore
@goekay
Copy link
Member

goekay commented Jun 27, 2024

thanks all!

faculoyarte pushed a commit to faculoyarte/steve that referenced this pull request Sep 4, 2024
SUPER is not needed anymore
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants