Make getServer() always return a string, as documented, even with new
Database::NEW_UNCONNECTED handles that have yet to call open(). If the
'host' parameter to __construct() is ''/null, getServer() now returns
'localhost' instead of null. This avoids problems like fatal errors in
calls to TransactionProfiler::recordConnection().
Use Database constants for "connectionParams" field keys for better
static analysis.
Also:
* Add Database::getServerName() method that returns "readable" server
names in the style of LoadBalancer::getServerName(). Note that the
"hostName" field is already passed in from LoadBalancer.
* Migrate most getServer() callers to getServerName() for easier
debugging and more readable logging.
* Also, normalize Database/LoadBalancer SPI logging context to use
"db_server" and reduce logging code duplication in LoadBalancer.
Bug: T277056
Change-Id: I00ed4049ebb45edab1ea07561c47e226a423ea3b
Also:
* Add LBFactoryMulti sanity check to avoid section/cluster collisions.
Since sections are just a type of cluster, using the same name can
cause confusion. WMF config already treats them as overall unique.
* Rename 'externalLoads' field in LoadBalancer for clarity.
Change-Id: I748db691b4918b797b82d75cfd8722453ccd9d7f
The name change happened some time ago, and I think its
about time to start using the name name!
(Done with a find and replace)
My personal motivation for doing this is that I have started
trying out vscode as an IDE for mediawiki development, and
right now it doesn't appear to handle php aliases very well
or at all.
Change-Id: I412235d91ae26e4c1c6a62e0dbb7e7cf3c5ed4a6
Done with `composer fix` and suppressing the rest (i.e. sniffs for
global variables, which for core should be suppressed anyway).
Additionally, add `-p` to `phpcbf`, as otherwise it just seems stuck.
Change-Id: Ide8d6cdd083655891b6d654e78440fbda81ab2bc
This allows full configuration of the LoadMonitor, not just setting
the class name like loadMonitorClass allows. Deprecate the later.
Also fix obsolete LoadBalancer constructor documentation and
standardize the style of LBFactoryMulti::__construct() comments.
Change-Id: Icfdc0d6bbf227dce56e65ad679b93ce3f604e9f7
Add new ILBFactory::setDomainAliases() method for injection database
domain aliases and call it in MWLBFactory::setDomainAliases().
Also:
* Remove overkill "last db/section" caching in LBFactoryMulti
* Clean up some LBFactoryMulti code comments
* Split out separate MWLBFactoryTest test file
Change-Id: If180a58c61178969ca7587c4a06b8786574c7254
*assertType is marked as deprecated, and should ideally be removed soon
(i.e. no hard deprecation to follow)
*Most usages of assertType in core were autofixed by using I8ef556b630812aeea77c5606713f53d9af609f1b
*assertTypeOrValue was removed because only used in SiteTest
(codesearch: https://codesearch.wmflabs.org/search/?q=assertTypeOrValue&i=nope&files=&repos=)
*SiteTest::assertTypeOrFalse was removed because unused
Bug: T192167
Change-Id: Icb3014b8fe7d1c43e64a37e0bdaaffec18bb482f
Move the DBO_TRX init logic out of Database::__construct() and into
LoadBalancer since the later already handles setting and clearing this
flag based on transaction rounds starting and ending.
Add 'lazyMasterHandle', 'topologyRole', and 'topologicalMaster' parameters
to Database::factory() and inject them via LoadBalancer all at once in order
to avoid worrying about call order. Move some type casting code to
Database::__construct().
Add IDatabase::getTopologyRole()/getTopologicalMaster().
Use constants for getLBInfo()/setLBInfo() for better usage tracking and
typo resistance.
Change-Id: I437ce434326601e6ba36d9aedc55db396dfe4452
Add public, protected or private to function missing a visibility
Enable the tests folder for the phpcs sniff
Change-Id: Ibefce76ea9984c47e08c94889ea2eafca7565e2c
assertEquals( null, … ) still succeeds when the actual value is 0, false,
an empty string, even an empty array. All these should be reported as a
failure, I would argue.
Note this patch previously also touched assertSame( null ). I reverted
these. The only benefit would have been consistency within this codebase,
but there is no strict reason to prefer one over the other. assertNull()
and assertSame( null ) are functionally identical.
Change-Id: I92102e833a8bc6af90b9516826abf111e2b79aac
If the specified default group does not have a corresponding server load map
defined, then ignore it and use GROUP_GENERIC.
Also, consolidate the group load and group reader index code for simplicity
Change-Id: Ic8bf9a3ebcbffb81fb14d7b1787a2adb97ac525d
This does what ChronologyProtector wants more rigorously and is better
named. Not all replica servers will have the same position, so they
should be compared to get the highest one.
Simplify the getMasterPos() method to only return master positions
as the other current callers do not need anything else. It will now
connect if needed as well. This should make the method naming better.
Reducing the use of replica derived replication postitions (instead
of those from the master) makes certain GTID issues less likely,
such as the matter of obsolete domain IDs.
Increase general test coverage of LoadBalancer.
Bug: T224422
Change-Id: I5420721ee339a24d09c26c38709500c7bbe797c2
Add and use IDatabase::getServerConnection() method to avoid loops caused
caused by pickReaderIndex() calling getConnection() for the master server.
That lead to getReadOnlyReason() triggering pickReaderIndex() again.
Make getLaggedReplicaMode() apply when the master has non-zero load and
the replicas are all lagged.
Remove "allReplicasDownMode" in favor of checking getExistingReaderIndex()
instead. This reduces the amount of state to keep track of a bit.
Follow-up to 95e2c99094
Bug: T226678
Bug: T226770
Change-Id: Id932c3fcc00625e3960f76d054d38d9679d25ecc
Split out private resolveGroups() method for normalizing query
groups. Move some server index validation to getConnectionIndex()
and make it stricter.
Make getConnectionIndex() group fallback logic aware of the generic
group. The main use case for custom default groups is heavy scripts
or jobs that run in the background. It is probably better to have
good DB server redundancy for the query group and rely on it rather
than possibly fallback onto all of the main servers used for normal
requests. The later behavior could spread slowness or outages.
Also make getAnyOpenConnection() more robust and readable by
splitting out a private pickAnyOpenConnection() method that
checks IDatabase::trxLevel().
In addition, remove redundant is_int() check from isOpen()
method as it will return false in that case even without it.
Remove bogus getConnection() parameter in testCopyTestData().
Change-Id: Ica619c5487c761c724791d151db7388e4b41b0aa
This is slightly more robust and makes the intent much clearer
than random calling code checking getServerCount() all over the
place. In addition, this yields better separation of concern.
Also, cleanup the LoadBalancer constructer a bit and make the
validation a bit stricter.
Make some server index comparisons strict while at it.
Change-Id: Icc1a35bd65c6862ff81faa3ab9b2aa7cafe29443
Add a "readIndexByGroup" field that does the same thing that the
generic read index does except for query grouped connections.
Also remove some pointless checks at the end of getReaderIndex().
Change-Id: I3bd6295be4355ee930960f89ccb07811dd8c5545
Use the method in getConnection()/openConnection() and make it
handle the local domain alias as well.
Also:
* Move some code to a new getConnectionIndex() method.
* Make the text of various index/domain exceptions more consistent.
* Improve some auto-commit flag code comments.
Change-Id: I7e0d4f2134ee91ad60b0d34bf01e05115193b04a
Even if there is only one database, if a DB_REPLICA connection
reference is obtained and an attempt is made to write it then
it will still fail. This can make it easier to spot incorrect
database usage even in a simple vanilla developer environment.
Note that methods like ILoadBalancer::getConnectionRef() can
be used for local connections as well as foreign ones.
Change-Id: I5523daad1bdd64624d3e0dd41bfd8d078b1078b0
Sqlite used the base implementation of trying a SELECT 1 query and
seeing if it failed. Instead, make it use the sqlite_master table.
Also remove the base version of that method since it would always
cause this problem and all subclasses have proper implementations.
Make LoadBalancerTest::assertWriteAllowed() more explicit and add
more assertions there.
Change-Id: I6c7b0bea8894c45dfe8931748d6687f0e5d1e101
* Handle the case where an onTransaction* callback for one handle
adds more onTransaction* callbacks to a different handle. Instead
of supporting only a short chain of such callbacks, try to resolve
the whole chain by using a loop in LoadBalancer and LBFactory.
* Add sanity checks to enforce the proper call order of LoadBalancer
transaction methods, such as those that execute callbacks. This is
the order that LBFactory already uses. Use ROUND_ERROR for problems
that can ruin the instance state. Such problems require rollback.
* Correct setTrxEndCallbackSuppression() calls in beginMasterChanges()
that were making tests fail.
* Make Database handle callback suppression for FLUSHING_ALL_PEERS
instead of making LoadBalancer/LBFactory have to manage it.
* Simplify finalizeMasterChanges() given that suppression does not
actually effect runOnTransactionPreCommitCallbacks().
* Make dangling callback warning in Database::close work properly.
* Actually use $fname in flushReplicaSnapshots().
* Use DBTransactionError instead of DBExpectedError in some places
where stages fail.
* Fix failing testGetScopedLock() unit tests so everything passes.
Add more comments to setTransactionListener and onTransactionIdle.
Change-Id: I6a25a6e4e5ba666e0da065a24846cbab7e786c7b
Once getMain() was called in setSchemaAliases(), the ChronologyProtector
was initialized and the setRequestInfo() call in Setup.php had no effect.
Only the request values read in LBFactory::__construct() were used, which
reflect $_GET but not cookie values.
Use the $wgDBtype variable to avoid this and add an exception when that
sort of thing happens.
Further defer instantiation of ChronologyProtector so that methods like
ILBFactory::getMainLB() do not trigger construction.
Bug: T192611
Change-Id: I735d3ade5cd12a5d609f4dae19ac88fec4b18b51
We need to make sure a DBO_TRX transaction was started before doing the
CREATE TABLE, because CREATE TABLE itself won't start one and sqlite
breaks if schema changes are done on one handle while another is open.
Also, incidentally, have the handles in these LoadBalancerTests log to
the standard channel. And clean up the auto-rollback of DBO_TRX
transactions to use ->rollback() instead of ->doRollback() plus
incorrect manual setting of trxStatus.
Bug: T191863
Change-Id: Ib422ef89e7eba21281e6ea98def9f98ae762b9fe
Various revision storage tests assume the sequences are reset
and fail otherwise.
Also disable some failing tests that are not applicable to sqlite
as well as postgres.
Change-Id: Ibdb034121a44e16bb35059a92baafb1867951ea8
The "AUTO" means AUTOCOMMIT, not "automatic transactions"/DBO_TRX,
which is basically the opposite concept. The new name does not
suffer from that ambiguity.
Keep the old constant as an alias for backwards compatibility.
Also remove LoadBalancer comment about non-existing field
Change-Id: I63beeb061fc9be73f320308e4d6393b58628b8c8
This returns the default domain without getting a connection
Also avoid assuming that LoadBalancerTest has the same DB handle
as other tests even though it makes its own LB object. That breaks
if temporary tables are used.
Change-Id: I351d42de38b3126222c5a40627a2a12f76a60939
