When addressing this, I believe that also the following scenario must be considered:

--source include/have_innodb.inc

--source include/have_debug.inc

--source include/have_debug_sync.inc

CREATE TABLE t1 (a INT) ENGINE=InnoDB;

connect (ddl,localhost,root,,);

SET DEBUG_SYNC = 'row_log_table_apply1_before SIGNAL copied WAIT_FOR conflict';

send ALTER TABLE t1 FORCE;

connection default;

SET DEBUG_SYNC = 'now WAIT_FOR copied';

BEGIN; SELECT * FROM t1;

SET DEBUG_SYNC = 'now SIGNAL conflict';

connect (backup,localhost,root,,);

SELECT * FROM t1;

sleep 1;

SELECT * FROM t1;

Here, the ALTER TABLE should be blocked in the MDL upgrade before handler::commit_inplace_alter_table(commit=true). Further DML is blocked from acquiring MDL on the table while the lock upgrade is in progress. The MDL conflict would not be resolved until connection default commits or rolls back or disconnects, or until the MDL upgrade times out (by default, in 1 year). (On MDL timeout, handler::commit_inplace_alter_table(commit=false) would be called without holding MDL_EXCLUSIVE.)

Now, replace the last SELECT with BACKUP STAGE BLOCK_DDL or similar. I tested this on the plain 10.4 branch without MDEV-5336. I do not know what would happen with the backup locks. We certainly would not want the backup end up being blocked for an extended period of time.

Last, imagine that the BEGIN; SELECT was replaced with BACKUP STAGE BLOCK_DDL. In that scenario, if that statement is blocking the MDL upgrade of the ALTER TABLE, it would be nice if concurrent DML was allowed until the backup lock is released, and the ALTER TABLE has a real chance of upgrading the MDL for the final commit.

First a comment regarding the original bug report:

It's not a deadlock as when connection 1 does commit, then all the other block are freed.

The reason you have a lock is the following:
ALTER TABLE starts by taking a global MDL_BACKUP_DDL lock (in lock table names) to protect against BLOCK_DDL, and then another lock for the table. It's waiting for the second lock here.
BACKUP STAGE BLOCK_DDL will wait until there are no DDL's running and thus have to wait for ALTER TABLE.
Don't see how we can fix that with current code without a notable redesign of where MDL LOCKS are taken. However things are not worse than before in this particular case as one can still execute DML for any other table than t1 while the server is waiting for the ALTER TABLE to finish.

Regarding Marko's comment:

>Now, replace the last SELECT with BACKUP STAGE BLOCK_DDL or similar. I tested this on the >plain 10.4 branch without MDEV-5336. I do not know what would happen with the backup
> locks. We certainly would not want the backup end up being blocked for an extended period of time.

The backup has to wait until ALTER TABLE either finishes or start copying data, the same way another ALTER TABLE on the same table has to wait.

Last, imagine that the BEGIN; SELECT was replaced with BACKUP STAGE BLOCK_DDL. In that scenario, if that statement is blocking the MDL upgrade of the ALTER TABLE, it would be nice if concurrent DML was allowed until the backup lock is released, and the ALTER TABLE has a real chance of upgrading the MDL for the final commit.

In this case the BLOCK_DDL will block ALTER TABLE just before the final commit. However any DML for other tables will continue as normal.

Imagine a long select, and any DDL waiting for it, then backup should hang as well, for the duration of the select . I do not think this was planned. Maybe a considerable redesign is needed, just to keep the promise of MDEV-5336 ?

Yes, any DDL's has to wait for SELECTS. You can consider that an inefficient too.
DDL's has to wait for other DDL's.
BACKUP_DDL will not wait for SELECTS, only for DDL's (not running ones but one that has taken the DDL lock).
However, as BACKUP_DDL doesn't block any DML, it's not that bad for it to take some time (just for longest transaction that has an ALTER TABLE waiting) to end, except that it will block new DDL's while doing that.

That said, we are experimenting with BLOCK_DDL to either go past any waiting BACKUP_DDL lock (done by re-ording lock order) or allowing other DDL's while BLOCK_DDL is waiting for the DDL lock. Either of these solution may solve this in a decent way.
However even with the current code, we have less locks than any other backup solution for MySQL/MariaDB, so we are going in the right direction.

DDL waits for SELECT, this is fine, and efficient for me.

BACKUP_DDL effectively waits for SELECT, because DDL is not running, and this is my concern. That DDL took a lock, is a technicality, what user sees is a hanging backup, waiting until SELECT is finished

In fact, what I'm is seeing MDEV-15636 because mariabackup still has lock-ddl-per-table option, and it I still has to KILL running DDLs before taking backup lock, when this option is on

I hoped that deadlock would be fixed, as it was promised by MDEV-5336.

There is no deadlock (as far as I can see or test).
The BLOCK_DDL will eventually get it's lock and then things will proceed as normal.
The user sees a lock on DDL and backup has to wait for all DDL's to get their locks or finish.
Things are much better than ever before in almost every case discussed, but I agree there are still possibilities
to make things even better and I am working on that.
Note that there is no reason to kill any queries as there are no deadlocks. All locks will be sorted out when the transaction
commits. That BLOCK DDL has to wait for the longest transaction with a waiting DDL is not optimal but not a blocker.
When we have a fix for that, there is no changes necessary in the backup code.

