Luke Carrier, [17.12.18 17:03] I have an interesting problem for everyone — TRUNCATE TABLE on sqlsrv can result in the following if replication is enabled... anyone remember MDL-15073 and how it was tested? SQLState: 42000 Error Code: 4711 Message: [Microsoft][ODBC Driver 13 for SQL Server][SQL Server]Cannot truncate table 'snip' because it is published for replication or enabled for Change Data Capture. MoodleBot, [17.12.18 17:03] MDL-15073 (https://tracker.moodle.org/browse/MDL-15073) - M2: delete_records should truncate table of delete of all records requested Status: Closed - Fixed Assignee: Petr Skoda Reporter: Petr Skoda Fix Versions: 2.0 https://tracker.moodle.org/browse/MDL-15073 Eloy Lafuente Plaza, [17.12.18 17:32] Buff, for sure that was not tried (I bet). Looks like we'll need some dboption to control that truncate does not happen unconditionally . Or some checker built within all drivers if that can be guessed somehow (via dictionary tables or whatever). Luke Carrier, [17.12.18 17:35] @stronk7 is there a case to be made that we should never issue TRUNCATE TABLE? I guess this was intended as a performance hack, but it seems techncially incorrect that we'd knowningly reset a primary key and knowingly reuse PK values when we potentially have replication enabled? Luke Carrier, [17.12.18 17:36] Happy to contribute a patch to either sqlsrv_native_moodle_database or moodle_database as others are likely more knowledgeable, but I'm leaning towards never issuing `TRUNCATE`s Tim Hunt, [17.12.18 17:37] How many calls are there in the code that hit this? Tim Hunt, [17.12.18 17:38] It's not perfect, but it is almsot greppable Eloy Lafuente Plaza, [17.12.18 17:38] well, i can imagine use cases where the truncate behavior is handy. Say remove all logs or things like that. For sure it makes non-sense for tables having (our fake) FKs pointing to it. But can imagine many "final" tables where it's not a problem. In the other side, whoever wants to do so and is 100% sure can program its own $DB->execute ('truncate table xxxx'); Not sure which uses we have @ core. Eloy Lafuente Plaza, [17.12.18 17:38] that's the point Eloy Lafuente Plaza, [17.12.18 17:40] $ ag 'delete_records\([^,]*\)' | wc -l 460 Eloy Lafuente Plaza, [17.12.18 17:40] 🙂 Tim Hunt, [17.12.18 17:41] That seems like a lot Eloy Lafuente Plaza, [17.12.18 17:41] indeed Tim Hunt, [17.12.18 17:41] To the point where I woudl ve very suspicious o fthe grep Eloy Lafuente Plaza, [17.12.18 17:41] $ ag 'delete_records\([^,\)]*\)' | wc -l 67 Eloy Lafuente Plaza, [17.12.18 17:42] (missed to add the ) closer, so it was capturing too many lines) Luke Carrier, [17.12.18 18:01] [In reply to Eloy Lafuente Plaza] This sounds like a fallacy — we're optimising for the case that (almost, maybe) never happens Tim Hunt, [17.12.18 18:02] Well, have you reviewed those cases? Tim Hunt, [17.12.18 18:03] Agreed they are rare, but that does not mean they are necessarily unimpaortant. (But if you found that they were, that would be good justification.) sam marshall, [17.12.18 18:12] If you do delete logs, definitely 'DELETE FROM' is not acceptable performance/space-wise with a large and highly indexed log table, so that is a real case not a nonexistent one. I thought that's exactly what Moodle does by default though :) We hacked it to use different tables each month (ish) so it can drop the whole table, Eloy Lafuente Plaza, [17.12.18 18:16] [In reply to Luke Carrier] Is that philosophical talking? Or what? 67 cases in core are. Eloy Lafuente Plaza, [17.12.18 18:17] and glad to get them reviewed we are Eloy Lafuente Plaza, [17.12.18 18:17] LOL Luke Carrier, [17.12.18 18:17] [In reply to Eloy Lafuente Plaza] In terms of technical correctness it seems questionable. To issue truncated on a live DB Eloy Lafuente Plaza, [17.12.18 18:23] to question things is nice, heh. Although in this case it seems your question maybe is questionable too. Sorry, joking (up to a point). I'm happy if that problem is visited and then we come to an agreement about how to proceed. 1) removing the performance hack (-1 here, right now). 2) making it manually skippable. 3) making it automatically skippable. 4) providing an alternative, explicit truncate_table() method for whenever we want to use it (still needing 2 and 3 for sure). Eloy Lafuente Plaza, [17.12.18 18:23] (sorry gtg now... feel free to create an issue...) Andrew Nicols, [17.12.18 21:57] [In reply to Luke Carrier] are you sure it resets PKs? Doesn’t do so in most DBs AFAIK Andrew Nicols, [17.12.18 22:00] Looks to be DB-dependent. Postgres does not reset them; MySQL does. Andrew Nicols, [17.12.18 22:01] Oracle doesn’t. MSSQL does. Luke Carrier, [17.12.18 22:11] [In reply to Andrew Nicols] Thanks for looking at this Andrew... was about to say I'd take a look tomorrow now Luke Carrier, [17.12.18 22:12] Would you rather keep truncate where it doesn't pose a risk to replication and make exceptions for SQL Server and MySQL? I really think we ought to avoid it at all as it's more of a DDL operation than DML, but I can see an argument in favour of the perf gain despite the correctness iffyness Andrew Nicols, [17.12.18 22:17] Not really sure. It ignores FK constraints and is not transaction safe, but we don’t generally respect FK constraints in Moodle, and rarely use transactions. Maybe for those that support it we should use it where no txn but @stronk7 has more knowledge in this area. Luke Carrier, [17.12.18 22:18] I'm about to roll out a workaround patch to our prod envs confined to sqlsrv_native_moodle_database... I'll hold off on upstreaming this until @stronk7's weighed in then. Cheers :) Eloy Lafuente Plaza, [18.12.18 09:18] [In reply to Andrew Nicols] Well, I assume that whoever wants to delete all the records from a table knows what's doing (in the context of Moodle). Said that, about ID sequences being reset or no... and from a pure practical perspective... I cannot imagine any difference if they are - or are not. In both cases, if the deletion was done causing some records elsewhere to become "orphaned" (breaking the "virtual" reference integrity)... then I would say the problem is the deletion, irrespectively of it being a delete or a truncate. Do we want to make the "don't reset sequences" on deletion a cross-db thing? I'm ok with that, but for sure it's not high priority, neither should make any difference (if the deletion was performed programmatically considering all the "virtual" FKs.) Basically this and my 1-4 original points are my thoughts. Andrew Nicols, [18.12.18 09:36] [In reply to Eloy Lafuente Plaza] I think the issue is that it breaks mssql replication though so probably only need to fix there Eloy Lafuente Plaza, [18.12.18 09:38] that's the 3) in my points... if it can be programmatically detected when publishing/replication is enabled... then ok to confine the fix there. Although 2) manually configuring the truncate to be avoided, could also help in some other unknown cases.