MDEV-33750: Sync configuratons from Debian Salsa #3146

illuusio · 2024-03-22T07:46:17Z

The Jira issue number for this PR is: MDEV-33750

Description

Sync configurations and changes from Debian Salsa. This is mostly same as PR #2778 as it stalled and closed but these commits can be ironed to be valuable addition.

Release Notes

These commits does not affect release notes.

How can this PR be tested?

There is Salsa-CI run 655744 which will have some failing tests.

This could be test manually in Debian Sid with
debian/autobake-debs.sh

and then manually installed.

Basing the PR against the correct MariaDB version

This is a new feature and the PR is based against the latest MariaDB development branch.
This is a bug fix and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

fauust

Overall, this looks good. It's very good that those bash scripts are verified with shellcheck and that you commented when and why you decided to skip some rules.

Some remarks though. For the reviewers, it's easier to have specific 'lint' commits, I think it would be good to have them in the future so we can easily separate cosmetic changes from functional ones.

On the linter aspect, I may be wrong but we do not have bash syntax guidelines and we probably should (ping @LinuxJedi). I personally try to follow https://google.github.io/styleguide/shellguide.html and use https://github.com/mvdan/sh with -d -i 2 -ci.

For instance, I personally prefer if; then or for; do on the same line, it makes script smaller and do not impact readability. But that's OK as it is now if we decide that it should be how we write bash loops.

fauust · 2024-03-25T09:35:28Z

debian/mariadb-server.postinst

 then
- echo "UPDATE mysql.user SET plugin='unix_socket' WHERE plugin='auth_socket';" |
- mariadbd --skip-innodb --key_buffer_size=0 --default-storage-engine=MyISAM --bootstrap 2> /dev/null
+ echo "UPDATE mysql.user SET plugin='unix_socket' WHERE plugin='auth_socket';" | \


The \ should not be necessary here (but maybe it's your linter that automatically adds it). More on the linter on general comment.

Both seems to work with BASH the same and shfmt say there should not be and beautysh does not case neither shellcheck so I think I'll remove them.

Right, both seem to work:

± cat test.sh #!/bin/bash set -e echo test-with-backslash | \ cat echo test-no-backslash | cat ± ./test.sh test-with-backslash test-no-backslash

fauust · 2024-03-25T09:41:43Z

debian/mariadb-server.mariadb.init

@@ -156,16 +177,22 @@ case "${1:-''}" in
 # Start MariaDB!
 /usr/bin/mariadbd-safe "${@:2}" 2>&1 >/dev/null | $ERR_LOGGER &

- for i in $(seq 1 "${MYSQLD_STARTUP_TIMEOUT:-30}"); do
+ for _ in $(seq 1 "${MYSQLD_STARTUP_TIMEOUT:-30}")


is _ var a bash best practice thing? Just curious.
Also, why not removing seq as you did on line 224.

For constituency I think as new one also have '_'
Reason is that new syntax {1..30} does not allow to use variable.. although it could be half minute without variable possibility.

I don't know if my new version is better or worse but there is no seq anyone and variable MYSQLD_STARTUP_TIMEOUT is defined if there is non.

is _ var a bash best practice thing? Just curious. Also, why not removing seq as you did on line 224.

Yes: https://www.shellcheck.net/wiki/SC2034#intentionally-unused-variables

fauust · 2024-03-25T10:02:20Z

@illuusio, @grooverdan why is the CI not running for this PR? Since this PR impact starting and probably upgrading MariaDB on Debian/Ubuntu, you should make sure that install and upgrade (minor and major) tests pass on BB.

grooverdan · 2024-03-26T03:17:17Z

debian/additions/debian-start.inc.sh

- --force -e "{}" &>"${tempfile}"
+ # The $MARIADB is intentionally used to expand into a command and arguments
+ # shellcheck disable=SC2086
+ LC_ALL=C echo '


At the risk of being too pedantic - beginning of this function says "trigging myisam-recover" and it doesn't. So minimally a better comment there. Somehow a basic open of the table is sufficient - was there a decision recorded for this (its a big search - I couldn't find it)? check table {} FAST could have been used as SQL.