The deadlock I'm referring to is the mentioned MDEV-15636 (mariabackup --lock-ddl-per-table hangs in FLUSH TABLES due to MDL conflict if ALTER TABLE issued)

Since this bug is prominently featured in the description MDEV-5336(Implement LOCK FOR BACKUP) assumed the problem is known/understood. if not, here is what happens.

This happens with mariabackup that runs with --lock-ddl-per-table option

a) A connection inside mariabackup is holding MDLs for all Innodb tables. MDL is acquired as SELECT 1 from table, a transaction. Transaction is not commited until the end of backup.
b) A user connection is doing ALTER TABLE for which MDL is held. ALTER waits.
c) Another connection inside mariabackup is trying to acquire FTWRL.. It waits for ALTER in b), which is waits for transaction holding MDL in a), which is not commited until the end of backup.

This ^ is a deadlock.

To resolve the deadlock, mariabackup issues KILL QUERY for the ALTER in b). This allows FTWRL to proceed. Another possible "solution" is --no-lock which avoids FTWRL.

So,
-I tested BACKUP STAGE with mariabackup.
-I read MDEV-5336. It said in description "This lock will also solve the problem with MDEV-15636 (killing running queries that conflicts with FLUSH) as the backup locks will
not conflict with other DDL locks"
-I removed KILL QUERY from mariabackup. I wanted to see whether the statement "this lock will also solve the problem with MDEV-15636" holds.
-As a result , mariabackup hanged in a test that tests "lock-ddl-per-table" option.

My conclusion therefore was that it did not really solve the problem with MDEV-15636.

When you have a fix for that, I will remove KILL QUERY from the backup code.

(I did not decide yet, whether to remove --lock-ddl-per-table option, maybe there is some weird case, where people would still like to use it. At least it turned out to be a useful tool to test BACKUP STAGE so far)

Hi!

Thanks for the explanation, it explains clearly the problem!

The issue is how inplace/online ALTER TABLE is implemented.
It first takes a DDL lock and then open tables in shared mode. Later, in mysql_sql_alter() and mysql_inplace_alter_table() it upgrades the table to exclusive lock and this blocks because of the active transaction.
Because of the DDL lock that is hold, there is no way for BACKUP STAGE BLOCK_DDL to proceed.

It's possible to fix this for ALTER TABLE, but you have the same issue with DROP TABLE and other DDL's, so there is no easy to fix it.
Another solution would be to change lock order and first do table level locks and then backup locks. The problem with this is that we could the the backup lock while the DDL is waiting for the transaction to end, but the side effect is that while the ddl is waiting, no DML's can be done at the tables, which would be even worse.

The solution is simple to not allow --lock-ddl-per-table when using backup locks.
As we want to allow DDL's during the major part of the backup, until BLOCK_DLL, this is the right thing to do anyway.
--lock-ddl-per-table is also not needed as we now have BACKUP STAGE BLOCK_DDL that does the same, but for all tables (not only innodb tables).

I checked also this scenario with the old FTWRL:
tr1: begin ; select 1 from t1;
tr2: alter table t1;
tr3: FLUSH TABLES WITH READ LOCK;

FTWRL locks on both 10.3 and new 10.4 as it as has to wait for ALTER TABLE that takes a MDL_INTENTION_EXLUSIVE/MDL_BACKUP_DDL lock which blocks the MDL_SHARED/MDL_BACKUP_FTWRL1 lock needed in lock_global_read_lock()

As I said, I'm not certain of the usefulness of lock-ddl-per-table.
I'd rather keep it, and I would also keep other things like --no-lock. The options are there, they are documented, and in case of bugs in BACKUP STAGE people could still can use them as workaround.

I can document them as "not needed anymore", and "deprecated", but they have not previously gone through any deprecation phase, so they need to be there for the time being.

I would also like to see fix in the server.
The promise of the new LOCK is that it does not block in presence of SELECTS, UPDATES, does not wait for ends of transactions, and even is able to run simultaneously with running DDLs .
Basically, the promise if that it is instant, almost always.

This needs to still hold if there is a non-running DDL present, otherwise BACKUP LOCK waits for all of the above

Yes, it's instant under some premisses. One is that the connection that is using BACKUP LOCKS is not holding any locks of it self.
There is no easy way to fix that if the connection holds locks that causes other connections to wait in DML or DDL, then it will cause BACKUP LOCK's to fail.

The purpose of BACKUP LOCK BLOCK_DDL is to wait for all running DDL's (those that has DDL lock) to finish and stop new's from being issued. This exactly what it does.
ALTER TABLE is fixed so that while it's copying data it's not blocking BLOCK_DDL.

I am open for suggestion of how to fix it that doesn't involve dark magic .
This discussions is however better to have on IRC

reworded the description so there is no reference to deadlock, as it seems confusing for some readers. Now it is called an "unexpected wait"