[Xzzz]

Summary

Stops writing flag changes to bugs_activity: new ones are only recorded in flag_activity, and everything that reads flag changes now reads both tables, bugs_activity for the changes up to a cutoff stored at checksetup and flag_activity for the later ones.

Part 2 of 1806896, whose part 1 (#2657) moved flag_state_activity to core as flag_activity and writes every flag change to both tables.

Changes

• Bugzilla::Bug and Bugzilla::Attachment no longer log flagtypes.name to bugs_activity; the user merge in editusers.cgi writes its flag changes to flag_activity.
• flag_activity.who, the user who made the change, becomes NOT NULL. #2808 (bug 2088705) added it as a nullable column and writes it on every change, so it must be deployed in an earlier release than this PR. On MySQL its foreign key is dropped for the change and added back at the end of checksetup.
• checksetup gives each flag that has no flag_activity row (flags last changed before flag_activity logged every type) a row with its current state, dated at its last change, in batches of 10,000 flags. History and bugmail don't show it, but the flag's next change gets its previous state. It then stores the cutoff, the time it ran, in the new one-row table flag_activity_migration, which also keeps it from running again.
• Bug history (GetBugActivity), bugmail (_get_diffs), the BMO user activity report and the UserProfile per-product counts read the flagtypes.name rows in bugs_activity up to the cutoff and the flag_activity rows after it (Bugzilla::FlagActivity->cutoff), so a change written to both tables, by a pod still on the older code during the deploy, is listed once. A request that changes a flag several times shows its net change, compared with the flag's last row before the request.
• Search changedbefore/changedafter/changedfrom/changedto/changedby/everchanged on flags, and the ReleaseTracking report, match either table.
• The BugModal activity stream finds a flag's last change by its flag_id in flag_activity.
• The other readers of a user's activity also read flag_activity: the non-bot last change time, the UserProfile stats, the TagNewUsers first activity, owner idle time, and changed* searches on any field.
• A flag type that has flag_activity rows can only be deactivated, not deleted, as its history would be deleted with it. The user deletion check in editusers.cgi counts flag_activity rows, and scripts/remove-non-public-data.pl keeps who.
• New tests: t/flag-activity-history.t, -search.t, -who.t, -user-stats.t, -stream.t, -start-rows.t and t/flag-type-delete.t. They set the cutoff with the new set_flag_activity_cutoff in Bugzilla::Test::Util.

The old flagtypes.name rows stay in bugs_activity; nothing is deleted.

Test plan

• Set a needinfo, a review on an attachment and an approval flag, then clear one: bug history, the activity stream and the bugmail show each change with its previous state, and bugs_activity gets no new flagtypes.name row
• Redirect a needinfo to someone else as another user: history shows that user
• On a bug with flag changes from before the deploy: history shows them once, with their previous state
• Search "Flags changed to needinfo?" and the user activity report find both older and new changes
• Merge two users with pending requests in editusers.cgi: the reassigned requests appear in bug history
• Delete a flag type that was used: only deactivation is offered

References

• Bugzilla link: 2085398
• Part 2 of 1806896
• Needs #2808 deployed first

[Xzzz]

Please note that the test_sanity critic failure is the $$ on master fixed by #2795. I'll re-run CI once it's merged, it should go green, unless I missed stg ;)

[Xzzz]

Also in this push, fixed some issues and optimized SQL request:

• c560c65: until July, flag_activity only logged review, feedback and needinfo, so other flags last changed before then had no row, and their next change showed no previous value (a clear disappeared from history). checksetup now gives each flag that has no row a starting one, with its current state, dated at its last change, in batches of 10,000 flags. History and bugmail don't show it, since it isn't newer than that change in bugs_activity.
• aca6a3b: bugmail skips flag changes that change nothing (a flag created and removed in one request), as history already did.
• f6a636a: changedfrom / changedto on any field also match flag changes.
• 98f9ae8: owner idle time uses three EXISTS instead of three joins, which multiplied each other's rows for active assignees.

The PR description is updated too.

[dklawren]

flags.modification_date is nullable but flag_activity.flag_when is NOT NULL, so a single flag with a null date makes checksetup die here, maybe use COALESCE(modification_date, creation_date)

[Xzzz]

Good catch, thanks!

Fixed in 873f583 with your COALESCE(modification_date, creation_date) and a new check in t/flag-activity-start-rows.t (checksetup dies without the fix).

[dklawren]

reuse FlagActivity->previous_id_sql here so changedfrom matches what history shows as removed (MAX(id) can pick an intermediate row from the same request)

[Xzzz]

Fixed in 2635ec3: changedfrom now uses previous_id_sql. It also uses last_of_request_sql, like changedto, so a request is one net change as in history. New checks in t/flag-activity-search.t, which fail without the fix.

[dklawren]

add last_of_request_sql (and skip no-op changes) so per-product counts match history, bugs_activity used to log one combined flag row per request

[Xzzz]

Fixed in 0059875: the per-product count now uses last_of_request_sql and skips no-op changes. New check in t/flag-activity-user-stats.t, which fails without the fix.

[dklawren]

literal '?' in the new CASE breaks the debug placeholder substitution further down, maybe build the status strings without a bare ?

[Xzzz]

Good catch, thanks!

Fixed in 97fa12e: the CASE no longer has a bare ?. It compares with CHAR(63) and builds the string from the status column.

Checked on MySQL with a throwaway test calling the report, as it needs CONVERT_TZ and so can't run in the SQLite tests, no committed test: the report rows are unchanged, and the debug SQL now has no ? left and runs, finding the flag rows. It fails without the fix.

[Xzzz]

Along with your comments, I fixed three other issues, each with a test:

• f290a46: _add_flag_activity_who now finishes on the next checksetup if it died partway. It was guarded only by the column existing, so who could stay nullable with NULLs,
• c5f3d23: flags set at creation no longer appear as changes in the first mail of a new bug, since lastdiffed is NULL and the flag rows had no time limit,
• 62a6b1b: changedby, delta_ts_non_bot, last_user_activity and touched skip flag_activity rows older than the last flag change in bugs_activity, as their who is only a guess from the setter.

The last one and the per-product count add a correlated subquery per row. It's covered by the index from the previous round, but I didn't measure it at production scale.

[dklawren]

per-bug MAX(bug_when) cutoff hides flag_activity rows whenever a later flagtypes.name row lands in bugs_activity, e.g. an old pod still writing both tables during the rolling deploy makes earlier flag_activity-only changes on that bug vanish from history, reports and changedby search, could we use a fixed cutoff stored at migration time instead

[Xzzz]

Yes sure, would a row in the params table (written once by checksetup, undeclared param then it won't appear in admin UI) works for you, or would you prefer a dedicated one-row table (most explicit but it's a schema change for a single value)?

[dklawren]

a dedicated one-row table please. an undeclared param doesn't survive well: migrate_params in Bugzilla/Config.pm drops params with no definition from the hash on every checksetup run and then pushes that hash to memcached, so Bugzilla->params loses the value even though the db row is still there (saving editparams does the same). a table like flag_activity_migration (cutoff DATETIME NOT NULL) filled once with NOW() next to the start rows, then read once per request via request_cache, keeps it out of the admin ui and is easy to drop later. with a fixed cutoff, bugs_activity flag rows need bug_when <= cutoff and flag_activity rows need flag_when > cutoff, and the per-row subquery and bugs_activity_bug_field_when_idx can probably go

[Xzzz]

Thanks, and you're right about migrate_params, I missed that. I'll do it as you describe:

• flag_activity_migration (cutoff DATETIME NOT NULL), filled once with LOCALTIMESTAMP(0) at the end of the start rows migration, which also uses it as its run-once guard. Read once per request through request_cache.
• flag_activity rows after the cutoff, and bugs_activity flag rows up to it in the readers that list or count changes: history, bugmail, the user activity report and the per-product counts. The ones that take a MAX or an EXISTS (non-bot last change, last user activity, touched, the search conditions, ReleaseTracking) only need the flag_activity side, as a change listed twice doesn't change their result.
• The per-row subquery and bugs_activity_bug_field_when_idx go away.

I'll push it with the NOT NULL step once #2808 is merged.

[Xzzz]

Done as described:
flag_activity_migration (cutoff) is filled with LOCALTIMESTAMP(0) at the end of the start rows migration, which uses it as its run-once guard, and Bugzilla::FlagActivity->cutoff reads it once per request. History, bugmail, the user activity report and the per-product counts read the bugs_activity flag rows up to the cutoff and the flag_activity rows after it. The other readers only use the flag_activity side. bugs_activity_bug_field_when_idx is gone.

The tests set the cutoff with a new set_flag_activity_cutoff helper in Bugzilla::Test::Util. The history and user stats tests also have a change after the cutoff written to both tables, as a pod on the older code does, and check that it's listed and counted once.

Fixed in 3c0d478

[dklawren]

during the rolling deploy old pods still insert into flag_activity without who. strict mode is turned off in Bugzilla/DB/Mysql.pm so mysql fills in 0, then the new fk to profiles rejects it and flag changes on old pods fail until the rollout ends (if the fk isn't there yet, the row is saved with who = 0 and the inner join on profiles in history drops it). could we ship this in two steps: first add who as nullable and write it while still writing bugs_activity, then make it NOT NULL and stop writing bugs_activity in a follow-up. or at least keep the column nullable for now and backfill who = setter_id WHERE who IS NULL OR who = 0 on every checksetup run

[Xzzz]

Right, thanks! I went with the two steps:

• Bug 2088705 - Record the acting user in flag_activity.who, as a nullable column #2808 (bug 2088705) adds who as a nullable column, with its foreign key and index, and writes it on every flag change, still writing bugs_activity. checksetup fills who = setter_id on the rows without it, on every run.
• Once it's deployed, this PR makes who NOT NULL (after checksetup has filled the rows left without it) and stops writing bugs_activity. During its rolling deploy, the old pods already write who.

So #2808 needs to go out in an earlier release than this one, I'll update this PR once #2808 is merged.

[Xzzz]

Done in 5c3d058

master with #2808 is merged in, and _set_flag_activity_who_not_null makes who NOT NULL right after _add_flag_activity_who has filled the rows left without it.

Checking it on MySQL showed one more thing: once #2808 has added the foreign key, MySQL refuses to make who NOT NULL ("Cannot change column 'who': used in a foreign key constraint"). The step now drops the key first, and bz_setup_foreign_keys adds it back at the end of checksetup, as the oauth2 migration does.

[dklawren]

a dedicated one-row table please. an undeclared param doesn't survive well: migrate_params in Bugzilla/Config.pm drops params with no definition from the hash on every checksetup run and then pushes that hash to memcached, so Bugzilla->params loses the value even though the db row is still there (saving editparams does the same). a table like flag_activity_migration (cutoff DATETIME NOT NULL) filled once with NOW() next to the start rows, then read once per request via request_cache, keeps it out of the admin ui and is easy to drop later. with a fixed cutoff, bugs_activity flag rows need bug_when <= cutoff and flag_activity rows need flag_when > cutoff, and the per-row subquery and bugs_activity_bug_field_when_idx can probably go