Also (for later), looking for filesystem based files might be quicker than information_schema. And Aria might auto-recover (especially after 10.6).

LC_ALL=C is now on echo (which serves no purpose) rather than $MARIADB which only effects output into the email. So I'm not sure there's a purpose in including it at all.

SC2086 is due to $MARIADB being unquoted (twice now which could be corrected.

xargs --no-run-if-empty would be worth saving a needless execution.

Is xargs -P desired to perform concurrent

As I didn't get how to make this CHECK table (I have to read documentation better) I just updated it little bit.

Checked with Monty - Aria tables will auto recover. MyISAM won't.
Option 1:

remove all between set +e / -e as old stuff assuming we never need to handle crashed upgrade from < 10.4, maybe users can handle this if it does happen.
Option 2:
execute as a single SQL string:

select concat('check table ', group_concat(CONCAT('`', TABLE_SCHEMA, '`.`', TABLE_NAME, '`')), ' fast') into @check FROM information_schema.TABLES WHERE TABLE_SCHEMA<>"INFORMATION_SCHEMA" AND TABLE_SCHEMA<>'PERFORMANCE_SCHEMA' AND ENGINE='MyISAM' OR (TABLE_SCHEMA='mysql' and TABLE_NAME='time_zone_leap_second'); execute immediate @check;

rational:

check table fast - doesn't need to fully read the entire table (unsure if information_schema will open table)

MyISAM only is needed

mysql.time_zone_leap_second include to ensure a single table is there.

quoting - in case of some table names, isn't fully coverage

debian/additions/debian-start.inc.sh

debian/mariadb-server.mariadb.init

grooverdan · 2024-03-26T03:27:35Z

debian/mariadb-server.mariadb.init

+ else
+ ERROR_LOG_FILE="$(mktemp).err"
+ echo # ensure newline
+ timeout --kill-after=20 10 /usr/bin/mysqld_safe "${@:2}" --log-error="$ERROR_LOG_FILE"


mariadbd-safe

Ok with this one.. there is also file: debian/additions/mariadb.conf.d/50-mysqld_safe.cnf which is also installed..

exe not changed yet.

Please see PR #3240 for this as it should take care of these ones

debian/mariadb-server.postinst

grooverdan · 2024-03-26T03:38:26Z

debian/rules

 endif

 BUILDDIR := builddir
 DEB_VERSION_REVISION := $(shell echo $(DEB_VERSION) | sed -e 's/^.*-//')
 DEB_VERSION_VERSION := $(shell echo $(DEB_VERSION) | sed -e 's/^.*:\(.*\)\(-\|+\).*/\1/')
 DEB_VERSION_MAJOR := $(shell echo $(DEB_VERSION_VERSION) | sed -e 's/^\(.*\)\..*$$/\1/')
 RELEASE := $(shell lsb_release -r -s) # Use changelog based DEB_DISTRIBUTION instead?
-TMP:=$(CURDIR)/debian/tmp
+TMP := $(CURDIR)/debian/tmp


Should TMP be an export? Its not used by rules (aka this makefile)?

Actually it's used in many places after check..

Its not used by rules (aka this makefile)?

Yes it is, $(TMP) appears 17 times in debian/rules.

debian/rules

debian/mariadb-server.postrm

grooverdan · 2024-03-26T03:52:17Z

debian/tests/upstream

 elif [ "$ARCH" = "armhf" ] || [ "$ARCH" = "i386" ]
 then
- echo "main.failed_auth_unixsocket : Test returns wrong exit code on armhf and i386 (but only in debci) https://jira.mariadb.org/browse/MDEV-23933" >> $SKIP_TEST_LST
+ echo "main.failed_auth_unixsocket : Test returns wrong exit code on armhf and i386 (but only in debci) https://jira.mariadb.org/browse/MDEV-23933" >> "$MTR_SKIP_TEST_LIST"


MDEV-23933 is fixed.

So this can be removed altogether?

debian/tests/upstream

debian/mariadb-server.mariadb.init

ottok · 2024-05-07T01:31:40Z

debian/mariadb-server.mariadb.init

@@ -176,8 +176,21 @@ case "${1:-''}" in
 # Start MariaDB!
 /usr/bin/mariadbd-safe "${@:2}" 2>&1 >/dev/null | $ERR_LOGGER &

- for _ in $(seq 1 "${MYSQLD_STARTUP_TIMEOUT:-30}")


Why not use seq, why need to stop using it?

ottok · 2024-05-07T02:00:28Z

👍 Unsurprisingly this PR looks good to me since it is upstreaming changes done in Debian in e.g.

Hope to see it merged soon!

Based on riscv64 build logs the RocksDB plugin currently builds fine on it, and the riscv64 platform is 64-bit and has correct endianness for RocksDB, so all the pre-requisites for it working exist, so it should work.

- mariadb-server: recursive-privilege-change "chown -R" - use correct URL https://jira.mariadb.org everywhere - dependency-is-not-multi-archified libmariadb-dev-compat depends on libmariadb-dev (multi-arch: no) - dependency-is-not-multi-archified mariadb-plugin-gssapi-client depends on mariadb-client (multi-arch: no)

The way DPKG_GENSYMBOLS_CHECK_LEVEL was exported did not actually have any effect on the build. Fix the syntax so that build will indeed fail if there there are new symbols in new upstream version.

- Unify on MTR_SKIP_TEST_LIST in both d/rules and autopkgtests - Unify MTR command in both d/rules and autopkgtests - Make d/rules section more verbose to help debugging why tests sometimes ran and sometimes not - If MTR fails, make the log a bit more verbose (inspired by https://github.com/MariaDB/buildbot/pull/76/files)

…5 in Debian Fix a large amount of minor fixes to maintainer scripts and other done downstream in the official Debian packaging. Changes include: https://salsa.debian.org/mariadb-team/mariadb-server/-/commit/38198d0b9e1c7821ddd074e308b25034bdcdce5b > Limit check of running mysqld/mariadbd to system users (Closes: #1032047) > > If a random user has their own copy of mysqld/mariadbd running, the > dpkg maintainer script should not care about it. https://salsa.debian.org/mariadb-team/mariadb-server/-/commit/8116354d22e0b8eade6d8f0594c57300d5d5cff5 > Make error more helpful in case server restart fails (Related: #1033234) > > Bugs such as https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1033234 > and https://bugs.launchpad.net/ubuntu/+source/mariadb-10.6/+bug/2011293 > show that currently dpkg stopping on service stop/start does not have > a very helpful error message. https://salsa.debian.org/mariadb-team/mariadb-server/-/commit/8675e97202171812a1afdb438a17cb29a99836fb > Complement upstream commits with more complete mysql->mariadb conversion > > The upstream commit 952af4a missed some places where 'mysql' or > 'MySQL' can and should be converted to use 'mariadb' or 'MariaDB'. https://salsa.debian.org/mariadb-team/mariadb-server/-/commit/c98361330063e7dccbf8d21aa20e48179ba5c1e4 > Fix indentation in Debian post and pre scripts > > There is several misindentation inside Debian post and pre > installation scripts. False indentation with space as indent space > should be 2 and indentation with tabs. > > Adopt upstream commit 7cbb45d in Debian by conserving customizations > in: > - debian/mariadb-server.postinst > - debian/mariadb-server.postrm > - debian/mariadb-server.preinst https://salsa.debian.org/mariadb-team/mariadb-server/-/commit/d0bcab443fa6d44084dc674ba29b79516c6239ba > Ensure spaces are used everywhere instead of tabs for indentation https://salsa.debian.org/mariadb-team/mariadb-server/-/commit/0300a9157cc69f75e01ac9c0d6e033d8be661492 > Complement previous upstream commits to fix Shellcheck issues > > - Unify if/then and while/do on separate lines > - Fix indentation to be consistent > - Use "$()" instead of backticks for subshells > - Exit code cannot be -1, must be 0-255 > - Remove unused variables MYCHECK and MYCHECK_PARAMS > - Rewrite messy command-line database calls to an easier to read form > that does exactly the same > - Use 'command -v' test instead of 'which' > > With this commit, all of debian/* is Shellcheck clean. Also * Update mariadb.conf.d template to tell users where to create logdir if they are not using journald * Remove use of work 'slave' * Add minor workaround for Debian Bug #1022994 if TMPDIR is empty * Make start/stop in maintainer scripts correctly check mariadbd ownership and only start/stop processes owned by root or 'mysql' * Remove obsolete 'NO_UPDATE_BUILD_VERSION=1' as it did not affect the RocksDB build reproducibility as previously assumed * Run 'wrap-and-sort -av'

Adapted from upstream commit 8171f9d but separated only the datadir section from the commit and wrote it in a way that does not trigger Shellcheck or English grammar nags. This check is intentionally not added to the preinst script as was done upstream in 30fb72c as the preinst script will always create the data directory if missing, and thus checking for it right after the creation is moot.

…ures MariaDB installs/upgrades in Docker containers (and elsewhere where systemd is not used) occasionally fail with output like: Starting MariaDB database server: mariadbd . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . failed! invoke-rc.d: initscript mariadb, action "start" failed. dpkg: error processing package mariadb-server-10.5 (--configure): installed mariadb-server-10.5 package post-installation script subprocess returned error exit status 1 This is not very helpful. Thus extend the init script to try the server start/restart one more time but with error log defined separately, and then print out the error log contents of this single start attempt. ... Starting MariaDB database server: mariadbd . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 230103 01:06:48 mysqld_safe Can't log to error log and syslog at the same time. Remove all --log-error configuration options for --syslog to take effect. 230103 01:06:48 mysqld_safe Logging to '/tmp/tmp.JlE4sdUMZz.err'. 230103 01:06:49 mysqld_safe Starting mariadbd daemon with databases from /var/lib/mysql Running '/etc/init.d/mariadb start' failed with error log: 230103 01:06:49 mysqld_safe Starting mariadbd daemon with databases from /var/lib/mysql 2023-01-03 1:06:49 0 [Note] /usr/sbin/mariadbd (mysqld 10.5.18-MariaDB-0+deb11u1) starting as process 10417 ... 2023-01-03 1:06:49 0 [Note] InnoDB: Uses event mutexes 2023-01-03 1:06:49 0 [Note] InnoDB: Compressed tables use zlib 1.2.11 2023-01-03 1:06:49 0 [Note] InnoDB: Number of pools: 1 2023-01-03 1:06:49 0 [Note] InnoDB: Using crc32 + pclmulqdq instructions 2023-01-03 1:06:49 0 [Note] InnoDB: Using Linux native AIO 2023-01-03 1:06:49 0 [Note] InnoDB: Initializing buffer pool, total size = 134217728, chunk size = 134217728 2023-01-03 1:06:49 0 [Note] InnoDB: Completed initialization of buffer pool 2023-01-03 1:06:49 0 [ERROR] InnoDB: Invalid flags 0x4800 in ./ibdata1 ...

Make small adjustment to MyISAM recovery function SQL statement and how to handle it.

Make all init.d script for loops to use new {1..5} syntax and rework one not to use seq as all the rest use new Bash syntax.

For making smoke test work sync current Debian Salsa-CI version for making needed changes

…oke test pass autopkgtests from package are not passing currently and make them pass with upgrading Salsa-CI YAML file.

illuusio requested a review from grooverdan March 22, 2024 07:46

grooverdan requested a review from fauust March 25, 2024 01:56

fauust approved these changes Mar 25, 2024

View reviewed changes

grooverdan reviewed Mar 26, 2024

View reviewed changes

tonychen2001 reviewed Mar 26, 2024

View reviewed changes

debian/mariadb-server.mariadb.init Show resolved Hide resolved

illuusio force-pushed the 10.11-MDEV-33750-update-debian-salsa branch from 805be1a to 6822c00 Compare April 8, 2024 09:00

illuusio force-pushed the 10.11-MDEV-33750-update-debian-salsa branch 3 times, most recently from 6fe07c0 to 712baf9 Compare April 17, 2024 09:54

illuusio force-pushed the 10.11-MDEV-33750-update-debian-salsa branch 2 times, most recently from 6cddd71 to 64b369f Compare May 6, 2024 07:01

ottok reviewed May 7, 2024

View reviewed changes

illuusio force-pushed the 10.11-MDEV-33750-update-debian-salsa branch from d796e2c to 13aefed Compare May 8, 2024 07:49

ottok and others added 11 commits May 13, 2024 09:17

MDEV-33750: Enable mariadb-plugin-rocksdb for riscv64

8cfffc5

Based on riscv64 build logs the RocksDB plugin currently builds fine on it, and the riscv64 platform is 64-bit and has correct endianness for RocksDB, so all the pre-requisites for it working exist, so it should work.

MDEV-33750: Fix DPKG_GENSYMBOLS_CHECK_LEVEL so it actually takes effect

1a369af

The way DPKG_GENSYMBOLS_CHECK_LEVEL was exported did not actually have any effect on the build. Fix the syntax so that build will indeed fail if there there are new symbols in new upstream version.

MDEV-33750: Rework MyISAM recovery script

b4fa75a

Make small adjustment to MyISAM recovery function SQL statement and how to handle it.

MDEV-33750: Remove seq in Debian init.d for-loop

0492805

Make all init.d script for loops to use new {1..5} syntax and rework one not to use seq as all the rest use new Bash syntax.

MDEV-33750: Sync smoke test from newer version of Debian Salsa-CI

58deaed

For making smoke test work sync current Debian Salsa-CI version for making needed changes

MDEV-33750: Update few Debian Salsa-CI tests from upstream to make sm…

9bcec2c

…oke test pass autopkgtests from package are not passing currently and make them pass with upgrading Salsa-CI YAML file.

illuusio force-pushed the 10.11-MDEV-33750-update-debian-salsa branch from 7f32bf3 to 9bcec2c Compare May 13, 2024 06:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MDEV-33750: Sync configuratons from Debian Salsa #3146

MDEV-33750: Sync configuratons from Debian Salsa #3146

illuusio commented Mar 22, 2024 •

edited

fauust left a comment

fauust Mar 25, 2024

illuusio Apr 16, 2024

ottok May 7, 2024

fauust Mar 25, 2024

illuusio Apr 16, 2024

illuusio May 6, 2024

ottok May 7, 2024

fauust commented Mar 25, 2024

grooverdan Mar 26, 2024

grooverdan May 3, 2024

illuusio May 6, 2024

grooverdan May 7, 2024

grooverdan Mar 26, 2024

illuusio Apr 16, 2024

grooverdan May 3, 2024

illuusio May 6, 2024 •

edited

grooverdan Mar 26, 2024

illuusio May 6, 2024

ottok May 7, 2024

grooverdan Mar 26, 2024

illuusio Apr 17, 2024

grooverdan May 3, 2024

ottok May 7, 2024

ottok May 15, 2024

ottok commented May 7, 2024 •

edited

MDEV-33750: Sync configuratons from Debian Salsa #3146

Are you sure you want to change the base?

MDEV-33750: Sync configuratons from Debian Salsa #3146

Conversation

illuusio commented Mar 22, 2024 • edited

Description

Release Notes

How can this PR be tested?

Basing the PR against the correct MariaDB version

PR quality check

fauust 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

fauust commented Mar 25, 2024

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

illuusio May 6, 2024 • edited

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

Choose a reason for hiding this comment

ottok commented May 7, 2024 • edited

illuusio commented Mar 22, 2024 •

edited

illuusio May 6, 2024 •

edited

ottok commented May 7, 2024 •

edited