tgr@deployment-tin:~$ mwscript sql.php --wiki=enwiki ...vote_ip in table securepoll_votes already modified by patch /srv/mediawiki-staging/php-master/extensions/SecurePoll/patches/patch-vote_ip-extend.sql. ...echo_subscription doesn't exist. >
That sounds like sql.php somehow triggered the update process, which really shouldn't be happening.
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | daniel | T157651 sql.php must not run LoadExtensionSchemaUpdates | |||
| Duplicate | daniel | T249599 DatabaseUpdater should not call hooks in the constructor | |||
| Resolved | daniel | T249603 DatabaseUpdater: protect methods for direct database modification | |||
| Resolved | Addshore | T249598 Wikibase schema updaters must not modify database directly | |||
| Resolved | daniel | T249584 Call LoadExtensionSchemaUpdates later | |||
| Resolved | Ladsgroup | T249683 Redefine mysql GRANTs for wikiadmin |
So, getting this test merged depends on redoing the wikibase schema hook application order for update.php. In CI, there seems to be a problem when it interacts with Flow hooks trying to make pages.
@aaron are you requesting code review from Core Platform or do you need something else?
Long term, this sort of thing should wind up being fixed by T191231: RFC: Abstract schemas and schema changes.
Short term, Wikibase changes are unlikely to be in scope for CPT. Looked at the core structure test patch.
Change 586473 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] maintenance: Remove sql.php temporarily
Change 586474 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@wmf/1.35.0-wmf.26] maintenance: Remove sql.php temporarily
Change 586473 merged by jenkins-bot:
[mediawiki/core@master] maintenance: Remove sql.php temporarily
This bug brought down everything for twenty minutes and caused an important table to vanish in production today.
Change 586474 merged by jenkins-bot:
[mediawiki/core@wmf/1.35.0-wmf.26] maintenance: Remove sql.php temporarily
Mentioned in SAL (#wikimedia-operations) [2020-04-07T01:06:52Z] <jforrester@deploy1001> Synchronized php-1.35.0-wmf.26/autoload.php: T157651 Remove sql.php from autoloader (duration: 00m 58s)
Mentioned in SAL (#wikimedia-operations) [2020-04-07T01:08:26Z] <jforrester@deploy1001> Synchronized php-1.35.0-wmf.26/maintenance/: T157651 Remove sql.php from maintenance/ (duration: 00m 58s)
Change 587010 had a related patch set uploaded (by Jcrespo; owner: Jcrespo):
[operations/puppet@production] restore: Add s8 instance to db1095
Change 587010 merged by Jcrespo:
[operations/puppet@production] restore: Add s8 instance to db1095
Flow was mentioned in the comments as a blocker to this work. I rebased the structure test patch to see if the error still comes up.
Looking at the code, there seems to be a whole set of dubious things going on which all together caused the disaster above:
Changing any of the above might have prevented the problem. I think most if not all of these things should be fixed.
I see violations from Wikibase:
This is technically a wikibase bug, caused by the hook handler doing direct schema modifications, rather than registering them. But there isn't any documentation that would have made it clear that this was a bad idea (tm)...
Change 587213 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] DatabaseUpdate: warn extensions about direct modification
I think this task will explode. What do you think of creating a subtask for Wikibase?
T249599: DatabaseUpdater should not call hooks in the constructor should address the original issue in this task's title.
I believe the main purpose of sql.php is to apply schema patches in a way you can control as an administrator, e.g. instead of update.php. The way you do that is by passing the patch file to sql.php. For that to work, it must expand table prefixes and table options, just like update.php would.
Having a maintenance script that can do this, is important to have and keep.
But, that also gets us to a number of other problems:
In fact: the maintenance/mysql.php script says:
* Note that this will not do table prefixing or variable substitution. * To safely run schema patches, use sql.php.
I suggest that we:
This feels like a good set of choices (including removing sql.php and pointing users to the other two alternatives.
(Do we have an equivalent of mysql.php for sqlite and postgress?).
I would suggest the opposite: keep sql.php, drop patchSql.php. I don't think many people are familiar with the latter (compare patchSql vs sql docs for example) and I don't think it's terribly useful - passing a file path is more user-friendly than passing a patch name. And it does not even replace the schema vars, meaning it's actively harmful to anyone who uses table prefixes or non-default table settings.
So, IMO
As said, I don't mind keeping sql.php as a cross-rdbms debug tool if there's interest for that.
However, I think use in production is problematic and should not be allowed for the same reason that echo <something> | mwscript eval.php shouldn't be used as part of production workflows. Such code will likely be untested, not reviewed by relevant maintainers, and not seen when change are made to the underlying code. We don't index the Puppet repository as part of "WMF-deployed MediaWiki code" and I don't think we should start. Data from a db table should generally be read via a PHP class that abstracts it. But short of that, at the very least with a minimal WMF-specific script in the WikimediaMaintenance extension.
The fact that that a Wikibase cron job is the only thing in production not following that, I think means this is something we generally follow but just didn't enforce or document in a way that new developers would have learned.
- keep sql.php for running scripts but require a --write flag like mysql.php does for scripts that change data. (I would even separate an admin mode for schema changes and a write mode for data changes via a restricted user.)
Also, when invoking sql.php in this way, it should always fatal in wmf production in the same way that update.php are disabled. Possibly by levering the same feature flag.
For the use case of patching, I think either of sql.php --write --admin --patch <filename> or patchSql.php <filename> would work. I do still prefer tha latter as it would make sql.php much easier to reason about.
We'd have a script that on the one side:
And on the other side:
Having the same maintenance script responsible for both seems like it could very well cause another outage in the future. Because so long as sql.php contains code that engages administrator credentials and initialises Installer classes, all it takes is one bad typo in a commit that changes sql.php and then the next person invoking it could be facing similar problems in the future. This is essentially Murphey's Law, which we've seen up close in T249565 where half a dozen edge cases lined up in just the right way to make it happen. Leaving this unplugged by itself likely won't cause any problems, but it's just something that could easily regress and we'd not know for many months until sometimes else ends up using it by mistake.
It also seems in terms of developer ergonimics that a separate endpoint would be easier to explain in documentation and make use of. I do agree that the current patchSql.php script is too hard to use, we'd model in after sql.php instead.
Change 587389 had a related patch set uploaded (by Krinkle; owner: Tim Starling):
[mediawiki/core@master] Revert "maintenance: Remove sql.php temporarily"
Change 587389 merged by jenkins-bot:
[mediawiki/core@master] Revert "maintenance: Remove sql.php temporarily"
This all makes sense to me. Since DDLs are not transactional in mariadb and so on, it adds an extra layer of disruption over "DELETE WHERE 1=1". Permissions with temp tables could get annoying though (Database.php read-only enforcement already has to look out for them). Starting off with a --write flag seems doable for now. It would be nice to make it use a replica DB by default (if --write is missing and --replicadb is not already set to a host or *).
Change 587213 merged by jenkins-bot:
[mediawiki/core@master] DatabaseUpdate: warn extensions about direct modification
Change 511778 abandoned by Daniel Kinzler:
[DNM] Register schema updates as callbacks during DatabaseUpdater construction
Reason:
Superceeded by I84f5fe6836a89352c3ff19c9a985ca359a5bf74d
@daniel I noticed a patch that makes some methods inaccesible to extensions. Does that mean this structure test is no longer needed? Or are there some dangerous methods exposed still?
Also, does the sql.php script now never run this hook?
See my comment on the patch: the structure test could still ensure that no dangerous methods are called on the Database object (rather than on DatabaseUpdater). The dangerous methods on DatabaseUpdater are no longer accessible to extensions, but they could still call Database::dropTable() directly.
Also, does the sql.php script now never run this hook?
The hook call was removed from the constructor of DatabaseUpdater, and is now triggered from doUpdates(). I see no way for sql.php to trigger it.
Change 507898 abandoned by Aaron Schulz:
Register schema update callbacks rather than running them on DatabaseUpdater construction
Reason:
obsolete