MDEV-31772 Innodb_rows_* missing

[dr-m]

trafficstars

• [x] The Jira issue number for this PR is: MDEV-31772

Description

In commit 0dab74ff3fbebfcaf899ad439b17c8077f66d78a (MDEV-28539) a number of counters were removed. Due to popular demand, we are reinstating some of the counters, in a way that incurs less performance penalty.

We reinstate the following global status variables: innodb_rows_read innodb_rows_deleted innodb_rows_inserted innodb_rows_updated

These counters will also include whatever was previously covered by innodb_system_rows_* counters.

We also add the INFORMATION_SCHEMA.INNODB_METRICS counters dml_reads, dml_inserts, dml_delete, dml_updates, but not their dml_system_* counterparts.

ha_innobase::external_lock(): Update the global export_vars.rows from the local ha_innobase::rows counters at the end of each SQL statement.

How can this PR be tested?

The regression test suite is extended, but we would also want to test the performance impact of this.

I would also like to know if we really have to reinstate the information_schema.innodb_metrics counters. I would like to deprecate and remove that interface, and just export everything as global status variables.

Basing the PR against the correct MariaDB version

• [ ] This is a new feature and the PR is based against the latest MariaDB development branch.
• [x] 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

• [x] 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.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[dr-m]

I ran a simple performance test: Sysbench oltp_update_non_index, 5×10000 rows, 256 concurrent connections, 60 seconds, CPU bound, RAM disk.

revision	average throughput/tps	run1/tps	run2/tps	run3/tps
09049fe496eea1c19cd3ce80a788fa4b75d9609e (10.11 baseline)	197918.43	197257.75	198060.00	198437.55
5284d6174e8771402c5db98f5227998b62e99dad	196706.34	196827.54	196510.00	196781.49

I am not happy about the 0.6% performance loss. I will experiment with sharding the export_vars.rows.

[dr-m]

I tried a few other ideas:

• splitting export_vars.rows into 128 shards
• updating the totals only on ha_innobase::close() (flush tables) and not at the end of each statement
• not updating the innodb_rows_read counter:

diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc
index b3b082ac449..9eddcdee2d2 100644
--- a/storage/innobase/handler/ha_innodb.cc
+++ b/storage/innobase/handler/ha_innodb.cc
@@ -9079,7 +9079,7 @@ ha_innobase::index_read(
 	case DB_SUCCESS:
 		error = 0;
 		table->status = 0;
-		rows.read++;
+		//rows.read++;
 		break;
 
 	case DB_RECORD_NOT_FOUND:
@@ -9335,7 +9335,7 @@ ha_innobase::general_fetch(
 	case DB_SUCCESS:
 		error = 0;
 		table->status = 0;
-		rows.read++;
+		//rows.read++;
 		break;
 	case DB_RECORD_NOT_FOUND:
 		error = HA_ERR_END_OF_FILE;

variant	average throughput/tps	run1/tps	run2/tps	run3/tps
bdf65893ddb2ce0552a20e53e2358350521d2d35 (10.11 baseline)	236735.67	238188.90	234919.27	237098.83
1 shard (c8fa41cac0d15f670094322464a15821dca87348 a.k.a. rebased 5284d6174e8771402c5db98f5227998b62e99dad)	234799.73	235391.10	234028.00	234980.08
1 shard, updated on ha_innobase::close()	235422.28	235362.99	235095.15	235808.70
1 shard, updated on ha_innobase::close(), no innodb_rows_read	235282.07	235984.95	233536.71	236324.56
128 shards (bdba02ddd17871f8f5fb6beade375692e90cf8ce)	236448.81	236424.07	235775.24	237147.11
128 shards, no innodb_rows_read	237621.26	237419.82	237657.59	237786.36
128 shards, updated on ha_innobase::close() (dffef77b5bec3929baf271184cfee2035fa8f257)	233779.64	233934.62	233604.62	233799.69
128 shards, updated on ha_innobase::close(), no innodb_rows_read	234488.34	235796.12	232811.35	234857.56

My benchmark is not reliable, because there is rather large variance for the numbers, The last 2 rows indicate a worse regression contrary to my expectations. For the baseline, I think I had observed one run that exceeded 240 ktps. The lowest that I got for it was 232404.93 tps.

[dr-m]

e9ccbc275796f24b97be09395af1f6378e636842 implements @FooBarrior’s suggestion of updating transaction-level counters.

I was thinking if there would be an even better way, to also make the counters available in ANALYZE FORMAT=JSON (trace_engine_stats(), 6e484c3bd99a6709e3554f32a99167042ea88496). The challenges are:

1. What if the thread pool migrates a transaction between threads while it is waiting for a lock? Is the call to handler::ha_handler_stats_reset() in TABLE::init() even correct with respect to the thread pool?
2. How to traverse all thread-local row operation counts to update the global counters? At trx_t::commit_cleanup(), or is there some way to invoke this less often? Again, this would seem to be a bottleneck due to cache line contention on the global counters.

Perhaps it is better to revise e9ccbc275796f24b97be09395af1f6378e636842 so that trx_t::commit_cleanup() will update nothing, and instead the counters will be updated by traversing trx_pools.

[dr-m]

With e9ccbc275796f24b97be09395af1f6378e636842 there was a clear performance regression. Let us see if eee6f9a0237f21c36cbc1370341b73a78bc5de20 is any better. It will avoid adding innodb_rows_read. The modification counters will only be updated in the slow code path of trx_t::commit_in_memory(). Thus, the only performance penalty for read-only transactions would be the zeroing of the counters in trx_init().