Differential D25051 Diff 2350 resources/sql/autopatches/20230917.fileattachment.01.delete.sql

Changeset View

Standalone View

resources/sql/autopatches/20230917.fileattachment.01.delete.sql

This file was added.

				USE {$NAMESPACE}_file;
				DELETE FROM file_attachment
				aviveyUnsubmitted Not Done Inline Actions I'm a little worried about this migration; Do we have garbage collector for this? it would be safer to let it run. avivey: I'm a little worried about this migration; Do we have garbage collector for this? it would be…
				EkubischtaUnsubmitted Not Done Inline Actions Agreed! Ekubischta: Agreed!
				DylsssAuthorUnsubmitted Done Inline Actions No there isn't any garbage collector for this specific issue (it's a bug originally from upstream so it's not intended for lingering file attachments to exist in the first place). I agree it's a destructive migration, although I don't see any other way to get rid of the orphaned records (which will clean up the UI spam in the Referenced Files curtain). I've ran the query a couple times locally, and as far as I can tell, it deletes the intended records which reference a file PHID which does not exist. Alternatively we could just change the UI logic to not show attachments that reference non-existent PHIDs, and leave the records left in the table, although I'd prefer not leaving a bunch of broken records using space/reducing efficiency. Dylsss: No there isn't any garbage collector for this specific issue (it's a bug originally from…
				valerio.bozzolanUnsubmitted Not Done Inline Actions Probably we can take some inspiration from this file: https://we.phorge.it/source/phorge/browse/master/src/infrastructure/daemon/garbagecollector/management/PhabricatorGarbageCollectorManagementCompactEdgesWorkflow.php valerio.bozzolan: Probably we can take some inspiration from this file: https://we.phorge.
				DylsssAuthorUnsubmitted Done Inline Actions I'm not sure a garbage collector would be any different to a migration, you'd still be running the same query but in just in different ways. Dylsss: I'm not sure a garbage collector would be any different to a migration, you'd still be running…
				aviveyUnsubmitted Not Done Inline Actions It's probably because I'm not comfortable enough with SQL, that I feel it would be safer to run the same query using PHP (but much slower): foreach ($load-all-file-attachments as $attachment) { $have_file = $load_file_with_phid($filePHID); if ($have_file) { $attachment->delete(); } } Running that periodically as a GC will also help(/hide) in any other cases where we generate bad attachments. avivey: It's probably because I'm not comfortable enough with SQL, that I feel it would be safer to run…
				valerio.bozzolanUnsubmitted Not Done Inline Actions it would be safer to run the same query using PHP The condition `WHERE phid=file_attachment.filePHID` is correct and deletes orphans. Running that periodically as a GC will also help(/hide) in any other cases where we generate bad attachments. I think it may be nice to just fix the past situation with a one-shot action, and then see what happens, without introducing a periodical garbage collector, that may just slow down the website to do nothing. Anyway the idea of the garbage collector is a very good B-plan if this patch will be not enough (but the patch seems to cover all future needs) valerio.bozzolan: > it would be safer to run the same query using PHP The condition `WHERE phid=file_attachment.
				WHERE NOT EXISTS
				(SELECT *
				FROM file
				WHERE phid=file_attachment.filePHID)