[bsrikanth-mariadb]

Problem:
When reading the reply of the remote server times out, the client
library compiled into the server returns without closing the connection
and without setting a client error. FederatedX took the raw
ER_NET_READ_INTERRUPTED for an error of the remote server and went on
using a connection that still had the reply of the failed statement to
arrive. As a result:

• the next statement on that connection failed in various ways, either
  with ER_QUERY_ON_FOREIGN_DATA_SOURCE ("Server has gone away") or with
  an error packet that mysqltest took for a malformed packet;
• inside a transaction, the following statements continued on a new
  remote session in autocommit mode, so they were committed on the
  remote server one by one even though the local transaction was later
  rolled back. The savepoints of the lost session were also still
  remembered by FederatedX.
  The same happened when the connection was lost while reading the reply.

Fix:

• After a timeout FederatedX closes the connection, so that the next
  statement opens a new one. The client still gets the error of the
  timeout, ER_NET_READ_INTERRUPTED.
• Whenever the connection is gone after a failed statement, timeout or
  not, the state of the lost session (autocommit mode, savepoints) is
  forgotten and the local transaction is marked to be rolled back, so a
  new connection starts the transaction again. The closed connection is
  also freed, which was leaked before.
• Errors of the client library are reported as
  ER_QUERY_ON_FOREIGN_DATA_SOURCE by ha_federatedx::info() as well, the
  same way as by the pushed down multi-table UPDATE/DELETE.
• The error state of ha_federatedx, read by info(), is initialized.

Test: federatedx_pushdown_upd_del covers the timeout of a pushed down
multi-table UPDATE and DELETE and of reading a result, the statements
after it, the same inside a transaction, a connection killed in the
middle of a statement, client library errors in info(), and that an
error of the remote server which merely has the code of
ER_NET_READ_INTERRUPTED is not taken for a timeout.

[DaveGosselin-MariaDB]

The following MTR test case shows a regression introduced by this patch but which works correctly on the parent commit. To summarize, the test inserts 1, 2 and 3 into a FederatedX table inside one local transaction and then commits. A trigger on the remote table rejects a=2 with an error whose code, 1159, is the same as a read timeout's error code. On the parent commit, the failed INSERT keeps the remote error and the transaction, and a=1 and a=3 are committed. With this patch, close_on_net_timeout() takes that error for a local read timeout, closes the healthy connection, and rolls back the transaction, so only a=3 makes it to the remote table. The parent commit does have a timeout bug, fixed by this patch. The trigger test never times out, though, so it apparently doesn't reach that path.

--source have_federatedx.inc
--source include/federated.inc
--source include/have_innodb.inc

connection slave;
CREATE DATABASE f2db;
CREATE TABLE f2db.s1 (a INT) ENGINE=InnoDB;
--echo # The remote server rejects a=2 with an error packet whose code is 1159
delimiter |;
CREATE TRIGGER f2db.tr BEFORE INSERT ON f2db.s1 FOR EACH ROW
BEGIN
  IF NEW.a = 2 THEN
    SIGNAL SQLSTATE 'HY000' SET MYSQL_ERRNO = 1159,
      MESSAGE_TEXT = 'error raised by remote trigger';
  END IF;
END|
delimiter ;|

connection master;
CREATE DATABASE f2db;
--replace_result $SLAVE_MYPORT SLAVE_PORT
eval CREATE TABLE f2db.s1 (a INT)
ENGINE=FEDERATED CONNECTION='mysql://[email protected]:$SLAVE_MYPORT/f2db/s1';

BEGIN;
INSERT INTO f2db.s1 VALUES (1);
--error 1159,2013,1430,1296
INSERT INTO f2db.s1 VALUES (2);
SHOW WARNINGS;
SELECT @@in_transaction;
INSERT INTO f2db.s1 VALUES (3);
COMMIT;

connection slave;
--echo # Rows on the remote server
SELECT a FROM f2db.s1 ORDER BY a;

connection master;
DROP DATABASE f2db;
connection slave;
DROP DATABASE f2db;
connection default;
source include/federated_cleanup.inc;

[DaveGosselin-MariaDB]

This new code is not exercised by the tests modified in this same commit.

[DaveGosselin-MariaDB]

This is probably fine, because we expect to cleanup and close down connections on such a failure, but this particular path isn't exercised by the tests modified on this commit.

[bsrikanth-mariadb]

The following MTR test case shows a regression introduced by this patch but which works correctly on the parent commit. To summarize, the test inserts 1, 2 and 3 into a FederatedX table inside one local transaction and then commits. A trigger on the remote table rejects a=2 with an error whose code, 1159, is the same as a read timeout's error code. On the parent commit, the failed INSERT keeps the remote error and the transaction, and a=1 and a=3 are committed. With this patch, close_on_net_timeout() takes that error for a local read timeout, closes the healthy connection, and rolls back the transaction, so only a=3 makes it to the remote table. The parent commit does have a timeout bug, fixed by this patch. The trigger test never times out, though, so it apparently doesn't reach that path.

--source have_federatedx.inc
--source include/federated.inc
--source include/have_innodb.inc

connection slave;
CREATE DATABASE f2db;
CREATE TABLE f2db.s1 (a INT) ENGINE=InnoDB;
--echo # The remote server rejects a=2 with an error packet whose code is 1159
delimiter |;
CREATE TRIGGER f2db.tr BEFORE INSERT ON f2db.s1 FOR EACH ROW
BEGIN
  IF NEW.a = 2 THEN
    SIGNAL SQLSTATE 'HY000' SET MYSQL_ERRNO = 1159,
      MESSAGE_TEXT = 'error raised by remote trigger';
  END IF;
END|
delimiter ;|

connection master;
CREATE DATABASE f2db;
--replace_result $SLAVE_MYPORT SLAVE_PORT
eval CREATE TABLE f2db.s1 (a INT)
ENGINE=FEDERATED CONNECTION='mysql://[email protected]:$SLAVE_MYPORT/f2db/s1';

BEGIN;
INSERT INTO f2db.s1 VALUES (1);
--error 1159,2013,1430,1296
INSERT INTO f2db.s1 VALUES (2);
SHOW WARNINGS;
SELECT @@in_transaction;
INSERT INTO f2db.s1 VALUES (3);
COMMIT;

connection slave;
--echo # Rows on the remote server
SELECT a FROM f2db.s1 ORDER BY a;

connection master;
DROP DATABASE f2db;
connection slave;
DROP DATABASE f2db;
connection default;
source include/federated_cleanup.inc;

sure, included this test case as well. It now succeeds.

[bsrikanth-mariadb]

Tried to get coverage as much as possible. But, there could be gaps.

[DaveGosselin-MariaDB]

Tried to get coverage as much as possible. But, there could be gaps.

Please use GCOV to measure what lines are tested by your changes. Obviously there are combinations across query types that increase complexity and GCOV won't necessarily indicate whether those are exercised, but it is useful for checking that the tests in the patch exercise the modified code.