Notable changes:
* In SqlBagOStuff::getConnectionFromServerInfo, only two loggers were
injected. The rest implicitly got a NullLogger due to being absent.
These are now effectively unsilenced.
* Database::__construct() required almost all parameters, even the
loggers. I've wanted to move some of DatabaseFactory into the ctor
here for a while. In order to make this change not a breaking
change, the new 'logger' parameter is optional with NullLogger as
default. This allowed some of the test cases, which were simply
passing NullLogger, to be fixed by passing nothing instead of
passing the new option name.
The Database class is behind a dozen layers of indirection for
real use, so this will still be injected just fine (DBF, LB, LBF,
MWLBF, etc.).
* In LegacyLogger, the handling for $wgDBerrorLog was previously
limited to DBConnection and DBQuery. This now includes errors
from other (generally, newer) parts of Rdbms as well, which were
previously missing.
This only affects sites (typically CI and dev setup) where
$wgDBerrorLog is used, as opposed to the more common
$wgDebugLogGroups by-channel configuration.
* TransactionProfiler gets its logger injected in a rather odd way,
via entrypoints (MediaWiki.php, ApiMain.php, and MaintenanceRunner)
as opposed to service wiring. This is kept as-is for now.
* In LBFactoryTest, in particular testInvalidSelectDBIndependent2,
there are cases that intentionally produce failures of which
the result is then observed. In CI we assert that dberror.log is
empty so instead of adding the missing logger fields to that
LBFactory instance, the only one set (replLogger) is removed.
The alternative is to set 'logger' now, which would naturally
cause CI failures due to unexpected entries coming through to
non-mocked error log.
Bug: T320873
Change-Id: I7ca996618e41b93f488cb5c4de82000bb36e0dd3
Remove 'insertSelectIsSafe' option, unused.
Remove 'topologicalPrimaryConnRef' option, no longer used as of two
months ago with I41a57247503 (8c9398f7f9).
Remove unneeded DatabaseSqlite::getTopologyBasedServerId
implementation which can inherit null instead of overriding with string
of "0". Only caller is SqlBagOStuff::makeTimestampedModificationToken
which can be used as MainStash DB, where its important that a given
server always has the same unique name within a set of db hosts that
may replicate to each other. By inheriting null as topology server ID,
it SqlBagOStuff will use IDatabase::getServerName instead. That in turn
uses the 'host' connection parameter, which defaults to null in
DatabaseFactory, and then falls back to the string "unknown" which is
as good as "0" for this purpose.
Bug: T299691
Change-Id: Iceb65c28cdd3c4a89b3c8b34c3f95d3285718ec0
Avoid expensive SHOW query for sites that were explicitly configured
to use pt-heartbeat but configured without a row SELECT condition.
Reduce excessive code complexity for configuration edge cases.
Remove getLagDetectionMethod() and just use the field instead.
Change-Id: I0d8592fde65d8a143506c55dccd1972a148bd489
Make DBConnRef enforce the DB domain selected during its lifetime
and allow more nested and successive use of the same connection handle
via DBConnRef. This can avoid extra connections in some cases where
getConnection()/getConnectionRef() is used.
Also:
* Reduce the number of connection pools arrays from six to two
* Merge getLocalConnection()/getForeignConnection() into one method
* Expand various related code comments
Since LoadBalancer::getReadOnlyReason() no longer user the local domain
but rather DOMAIN_ANY, it should not result in "USE" errors if the local
domain does not have a database on the server.
This version of the patch removes the unused reuseConnectionInternal()
method (the method was previously added back to the patch by mistake).
Bug: T226595
Change-Id: I62502f4de4f86a54f25be1699c4d1a1c1baee60b
Make DBConnRef enforce the DB domain selected during its lifetime
and allow more nested and successive use of the same connection handle
via DBConnRef. This can avoid extra connections in some cases where
getConnection()/getConnectionRef() is used.
Also:
* Reduce the number of connection pools arrays from six to two
* Merge getLocalConnection()/getForeignConnection() into one method
* Expand various related code comments
Bug: T226595
Depends-On: If808cbab429d41e1f2289683533e4a781a4bdf5e
Change-Id: I540b08920997c57cad6445ddb09d8e663eaf4714
Remove it from the interface as well. The interface is marked stable
to implement for (theoretical) database extensions.
This is not a breaking change as any existing implementations of the
method may continue to exist, and continue to be uncalled by core.
The interface is not stable to type or call, and the built-in
implementation is internal/unexposed.
Change-Id: Iced3ab3dfca70dec2958eb9d750e7025b95b7de3
Also hoist up the `@covers` annotation for the test class overall,
and widen some of the more specific ones to make up for the fact
that this code has recently been refactored and thus is now part
of different classes.
Change-Id: I67d19502fd191042c902d8b48d8cbd3ff76473d6
Previously, invalid SQL would be sent to the database server,
resulting in a SQL syntax error. Log a warning instead.
Move USE/IGNORE INDEX handling out of makeSelectOptions().
Inject the Database error logger callback into SqlPlatform.
Bug: T315195
Change-Id: Ie33faddcd766926f083e4421c6d0508a16b58f71
The default mode (using server_ids) will now use SHOW REPLICA STATUS
data instead of connected to the master. If the source server is not
the root source server (e.g. the primary), then lagDetectionOptions
should be configured to either specify the primary server ID or use
custom channel columns (like WMF sites).
Remove getMasterServerInfo() and getPrimaryServerInfo() method.
Clean up some related comments and field names.
Bug: T299691
Change-Id: I41a57247503a69367cd73d365f8aa8af04398e46
We also bring along Database::attributesFromType(), which relied on the
private ::getClass(). This requires us to inject DatabaseFactory through
the LBFactory/LoadBalancer hierarchy.
Database::factory() is now soft deprecated. All callers outside of
includes/installer/ still need migration.
Bug: T299691
Bug: T315270
Change-Id: I7d057a9438f1b097554679975e4e9b2fc99e7c2b
This just makes the class overly complex and defies the whole point of
having this class in the first place (=delaying getting db connections).
It makes it harder to read and understand and specially polymorphic
arguments are quite confusing.
It also would break the reloading db on the fly (I6c3ffde62f6e), users
of DBConnectionRef should not know about the underlying connection, this
class is trying encpsulate that and accepting connection in constructor
breaks this encapsulation.
Bug: T298485
Depends-On: I951ab99ae766788dbae8bd338aaae114a388b21f
Change-Id: I743c0565504c65c4dedf29f2a36d4c386601fda0
Calling ->onlyMethods( [] ) with an empty array does have an effect.
By default, all methods are mocked, which means the original code is
not called. Calling ->onlyMethods( [] ) turns this around. No methods
are mocked but all call the original code.
This is almost the same as ->enableProxyingToOriginalMethods(). The
difference is that ->enableProxyingToOriginalMethods() also requires
the original constructor to be called, but ->onlyMethods( [] ) does
not.
We can get rid of this confusing setup in tests that don't need it.
All tests in this patch that succeed with a simple ->createMock()
just demonstrated that they don't need it.
Change-Id: I341323a1ca793c039498f80b7f073c124b6b6ae0
Now that we moved considerable chunk of DatabaseSqlite to
SqlitePlatform, it's easier to mock and test without actually needing a
live db connection.
This allows moving a lot of tests from integration to unit tests making
them much faster.
Change-Id: I8ce7c470abed5144ba456a000c733e5d739c5c4e
This introduces $wgLBFactoryConf['configCallback'] which can be set to a
function that returns updates to be applied to $wgLBFactoryConf. The new
method LBFactory::autoreConfigure() can be called to check the callabck
and, if the config changed, reconfigure all existing LoadBalancers.
Reconfiguring the LoadBalancers causes all open connections to be
invalidated; however, any DBConnRef instances will remain valid and will
acquire a fresh connection from the LoadBalancer automatically when
appropriate.
As a proof of concept, this patch adds support for config reloding
into WikiExporter.
Bug: T298485
Change-Id: I6c3ffde62f6e038730736abe980befd90ec43e1a
We really shouldn't do regex matching on SQL we produced, we are better
then that but until we refactor the code to a better shape, it can move
to the platform.
Bug: T307616
Change-Id: Ie2c84f208dfbed036b14c0a0669e004cc84e0f4b
Less setup code, easier to read.
I also found something that looks like it should have been an
assertInstanceOf(), so I made it one.
Change-Id: I7ba9b580c270a103feb0f55b910157f93197fa43
createMock() does the same, but is much easier to read.
A small difference is that some of the replacements made in this
patch didn't use disableOriginalConstructor() before. In case this
was relevant we should see the respective test fail. If not we can
save some CPU cycles and skip these constructors.
Change-Id: Ib98fb06e0fe753b7a53cb087a47e1159515a8ad5
Rewrite the queries so that the column values of conflicted
proposed rows can be referenced in the SET expressions used to
update existing rows. Add IDatabase::buildExcludedValue() to
derive such references.
Make upsert() only use the first key in the unique key list as
the one to use for finding existing rows and merging them. Any
other key will just cause duplicate key errors/rollback upon
row collisions. For example, suppose a `user` table has unique
keys on both UUID and email address. It would be undesirable
for upsert() to clobber an existing `user` row just because
someone tried to make an account with the same email address.
Using upsert() makes sense when the following hold:
- The table has a single unique key that conveys "identity".
AUTOINCREMENT and UUID columns are particularly good here.
If row A existed at time t1, row B existed at time t2, and
both rows have matching values for this "identity key", then
they are conceptually the "same" row. It does not matter if
other columns differ or if the row was deleted and recreated.
- Any other unique keys are just business constraints unrelated
to the concept of identity. For example, an `accounts` table
might have ID, name, and email address each as unique keys.
- For each proposed row, upsert() looks for an existing row
that conflicts along the identity key. If there is one, then
it gets updated, otherwise, the proposed row is inserted.
Other unique key collisions result in operation rollback.
Bug: T113916
Change-Id: Iddd9f120ee966c76b3acb35e62ea14ec4c6f925d
Add Database::queryMulti(), which will execute an array of
queries as a batch with minimal roundtrips.
SQLite fallbacks to looping through each statement and
invoking doQuery().
Add QueryStatus class to reduce complexity in Database.
Rewrite doQuery() as doSingleStatementQuery().
Change-Id: I3d51083e36ab06fcc1d94558e51b38e106f71bb9
This moves a lot of methods used in selectSQLText method so the move
would be easier. I tried moving it but the patch basically exploded.
Bug: T307616
Change-Id: Ib8c68b4506055b63c769926d6d7cfd1069d951f4
This would simplify the code for its users a lot.
Bug: T255493
Depends-On: I6ab4440a13f4682dd3a6e9cfc6c629222fbeab8a
Change-Id: I6e7544763bde56fc1e19de0358b71ba984a978ca
The first of many changes decoupling SQL building blocks from Database
class.
Bug: T299691
Depends-On: I5d1d5b9b875bced7bda234f45d6d22ed59db4871
Change-Id: I784e78361f5ee629d31c68629d669ee0ddddf929
Instance ownership is supposed to protect LoadBalancer and Database
against unauthorized calls to internal methods other than by the owning
LoadBalancer/LBFactory. This seems like unnecessary complexity. It was
introduced for T231443 and T217819, but the link was speculative and in
the end it didn't help to fix or isolate those bugs. Since then it has
caused a production error (T303885) and an intermittent CI failure
(T292239).
Instead, split the ILoadBalancer interface, introducing
ILoadBalancerForOwner, which contains the methods which are only safe
to call by LBFactory or by the caller of LBFactory::newMainLB(). This
allows phan to statically detect inappropriate calls to internal
methods.
Ownership was used for convenience for two things unrelated to its
original purpose:
* Suppressing calls to ScopedCallback::newScopedIgnoreUserAbort() when
the caller has already called it. But nested calls are apparently
harmless, so I just called it unconditionally.
* Suppressing exceptions from Database::close(). I extended the
behaviour for owned instances to apply to all instances, so even
unowned instances will no longer throw on close.
CodeSearch suggests nothing in extensions is calling these methods with
an owner parameter. One extension (Wikibase) overrides a method with an
owner parameter in a test mock class and so needs to be simultaneously
updated.
Depends-On: Ib03aba9d8f5f05b875a321d00b14483633a636a8
Change-Id: I27ba4973d24d759c88b3868c95e7db875801ca0c
This is the first step to split parts of Database that doesn't require a
connection and are used for query parts.
Bug: T299691
Change-Id: I140aa4328865994499926f898233867ce383908c
Rename canRecoverFromDisconnect() in order to better describe
its function. Make it use the transaction ID and query walltime
as arguments and return an ERR_* class constant instead of a bool.
Avoid retries of slow queries that yield lost connection errors.
Track session state errors caused by the loss of named locks or
temp tables (e.g. during connection loss). Such errors will prevent
further queries except for rollback() and flushSession(), which must
be issued to resolve the error.
Add flushPrimarySessions() methods to LBFactory/LoadBalancer
and use it in places where session state loss is meant to be
safely aknowledged.
Change-Id: I60532f86e629c83b357d4832d1963eca17752944
Pass the error number as an argument, similar to isConnectionError()
Fix related mysql documentation links
Change-Id: Id32ef2fd27de65376960de3f5138ffdf7654ff71
