[16:39:27] Does anyone know offhand if the mysql->sqlite munger deals with `AFTER` in alter table? [17:19:13] I can't see anything obvious in DatabaseSqlite::replaceVars [17:25:35] Reedy: I vaguely recall that schema patches do still have separate file for mysql vs sqlite [17:25:50] Which though? ;p [17:26:00] If that is indeed the case, that probably means there hasn't been much motiviation to make that statement work as its unexpected at runtime [17:26:11] The exception being tables.sql [17:26:31] https://www.sqlite.org/lang_altertable.html [17:26:47] Most of the files for sqlite are the same as mysql [17:26:57] It's if you need to change the definition of a column [17:27:10] Reedy: sqlite patches (speaking from memory) tend to create new, import and rename or something like that. [17:27:17] Yes, for a definition change [17:27:18] or maybe we don't care about column order? [17:27:34] You can use ALTER TABLE to just add a column on sqlite though [17:28:10] we do indeed have archive/sqlite/patches separately [17:28:13] so that supports that theory [17:28:35] if there is stuff like 'ALTER' being piped through our munger, that sounds like a bug. [17:28:38] afaik that wouldn't be supported. [17:28:46] We do some mungling (thanks at least in part to MaxSem) of MySQL SQL into SQLite [17:29:04] yeah, for runtime queries and for initial schema definition. [17:29:13] e.g. select/insert/update/delete and create tables. [17:29:32] https://github.com/wikimedia/mediawiki/blob/master/includes/libs/rdbms/database/DatabaseSqlite.php#L932-L983 [17:29:39] That suggests we don't touch alter [17:30:02] It's just the usual issue of trying to find a $random extension that does it or not [17:30:12] Indeed. for schema changes we require explicit sql patches, which are not meant to rely on munging [17:30:19] and are created explicitly for sqlite [17:30:21] not re-used [17:30:56] did you run into a problem? [17:31:07] but again, https://www.sqlite.org/lang_createindex.html will work with a mysql syntax [17:31:18] I was clearing up the OAuth gerrit changes [17:31:35] Gergo did https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/OAuth/+/255492/ years ago [17:31:57] I've re-done it in https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/OAuth/+/564064/ [17:32:24] But wasn't sure with the ALTER TABLE with `AFTER` syntax could be de-duplicated [17:37:23] I think what confused me... Was in https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/OAuth/+/255492/ he (for whatever reason,I haven't checked what the files were all like back then) one mysql alter table and one sqlite alter table [17:37:39] And https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/OAuth/+/255492/2/backend/schema/developer_agreement.sql,unified wouldn't have worked for sqlite (unless we munged it) [17:37:48] So I dunno if it was just a mistake etc [17:38:25] yeah, afaik that wouldn't have worked. [17:38:54] I don't know if there's a subset of schema changes that happens to work today with the same patch but either way I would consider that unofficial/fragile. [17:38:54] https://github.com/wikimedia/mediawiki-extensions-OAuth/tree/da8d90c0ce21d1f3f094b4e4ff45e1082552acd8/backend/schema [17:39:04] There are probably some... [17:39:11] That just add it to the "end" of the table [17:39:12] Afaik current practice is for changes to the schema to always be made explicitly for each supported backend. [17:39:14] And that all works fine, mostly [17:39:29] so initial definition shared between mysql/sqlite, any and all changes to it, separate [17:39:32] Until you start doing schema changes with select * and shit ends up in the wrong column [17:39:39] (which we had in MW core a little while ago) [17:39:50] So yeah, I guess this at least confirms what I was thinking [17:39:59] The patches I de-duped are fine, the rest need to stay :) [17:40:37] As the files are (basically) identical [17:40:50] well, except [17:40:51] diff mysql/OAuth.sql sqlite/OAuth.sql [17:40:51] 100c100 [17:40:51] < -- Access token identifier [17:40:51] --- [17:40:54] > -- Access token [17:40:56] which is kinda lol [23:20:51] Reedy would you be able to re-review https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/Flow/+/561695/ please? :) [23:21:51] Reedy: very likely a mistake, yes. Thanks for resurrecting that patch :) [23:22:56] tgr: Feel free to +2 my replacement ;P [23:23:11] There's a few other old patches for OAuth that probably want abandoning [23:26:06] I still have probably unreasonable hope that I will find time to finish my monstre workflow refactoring patch