Making them sitting next to each other would make the class more
readable and also discouraging direct use of these methods outside of
rdbms (also implicitly by putting them at the bottom of the class).
These methods should not be stable to override anymore, their
counter-part in platform should be (for extensions planning to add
support for more rdbms types)
Bug: T307616
Change-Id: I5d6c5775f2a7378a73c69618ed09acb1b31374a8
This is mostly about adding return types to methods that implement PHP
interfaces, and not passing null to core functions that want a string.
After this patch, and an update to return types in RemexHtml,
tests/phpunit/integration/ has no more errors than in PHP 8.0.
Bug: T289879
Bug: T289926
Change-Id: Ia424f5cc897070f4188ae126b5bf6a1f552db0e1
* Move queryMulti() to IDatabase
* Provide default parameters
* Unpack complex expression in do/while condition
* Fix some comments
* In buildExcludedValue, remove rambling about bad alternatives.
Change-Id: I675fcbd397492e7efdb023debc215c30e21c7ffe
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
* It's now only logged when we actually read or write to CP.
This makes it less confusing to see the message in cases where
CP isn't or can't be used, as it would claim to use it when really
it isn't/wouldn't.
* This was adding some overhead as it was the only non-wfDebug()
thing I could find on all requests. Mainly noticable locally with
mw-docker and writing to mw-www-debug.log. This helps a bit
with T85805 and making dozens of load.php requests in debug mode
less slow.
* It duplicated the channel name outside its class instead of
using DI.
Change-Id: I0947b98d285f26ab5af8dd58a6dce2e3e0525d44
If there are row or named locks held, ping() should be called to
make sure that the locks where not lost. If they were lost, then
no transactions should be committed.
Slightly optimize isWriteQuery() by checking QUERY_CHANGE_TRX.
Bug: T307133
Change-Id: I8f97e1e3a4aa72c31ac2d642511af8dadf070b07
Fix and simplify the flags to executeQuery(). The QUERY_CHANGE_ROWS flag
caused the DB_REPLICA role check to be violated.
Remove the UNLOCK TABLES query since lockTables()/unlockTables() have
since been removed.
Bug: T306632
Change-Id: I8fcf940028abd70c0d66bec1780308d3c2367b61
Using a bitfield breaks server array template merging in LBFactoryMulti.
The only nice thing about flags compared to boolean constructor
parameters is the existence of setFlag(), clearFlag() and
restoreFlags(). But that is not useful for parameters that are used
during connect.
Bug: T134809
Change-Id: I088ef4a0e4c11a2cfe17415b325b455164004c1e
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 error codes are strings (e.g. "55P03"). Casting to an integer
breaks comparisons by callers (all internal) and makes error strings
confusing.
Use "00000" instead of 0 to reduce this confusion (does not affect
current callers).
Follow-up to 478c4892.
Change-Id: I3dd8929f4c8e604eef669710e684724977381c74
This is to allow us aggregate errors coming from different dbs.
Simply add the dbname as an attribute
Bug: T306634
Change-Id: I7c4609ececbf727336c174616d525d2df9637559
The first of many changes decoupling SQL building blocks from Database
class.
Bug: T299691
Depends-On: I5d1d5b9b875bced7bda234f45d6d22ed59db4871
Change-Id: I784e78361f5ee629d31c68629d669ee0ddddf929
As required by the Iterator interface. In PHP 8.1 we can suppress the
warning with #[\ReturnTypeWillChange], but it's wrong and should change
anyway.
Depends-On: I576109808755b054e7876556f244ee8eafd12f9d
Change-Id: Ibaa7b4c81d8f114783db45c9100200ba14547c8f
Callback style iteration made sense before generators existed, but
generators make for simpler code. The "call method" variants made
sense before closures existed but defeat static analysis.
So, in LBFactory:
* Add ILBFactory::getAllLBs()
* Deprecate ILBFactory::forEachLB()
* Remove LBFactory::forEachLBCallMethod(), was protected.
* Add LBFactory::getLBsForOwner(), which is protected and has the
internal interface in @return. Adding a new abstract method breaks
Wikibase tests despite LBFactory not being stable to extend.
* Migrate callers. Generators allow you to return/break from the middle
of the loop, which implies a little rearrangement for some callers.
In LoadBalancer, connections supposedly of type IDatabase were
routinely type-hinted as Database in closure parameters so that methods
could be called that were not in the interface. So it's convenient to
get rid of public iteration methods entirely in favour of private
methods returning Database[].
* Hard-deprecate ILoadBalancerForOwner::forEachOpenConnection()
and replace it with a private generator method since nothing called
it externally except for core tests.
* Hard-deprecate ILoadBalancerForOwner::forEachOpenPrimaryConnection()
and replace it with a private generator. DeferredUpdates needed it for
iterating over IDatabase::explicitTrxActive(), so add
ILoadBalancer::explicitTrxActive() as a replacement.
* Replace private method LoadBalancer::forEachOpenReplicaConnection()
with a generator.
Depends-On: If0b382231e27d6d1197fb7b6aef6ab50335df4e5
Change-Id: I64514e77b9bfe737be5b12e1d3c9c49976bb522f
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
