Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Fix thumbnails index#1946

Closed
jkolo wants to merge 2 commits into
matrix-org:developfrom
jkolo:fix_thumbnails_index
Closed

Fix thumbnails index#1946
jkolo wants to merge 2 commits into
matrix-org:developfrom
jkolo:fix_thumbnails_index

Conversation

@jkolo
Copy link
Copy Markdown
Contributor

@jkolo jkolo commented Feb 24, 2017

Based on PR #1816

@matrixbot
Copy link
Copy Markdown
Member

Can one of the admins verify this patch?

2 similar comments
@matrixbot
Copy link
Copy Markdown
Member

Can one of the admins verify this patch?

@matrixbot
Copy link
Copy Markdown
Member

Can one of the admins verify this patch?

@r3k2
Copy link
Copy Markdown

r3k2 commented Sep 7, 2017

I think im having this issue now.. #2442

@jkolo jkolo force-pushed the fix_thumbnails_index branch from ec2a345 to 28b158b Compare September 7, 2017 09:19
@jkolo
Copy link
Copy Markdown
Contributor Author

jkolo commented Sep 26, 2017

bump

@lukebarnard1
Copy link
Copy Markdown
Contributor

So it turns out that if the bug is due to the constraint in the DB, it should also be affecting the pre-generated thumbnails, given that we pre-generate for both "scale" and "crop" methods.

OR it's possible that we never pre-generate images of the same size but using different methods, which is looking likely, given the default is:

- width: 32
  height: 32
  method: crop
- width: 96
  height: 96
  method: crop
- width: 320
  height: 240
  method: scale
- width: 640
  height: 480
  method: scale
- width: 800
  height: 600
  method: scale

@lukebarnard1
Copy link
Copy Markdown
Contributor

Having added:

- width: 32
  height: 32
  method: scale

things seem to work without issue, so this is likely not the right fix.

Importantly, in local_media_cache_thumbnails we store actual thumbnail dimensions, which will vary for a given image using different methods. In remote_media_cache_thumbnails, we store the requested dimensions, which will not change for a given image using different methods, which means we can't insert a new entry.

So basically I think we only need to add the constraint to remote_media_cache_thumbnails. OR we can store the actual thumbnail sizes like we do for the local one but 🤷‍♂️.

@richvdh
Copy link
Copy Markdown
Member

richvdh commented Mar 7, 2019

Does anybody remember what exactly this is trying to fix?

@richvdh
Copy link
Copy Markdown
Member

richvdh commented Mar 22, 2019

looks like it is trying to fix #2182

@rubo77
Copy link
Copy Markdown
Contributor

rubo77 commented May 29, 2019

Please rebase!

also should you change the creation in

CREATE TABLE IF NOT EXISTS local_media_repository_thumbnails (

add thumbnail_method to the UNIQUE list

And does this really fix #2182 ? there is suggested another fix by @ananace: #2182 (comment)

ALTER TABLE remote_media_cache_thumbnails DROP CONSTRAINT remote_media_cache_thumbnails_media_origin_media_id_thumbna_key; CREATE INDEX remote_media_cache_thumbnails_media_origin_media_id_thumbna_key ON remote_media_cache_thumbnails (media_origin, media_id, thumbnail_width, thumbnail_height, thumbnail_type, thumbnail_method);

Can some admin take this over please? @jkolo isn't acitve siince months.

@richvdh
Copy link
Copy Markdown
Member

richvdh commented May 29, 2019

I'm pretty sure this an incorrect change, and has bitrotted.

@richvdh richvdh closed this May 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants