Try setting wsrep_sync_wait for mysql connections

[yhabteab]

This PR is an alternative version of #665 and utilises what is described in #665 (comment).

Using this PR avoids the need for custom driver registration and also eliminates the need for any other custom connectors/drivers that aren't of type driver.RetryConnector. This PR also adds an option to driver.RetryConnector to post initialise a connection after Connector#Connect().

closes #665

[julianbrost]

Before I go too deep into implementation details: @lippserd Do you see any issue in removing registering the named drivers in the sql package?

Apart from that, just one comment of something I found strange so far.

[lippserd]

Before I go too deep into implementation details: @lippserd Do you see any issue in removing registering the named drivers in the sql package?

Nope, removing them is fine.

[Al2Klimov]

Why did you drop my unit tests during cherry pick?

[yhabteab]

Why did you drop my unit tests during cherry pick?

What does exactly cover your unit tests? It's somewhat exaggerated just to test error cases that are ultimately triggered by the underlying driver, which does not lie within the responsibility of Icinga DB.

[yhabteab]

I just have rebased this and resolved some conflicts!

[Al2Klimov]

Why did you drop my unit tests during cherry pick?

What does exactly cover your unit tests? It's somewhat exaggerated just to test error cases that are ultimately triggered by the underlying driver, which does not lie within the responsibility of Icinga DB.

It is our responsibility to swallow the correct error.

[julianbrost]

Why did you drop my unit tests during cherry pick?

What does exactly cover your unit tests? It's somewhat exaggerated just to test error cases that are ultimately triggered by the underlying driver, which does not lie within the responsibility of Icinga DB.

It is our responsibility to swallow the correct error.

You're probably referring to this test file from #665: https://github.com/Icinga/icingadb/blob/error-1452-foreign-key-577/pkg/driver/mysql_test.go

To be honest, I haven't looked at it in detail and it's not immediately obvious to me what this actually tests. So please explain them.

The current implementation isn't really ideal for testability. Maybe we could improve that by replacing setGaleraOpts(ctx, conn, wsrepSyncWait) with a more generic setSessionVariableIfExists(ctx, conn, variable, value) function that could then be tested against an actual database server in tests/sql/ with other variables. Like setting a random variable name to test that the error is ignored, setting a variable present on all MySQL/MariaDB versions to an invalid value to test the other error handling, etc.

[julianbrost]

that could then be tested against an actual database server in tests/sql/

When writing this, I missed that this would add dependencies between icingadb/go.mod and icingadb/tests/go.mod that we want to avoid as the integration tests are supposed to test the functionality of a compiled Icinga DB binary and therefore not import its code.

As we plan to move that database code to icinga-go-library anyways, I'd rather take the opportunity there to add proper tests for more of the database code. @yhabteab already started a PR and I wrote a comment where I explained a bit how I imagine this: Icinga/icinga-go-library#10 (review)

The tests proposed in#665 seem to mainly test that one error handling condition is there with quite a bit of complexity in the test cases that seems unnecessary for what it's supposed to test. Restructuring the tests within this project when the corresponding implementation is supposed to be moved to another project soon anyways, feels like unnecessary work, so I'd actually postpone the proper automatic testing of this function until it has reached its new home. Apart from that, the existing integration tests already cover that this PR isn't fundamentally breaking anything and automatically testing that Icinga DB works with a Galera cluster as part of this PR is out of scope in any way.