Visitar URL original
Fix problem with postgresql complaining about a wrong cast by jvillafanez · Pull Request #41051 · owncloud/core · GitHub
Skip to content

Fix problem with postgresql complaining about a wrong cast - #41051

Merged
jvillafanez merged 3 commits into
masterfrom
preview_cleanup_fix_postgres3
Oct 30, 2023
Merged

Fix problem with postgresql complaining about a wrong cast#41051
jvillafanez merged 3 commits into
masterfrom
preview_cleanup_fix_postgres3

Conversation

@jvillafanez

@jvillafanez jvillafanez commented Oct 17, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Description

The DBs can change the query plan whenever they want. In particular, the second condition, which was expected to be applied second, could be applied first. This means that the cast function could be applied to regular filenames instead of the "only digit" ones (which reference to file ids).
For postgresql, this situation causes an error because the cast function fails to convert a random string into an integer.

Since the issue depends on the DB's query planner, it could happen that some installations running prosgresql aren't affected. Big installations might not be affected because scanning the whole filecache table should cost higher than running an indexed filter (which might not be true for small installations)

Related Issue

#41040

Motivation and Context

We don't have any control over the DB's query planner, so we need to consider that the second condition is applied first even though it might not make sense on average.

How Has This Been Tested?

Manually tested on some DBs.
Unit tests pass on all the DBs.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@jvillafanez jvillafanez self-assigned this Oct 17, 2023
@update-docs

update-docs Bot commented Oct 17, 2023

Copy link
Copy Markdown

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@jvillafanez

jvillafanez commented Oct 17, 2023

Copy link
Copy Markdown
Member Author

Note
For sqlite, the "not exist" condition might get false positives if the filename starts with numbers: the cast will convert "123explode" to 123, so we'll consider such row as a thumbnail folder (which it isn't), however the "parent" check will fail so the row won't be part of the result (which is correct).
It might affect the performance because we might need to check additional items.
We'll leave it as it is because the query is already complex enough. Sqlite isn't recommended, so the possible performance loss shouldn't be a big deal. Need to include a comment in the code though.

@jvillafanez
jvillafanez force-pushed the preview_cleanup_fix_postgres3 branch from 3317647 to 054a3e6 Compare October 18, 2023 09:01
@jvillafanez
jvillafanez marked this pull request as ready for review October 18, 2023 10:43
@jvillafanez jvillafanez mentioned this pull request Oct 18, 2023
Closed
pako81
pako81 reviewed Oct 18, 2023
View reviewed changes
Comment thread changelog/unreleased/41051 Resolved
@pako81
pako81 requested a review from DeepDiver1975 October 18, 2023 13:23
@jvillafanez
jvillafanez force-pushed the preview_cleanup_fix_postgres3 branch from e294b87 to 2a5d641 Compare October 18, 2023 13:44
@sonarqubecloud

sonarqubecloud Bot commented Oct 18, 2023

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

81.8% 81.8% Coverage
0.0% 0.0% Duplication

@DeepDiver1975

DeepDiver1975 commented Oct 19, 2023

Copy link
Copy Markdown
Contributor

@jvillafanez I assume the slightly more complex where clause has no impact on the query plan .... ?

@jvillafanez

jvillafanez commented Oct 19, 2023

Copy link
Copy Markdown
Member Author

