Copying from the merged-in duplicate:

In T256886#6273061, @Proc wrote:

Having taken a quick look at this, seems to be due to the $item->canView() checks failing (eg at SpecialRevisionDelete::showForm, line 420). That function is in RevDelRevisionItem, line 81:

return RevisionRecord::userCanBitfield(
	$this->getRevisionRecord()->getVisibility(),
	RevisionRecord::DELETED_RESTRICTED,
	$this->list->getUser()
);

Would probably have to do a couple of bit checks, but seems do-able - the various different fields suppressed result in a different bit value. Perhaps in a duplicate function, since canView can't take a parameter.

https://en.wikipedia.org/w/index.php?date-range-to=2019-04-04&tagfilter=&title=Khaled_Juffali&action=history

Similarly I want to zap the content of those revisions with the oversighted username for copyvio, but can't.

Finally had a chance to circle back to this. Unlike what @Proc had hoped, this will take a substantial change in code. The reason becomes clear when you see mw:Bitfields_for_rev_deleted#Current_values: we use the same bits to store the deletion and the suppression flag for each of the revision's three components (text, summary, user). With our current approach, it is not possible to configure a revision in such a way that its text is suppressed and its summary is deleted (but not suppressed).

To support concurrent use of rev-del and supression, we will need to change the revision deletion constants. One strategy is to use 1, 2 and 4 like we do now, but instead of adding 8 to them to indicate suppression, we could multiply the 1/2/4 bits by 100. The table below shows how the revision visibility bit will be calculated under the existing and proposed method. This proposal has a few advantages:

• Decouples the rev-del status and the suppression status (main purpose)
• Code will remain backward compliant; that is, the only values reused will be those which retain the same meaning
• There is no need for a maintenance script to update all revisions in the database. Every time a change is made the revision, the new value will be calculated using the new representation
• The math necessary to interpret these is not substantially more complex (or computationally more expensive); our current approach uses binary math; the proposed approach will be exactly identical for revisions that are not suppressed. For those that are suppressed, it will first separate the left-most digit from the rightmost digit (a simple division and modulo operation) and then uses the same old logic on each of those.

It has one key disadvantage:

• The rev_deleted field in the revision table is an 8-bit TINYINT field, which means it only supports values between 0 and 255. The proposed approach requires storage space for numbers as big as 700. We would need to ALTER the revision table, which would be an expensive Schema-change

We could use a more sophisticated algorithm that restricts the values to be between 0 and 255, but it would not be as clean. For instance, we can use a small multiplier of 16. This will also guarantee backward compliance, but the math to interpret it will be more involved.

I am happy to write the patch for it, but rather wait for some feedback first.

I am adding three people here, whom I would like to ask to opine on my last comment right above. Daniel is added because it seems like he was the one who created RevisionRecord and would know a lot about it. Max because he was among the last people to touch the code where RevisionRecord deletion constants are defined. Sam because he has always been extremely helpful to me and is generally a thoughtful developer.

TL;DR: should we revised our deletion constants to allow for coexistence of rev-del and suppression, and if yes, what strategy do you like better?

Might I suggest instead

Bitwise this results in the 8 bits:

<unused><unused><unused><unused>	<suppress rather than delete><user><comment><text>

<unused><suppress user><suppress comment><suppress text>	<mark legacy format><delete user><delete comment><delete text>

where the <suppress user/comment/text> bits only do anything if the <delete *> bit is activated. If the <delete user> bit is 1, and the <suppress user> bit is 0, user is visible to admins, but if the <delete user> bit is 1, and the <suppress user> bit is 1, it is visible only to oversighters.
This does not require a schema change, and should be completely backwards compatible since if the <mark legacy format> bit is true in the new definition, the old rules are used, and if it is false it doesn't matter because it is not a part of the new definition

I love that! Let me see if I can make a patch out of this one.

This will probably also need a maintenance script, and it might be worth tagging DBA to get feedback on the change before proceeding

My plan was to bring it to DBA attention once we have a patch. The proposal will not result in a schema change.

I agree regarding the maintenance script. Revisions that are deleted/suppressed are rarely touched again, in terms of their rev-del/suppress status. So while it is true that the code could update old entries to their new value if they were ever updated, because updates are rare, it is best to have a maintenance script.

However, the decision as to whether and when to run the script for WMF deserves a separate task. One could argue that because code will remain backward compliant, we might as well never update the revision table. This won't be the first time we have old and new data using different data models in the same DB field. In logging we have changed how some log properties are recorded over time. In AbuseFilter, we have similar examples too.

Fields using the same bit logic (as far as I can tell):
rev_deleted
ar_deleted
oi_deleted
fa_deleted
rc_deleted
log_deleted

I suggest adding a service for the migration, like for ActorMigration, but that handles setting and reading the values for the specific fields. Tagging CPT as a feature request to review, and my task workboard since I might be able to work on this (assuming this is approved to go forward; double checking first since this is a larger scope that previously expected)

Thanks Danny. I will be happy to provide support through code review, at least.
I retitled the task to reflect its new scope

@Ladsgroup would this be something you could work on in your new role?

I can consult on this on how to move forward here but I don't think I can take the lead on the work.

I can say from DBA side, making schema changes on revision table will be quite hard, alter tables on each one of replicas can take more than a day (on s8 and s1 replicas) and require a master switchover of each section. Most important point is to keep backward compatibility as the schema change will be gradual. I tried to do a draft of how data migration can be done a while ago: https://www.mediawiki.org/wiki/User:ASarabadani_(WMF)/Database_for_devs_toolkit/How-to/Data_migration

Anyway, the schema change part aside (as it might not be needed anyway). Generally it looks good but I really prefer doing T20493: RFC: Unify the various deletion systems first to ease the work for this (and much more benefits). That ticket is on my long-term plans of improving databases (T297633: <Tech Initiative> Improving Databases in MediaWiki) but won't get to it this year or even more unless other teams in WMF do it.

One last point is to make sure sanitation rules to the cloud (maintain-views) don't break specially because this means it might leak deleted or worse suppressed information to public.

T346293: ipblocks schema redesign for multiblocks propose to migrate the suppression state to actor table. If similar also happens in comment field, rev_deleted will only concern the text.