Scalar casts are still allowed (for now), because there's a huge amount
of false positives. Ditto for invalid array offsets.
Thoughts about the rest: luckily, many false positives with array offsets
have gone. Moreover, since *Internal issues are suppressed in the base
config, we can remove inline suppressions.
Unfortunately, there are a couple of new issues about array additions
with only false positives, because apparently they don't take
branches into account.
Change-Id: I5a3913c6e762f77bfdae55051a395fae95d1f841
Until I70473280, integer literals were always quoted as strings, because
the databases we support all have no problem with casting
string-literals for comparisons and such.
But it turned out that gave MySQL/MariaDB's planner problems in some
queries, so we changed it to not quote actual PHP integers.
But then we run into the fact that PHP associative arrays don't preserve
the types of keys, it converts integer-like strings into actual
integers. And when those are passed to the DB unquoted for comparison
with a string-typed column, MySQL/MariaDB's planner has problems again
while PostgreSQL simply throws an error. Sigh.
This patch adjusts Database::makeWhereFrom2d to assume that the $subKey
column is going to need all values quoted, as is the case for all
callers in MediaWiki.
Bug: T239877
Change-Id: I69c125e8ab9e4d463eab35c6833aabdc436d7674
Code cleanup and hardening (see also: T234014) of Database-related
lib code in MediaWiki core.
Bug: T233342
Change-Id: I3c968f4f5300374253dc80d99596cac50fbeb59e
Quotes started being added to integers in r4984 (August 2004). Before
that, is_numeric() was used to determine whether to add quotes, so
quotes were omitted from numeric strings, which is obviously wrong.
The idea here is to use the type of the variable to hint to the database
as to whether quotes are needed. The results are somewhat inconsistent,
since some callers do not convert numeric strings obtained from user
input to integers. That makes it a more conservative change. Callers can
opt out of unquoted integers by casting them to string.
The reason for doing this is that quoting integers turns out to be not
as harmless as originally assumed. We found a case of it confusing the
MariaDB query planner, causing inappropriate indexes to be used.
I also made addQuotes() consistently return a string, instead of
returning an integer for boolean values. This was already the case for
MySQL, but it seems like a good idea everywhere.
Bug: T238378
Change-Id: I70473280f542ee5ecd79e187f580807410fbd548
This is to ensure that the CI job is working with the new version.
Note: redundant_condition_detection should have worked as expected by
this version, but unfortunately it still has false positives.
Bug: T235049
Bug: T231636
Change-Id: Idaba6584cb5b2ff19b6455c7bbec6b89619ddbff
This broke after e0cc49ce39, due to the field 'master'
being removed from the log context. The LegacyLogger logic
forwarding these messages to MWDebug (for the debug toolbar)
however, was dependant on.
Users of debug toolbar experienced a silent failure because the
logic in question is very tolerant of missing fields. This is
because it uses those fields to distinguish the 'sql' messages
from channel=DBQuery from other messages in the same channel.
Making that less fragile is outside the scope of this commit.
This commit:
* Restore the basic functionality by making sure MWDebug::query()
gets called again for DBQuery messages.
* Remove the code relating to the 'master' field as this no longer
exists in RDBMS. It also wasn't used anywhere (to be used,
it would need to be read by mediawiki.debug/debug.js).
* Remove unexpanded "{method}" and "{runtime}" noise in the debug
toolbar text. This was introduced by he conversion to PSR-3
logging.. These fields are already rendered separately by
the toolbar and should not be part of the "SQL" column.
To do this, we need to log the $sql bit as its own key, so
I've made this a context field as well.
* Reduce the condition logic in LegacyLogger to only looking for
'DBQuery' and 'sql'. This way, if it breaks again it will
still call the logic within and emit E_NOTICE instead, which
would help detect the issue (and still fallback to at least
showing the queries). Unlike before this commit where it took
quite some time to figure out why it wasn't working.
* The above fixes still weren't enough to get queries to show
up in the Debug toolbar for me. Turns out, this was because
my local setup (mediawiki-docker-dev) uses a master-replica
set up. The setup doesn't use any custom LBFactory config,
just plain $wgDBservers. The logic for turning these plain
settings into LBFactory (in MWLBFactory.php) does kick in,
and does run (unlike if I had custom wgLBFactoryConf).
But, the DBO_DEBUG flag didn't make it through because of
the += operator preferring any pre-existing value my setup
has, which is just `DBO_DEFAULT`.
Merging 'flags' keys seems unsafe in general, but adding
DBO_DEBUG based on $wgDebugDumpSql seems innocent and doesn't
affect other behaviour (it's a case of DWIM).
Bug: T231742
Change-Id: I122bb1a65620a7ae4e1943136c975b63524a5111
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
This reverts commit 4bd1b4b455.
Reason for revert: Incorrect syntax for SQLite "ON CONFLICT (...) DO UPDATE".
Bug: T234795
Change-Id: I987e6e7052368d5d9d0b6de9dd189edc4feaebe5
Logging this exception will make a duplicate entry in logs
cause 'query' will call 'reportQueryError' later on that will
log the same error.
Bug: T233756
Change-Id: Ic44f81761f24f6c5ee7b4d8d0fc51ba8661eaa8c
Enforce this pattern for the remaining LoadBalancer methods.
Carry this over into Database::close() to decide how loud the
error handling should be.
In LBFactory, clean up ownership of newMainLB()/newExternalLB().
The should have a null owner if called from outside the class
since the LBFactory does not track nor care about them anymore
in that case. Disable newMainLB() for LBFactorySingle as it
makes no sense and was broken.
Also remove some redundant abstract LBFactory methods that
just duplciate ILBFactory.
Bug: T231443
Bug: T217819
Depends-On: I7ed5c799320430c196a9a8e81af901999e2de7d0
Change-Id: I90b30a79754cfcc290277d302052e60df4fbc649
Defer the queries until a connection exists. Only issue issue the
them for databases that are different than the currently opened file.
Also, make handleSessionLossPreconnect() aware of attached databases.
In LoadBalancer::reallyOpenConnection(), avoid having the "catch" block
appear like it returns a half-constructed Database.
Change-Id: I9f676bb72a1ab06f0eac5820dce28231741c283d
All of these suppression prevent the detection of many common mistakes,
and could easily prevent things like T231488. Especially if there are
few issues of a given type, it's way better to suppress them inline,
instead of disabling them for the whole core.
This patch only touches the one with a lower count (although those
counts may be out of date).
Bug: T231636
Change-Id: Ica50297ec7c71a81ba2204f9763499da925067bd
Make LoadBalancer::reallyOpenConnection() handle initializing DBO_TRX
instead of Database::__construct().
Also:
* Avoid having the "catch" block appear like it returns a
half-constructed Database.
* Use the variable name $conn instead of $db to be consistent
throughout the class. Only send Database::__construct() parameters
that it recognizes instead of mixing in setLBInfo() data.
Change-Id: Iffc3d1d0713051a164adb51a4c4ee12e4ac887c3
Rename Database::hasFlags() to sound less similar to getFlag()/setFlag().
The order of class fields is roughly:
* Objects and resources
* Configuration
* Mutable options that do not have to be kept in sync with the connection
* Session level state tracking (should be in sync with the connection)
* Transaction level state tracking (should be in sync with the connection)
* Results from or timestamp of the last time a certain event occurred
Change-Id: Ibdca2fefe7ed2c792344c5602b5191a950eed933
Callers should use query batching instead. Without buffering, either
the RAM burden is shifted onto the RDBMS server (with MVCC vacuuming
also being stalled) or the RDBMS server has to keep adding locks as
the cursor advances. Also, if a caller does not read all the results
(possibly due to an exception), then the SQL commands sent/read get
out of sync, which is too fragile.
There are no DBO_NOBUFFER callers in WMF gerrit repos.
Change-Id: I3712149633d0f01bb6990e324e53dd58abba9cfd
After approval of RFC T191231, we are going to drop oracle and mssql
and it will be possible to bring back the support using the abstract schema
Adding to release notes will be done in a follow-up
Bug: T230418
Change-Id: I90bd5cfcc3e18011b193c965fdb1fa54675040b5
Define missing DatabaseSqlite::doSelectDomain() method to handle attempts
to change the database, prefix, and/or schema.
Also add sanity check to serverIsReadOnly() to make sure open() was called
Change-Id: I72c25bf4dab5e01def3fb9472217e7637aede1d4
So that it is possible to distinguish master queries from replica
queries. Useful when debugging issues such as T221763.
Change-Id: I38896efa46f2c7cf289e5bdc543a1692e251ec1d
Mainly:
* Use oci_new_connect() for Oracle to avoid broken connection reuse
similar to the PGSQL_CONNECT_FORCE_NEW flag in DatabasePostgres
* Set 'client_min_messages' unconditionally for PostgreSQL
* Factor out Database::getConnectExceptionAndLog() helper method
* Use the same style of query() calls in DatabaseOracle::open() as
the other subclasses
* Make sure the Database driver handle field is null on failure
instead of false for sanity
Also:
* Disallow changing of Database handle DBO_* flags after construction
where it does not make sense to change them
* Do not mention DBO_* flags meant for non-config use in $wgDBservers
* Ignore DBO_PERSISTENT for SQLite if DBO_TRX is also set for sanity
* Remove $wgDBOracleDRCP variable to discourage careless automatic
setting of DBO_PERSISTENT that breaks LoadBalancer assumptions
Change-Id: Iea948f7f872294ea8fc5d897fc10c9d29b7141d5
Since dbSchema() always casts the result to a string, using this method
with a call to open() is broken if the RDBMs does not support DB schemas
and thus requires null
Follows-up 7911da9c6f (last week), which added the check in
DatabaseMysqlBase::open() check. Also follows fe0af6cad (last year),
which made dbSchema() consistently return string. Before that, an
implicit null was passed in from Database::factory for mysql, which hid
the class default of empty string.
Bug: T227708
Change-Id: I67207fbaa39c5cc3fe062077cc654f048090e009
Make flushSnapshot() logging more detailed and check for explicit transaction
rounds. Also removing periods and trailing newlines from log/exception messages.
Change-Id: I0f6520f563680ab3a65b6338ced59ba25a2ec7b5