For 718 entries in the filecache table

                                                                                       QUERY PLAN                                    
                                                   
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 Sort  (cost=70.28..70.28 rows=1 width=42) (actual time=0.686..0.690 rows=5 loops=1)
   Sort Key: thumb.storage
   Sort Method: quicksort  Memory: 25kB
   ->  Nested Loop Anti Join  (cost=54.34..70.27 rows=1 width=42) (actual time=0.167..0.678 rows=5 loops=1)
         ->  Nested Loop  (cost=54.05..69.88 rows=1 width=42) (actual time=0.109..0.287 rows=91 loops=1)
               ->  HashAggregate  (cost=49.73..49.74 rows=1 width=8) (actual time=0.081..0.085 rows=6 loops=1)
                     Group Key: oc_filecache_1.fileid
                     Batches: 1  Memory Usage: 24kB
                     ->  Nested Loop  (cost=0.29..49.72 rows=1 width=8) (actual time=0.035..0.075 rows=6 loops=1)
                           ->  Seq Scan on oc_storages  (cost=0.00..16.45 rows=4 width=4) (actual time=0.012..0.016 rows=6 loops=1)
                                 Filter: (((id)::text ~~ 'home::%'::text) OR ((id)::text ~~ 'object::user:%'::text))
                                 Rows Removed by Filter: 9
                           ->  Memoize  (cost=0.29..8.30 rows=1 width=12) (actual time=0.009..0.009 rows=1 loops=6)
                                 Cache Key: oc_storages.numeric_id
                                 Cache Mode: logical
                                 Hits: 0  Misses: 6  Evictions: 0  Overflows: 0  Memory Usage: 1kB
                                 ->  Index Scan using fs_storage_path_hash on oc_filecache oc_filecache_1  (cost=0.28..8.29 rows=1 width=12) (actual time=0.007..0.007 rows=1 loops=6)
                                       Index Cond: ((storage = oc_storages.numeric_id) AND ((path_hash)::text = '3b8779ba05b8f0aed49650f3ff8beb4b'::text))
               ->  Bitmap Heap Scan on oc_filecache thumb  (cost=4.32..20.08 rows=6 width=50) (actual time=0.009..0.028 rows=15 loops=6)
                     Recheck Cond: (parent = oc_filecache_1.fileid)
                     Filter: (fileid > 0)
                     Heap Blocks: exact=56
                     ->  Bitmap Index Scan on fs_parent_storage_size  (cost=0.00..4.32 rows=6 width=0) (actual time=0.005..0.005 rows=16 loops=6)
                           Index Cond: (parent = oc_filecache_1.fileid)
         ->  Index Only Scan using oc_filecache_pkey on oc_filecache  (cost=0.29..0.35 rows=1 width=8) (actual time=0.002..0.002 rows=1 loops=91)
               Index Cond: (fileid = (regexp_replace((thumb.name)::text, '(^.*[^[:digit:]]+.*$|^$)'::text, ((thumb.fileid)::character varying(250))::text))::bigint)
               Heap Fetches: 40
 Planning Time: 1.176 ms
 Execution Time: 0.763 ms
(29 rows)

That would match more or less the first query plan in #41040 (comment) .
I'm still unsure how to trigger the second one

@jvillafanez
jvillafanez merged commit 62b5dee into master Oct 30, 2023
@delete-merged-branch
delete-merged-branch Bot deleted the preview_cleanup_fix_postgres3 branch October 30, 2023 11:27
DeepDiver1975 pushed a commit that referenced this pull request Nov 16, 2023
@jvillafanez @DeepDiver1975
Merge pull request #41051 from owncloud/preview_cleanup_fix_postgres3
d08083d 
Fix problem with postgresql complaining about a wrong cast
DeepDiver1975 pushed a commit that referenced this pull request Nov 17, 2023
@jvillafanez @DeepDiver1975
Merge pull request #41051 from owncloud/preview_cleanup_fix_postgres3
b523ac5 
Fix problem with postgresql complaining about a wrong cast
phil-davis pushed a commit that referenced this pull request Nov 22, 2023
@jvillafanez @phil-davis
Merge pull request #41051 from owncloud/preview_cleanup_fix_postgres3
b638cdd 
Fix problem with postgresql complaining about a wrong cast
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DeepDiver1975 DeepDiver1975 Awaiting requested review from DeepDiver1975
1 more reviewer
@pako81 pako81 pako81 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@jvillafanez jvillafanez

Labels

3 - To Review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jvillafanez @DeepDiver1975 @pako81