Page MenuHomePhabricator

filerevision view should not filter out deleted file revisions
Closed, ResolvedPublic

Description

The filerevision views should have the same data available as the existing image and oldimage views instead of only including fr_deleted = 0. This information is also available via the web UI and action API.

Current:

filerevision:
  source: filerevision
  view:
    select fr_id, fr_file, fr_size, fr_width, fr_height, fr_metadata, fr_bits, fr_description_id, fr_actor, fr_timestamp, fr_sha1, fr_archive_name, fr_deleted
  where: >
    fr_deleted = 0

Proposed:

filerevision:
  source: filerevision
  view: >
    select fr_id, fr_file, fr_size, fr_width, fr_height, fr_metadata, fr_bits,
    if(fr_deleted&2,0,fr_description_id) as fr_description_id,
    if(fr_deleted&4,null,fr_actor) as fr_actor,
    fr_timestamp, fr_sha1, fr_archive_name, fr_deleted
filerevision_userindex:
  source: filerevision
  view: >
    select fr_id, fr_file, fr_size, fr_width, fr_height, fr_metadata, fr_bits,
    if(fr_deleted&2,0,fr_description_id) as fr_description_id,
    fr_actor, fr_timestamp, fr_sha1, fr_archive_name, fr_deleted
  where: (fr_deleted&4)=0

Reference:

image:
  source: image
  view: >
    select img_name, img_size, img_width, img_height, img_metadata, img_bits,
    img_media_type, img_major_mime, img_minor_mime,
    coalesce(img_description_id,0) as img_description_id,
    img_actor, img_timestamp, img_sha1
[...]
oldimage:
  source: oldimage
  view: >
    select oi_name, oi_archive_name, oi_size, oi_width, oi_height, oi_bits,
    if(oi_deleted&2,0,oi_description_id) as oi_description_id,
    if(oi_deleted&4,null,oi_actor) as oi_actor,
    oi_timestamp, oi_metadata, oi_media_type,
    oi_major_mime, oi_minor_mime, oi_deleted, oi_sha1
[...]
oldimage_userindex:
  source: oldimage
  view: >
    select oi_name, oi_archive_name, oi_size, oi_width, oi_height, oi_bits,
    if(oi_deleted&2,0,oi_description_id) as oi_description_id,
    oi_actor, oi_timestamp, oi_metadata, oi_media_type, oi_major_mime,
    oi_minor_mime, oi_deleted, oi_sha1
  where: (oi_deleted&4)=0

Related Objects

View Standalone Graph
This task is connected to more than 200 other tasks. Only direct parents and subtasks are shown here. Use View Standalone Graph to show more of the graph.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

I'd say if Privacy Engineering team is happy, I have no objections.

sbassett added a project: SecTeam-Processed.
sbassett subscribed.

I'd say if Privacy Engineering team is happy, I have no objections.

+1 from Privacy Engineering and Security-Team.

What does fr_deleted=1 mean? It's not super clear to me from reading the code, but it seems to me that it might mean that the file's entire history is deleted. If that's the case, should we still exclude rows with fr_deleted=1 entirely?

What does fr_deleted=1 mean? It's not super clear to me from reading the code, but it seems to me that it might mean that the file's entire history is deleted. If that's the case, should we still exclude rows with fr_deleted=1 entirely?

It means the file is deleted and the rows are moved to filearchive (so it it gets restored, the file id wouldn't change). There are some complexities on when the file is deleted but another existing file is moved over but I spare you the details. TLDR is that we want to prepare for {T20493]

On whether it should be fully removed, sorta yeah but I think the argument here is that deleted data as long as it's not the text or actual content is harmless. See that we expose archive table too. But it is something I'm honestly not 100% comfortable with

OK, as long as it's consistent with what we're already doing for other types of content, I'm fine with it.

Zabe added a project: DBA.

Change #1296029 had a related patch set uploaded (by Zabe; author: Zabe):

[operations/puppet@production] maintain-views: Loosen views for filerevision table

https://gerrit.wikimedia.org/r/1296029

Change #1296029 merged by Ladsgroup:

[operations/puppet@production] maintain-views: Loosen views for filerevision table

https://gerrit.wikimedia.org/r/1296029

Cookbook cookbooks.sre.wikireplicas.update-views run by ladsgroup: Started updating wiki replica views

Cookbook cookbooks.sre.wikireplicas.update-views started by ladsgroup completed:

  • an-redacteddb1001.eqiad.wmnet (PASS)
    • Ran Puppet agent
    • Ran 'maintain-views --replace --auto-depool --all-databases --table filerevision'
  • clouddb1017.eqiad.wmnet (PASS)
    • Ran Puppet agent
    • Ran 'maintain-views --replace --auto-depool --all-databases --table filerevision'
  • clouddb1018.eqiad.wmnet (PASS)
    • Ran Puppet agent
    • Ran 'maintain-views --replace --auto-depool --all-databases --table filerevision'
  • clouddb1020.eqiad.wmnet (PASS)
    • Ran Puppet agent
    • Ran 'maintain-views --replace --auto-depool --all-databases --table filerevision'
  • clouddb1023.eqiad.wmnet (PASS)
    • Ran Puppet agent
    • Ran 'maintain-views --replace --auto-depool --all-databases --table filerevision'
  • clouddb1025.eqiad.wmnet (PASS)
    • Ran Puppet agent
    • Ran 'maintain-views --replace --auto-depool --all-databases --table filerevision'
  • clouddb1013.eqiad.wmnet (PASS)
    • Ran Puppet agent
    • Ran 'maintain-views --replace --auto-depool --all-databases --table filerevision'
  • clouddb1014.eqiad.wmnet (PASS)
    • Ran Puppet agent
    • Ran 'maintain-views --replace --auto-depool --all-databases --table filerevision'
  • clouddb1015.eqiad.wmnet (PASS)
    • Ran Puppet agent
    • Ran 'maintain-views --replace --auto-depool --all-databases --table filerevision'
  • clouddb1016.eqiad.wmnet (PASS)
    • Ran Puppet agent
    • Ran 'maintain-views --replace --auto-depool --all-databases --table filerevision'
  • clouddb1022.eqiad.wmnet (PASS)
    • Ran Puppet agent
    • Ran 'maintain-views --replace --auto-depool --all-databases --table filerevision'
  • clouddb1024.eqiad.wmnet (PASS)
    • Ran Puppet agent
    • Ran 'maintain-views --replace --auto-depool --all-databases --table filerevision'

Cookbook cookbooks.sre.wikireplicas.update-views run by ladsgroup: Started updating wiki replica views

Cookbook cookbooks.sre.wikireplicas.update-views started by ladsgroup completed:

  • an-redacteddb1001.eqiad.wmnet (PASS)
    • Ran Puppet agent
    • Ran 'maintain-views --replace --auto-depool --all-databases --table filerevision_userindex'
  • clouddb1017.eqiad.wmnet (PASS)
    • Ran Puppet agent
    • Ran 'maintain-views --replace --auto-depool --all-databases --table filerevision_userindex'
  • clouddb1018.eqiad.wmnet (PASS)
    • Ran Puppet agent
    • Ran 'maintain-views --replace --auto-depool --all-databases --table filerevision_userindex'
  • clouddb1020.eqiad.wmnet (PASS)
    • Ran Puppet agent
    • Ran 'maintain-views --replace --auto-depool --all-databases --table filerevision_userindex'
  • clouddb1023.eqiad.wmnet (PASS)
    • Ran Puppet agent
    • Ran 'maintain-views --replace --auto-depool --all-databases --table filerevision_userindex'
  • clouddb1025.eqiad.wmnet (PASS)
    • Ran Puppet agent
    • Ran 'maintain-views --replace --auto-depool --all-databases --table filerevision_userindex'
  • clouddb1013.eqiad.wmnet (PASS)
    • Ran Puppet agent
    • Ran 'maintain-views --replace --auto-depool --all-databases --table filerevision_userindex'
  • clouddb1014.eqiad.wmnet (PASS)
    • Ran Puppet agent
    • Ran 'maintain-views --replace --auto-depool --all-databases --table filerevision_userindex'
  • clouddb1015.eqiad.wmnet (PASS)
    • Ran Puppet agent
    • Ran 'maintain-views --replace --auto-depool --all-databases --table filerevision_userindex'
  • clouddb1016.eqiad.wmnet (PASS)
    • Ran Puppet agent
    • Ran 'maintain-views --replace --auto-depool --all-databases --table filerevision_userindex'
  • clouddb1022.eqiad.wmnet (PASS)
    • Ran Puppet agent
    • Ran 'maintain-views --replace --auto-depool --all-databases --table filerevision_userindex'
  • clouddb1024.eqiad.wmnet (PASS)
    • Ran Puppet agent
    • Ran 'maintain-views --replace --auto-depool --all-databases --table filerevision_userindex'
Ladsgroup moved this task from Triage to Done on the DBA board.